-
Notifications
You must be signed in to change notification settings - Fork 427
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve Separation of Concerns #804
Comments
This allows the default strings to be overridden via configuration after instantiation but before they are used. TGMPA#804
This allows each list to use its corresponding TGMPA instance, instead of all lists using the same global instance. TGMPA#804
This allows each bulk installer to use its corresponding TGMPA instance, instead of all installers using the same global instance. TGMPA#804
…ditionally Classes should be available for use by whatever needs them, on any page. Simply declaring them if they don't already exist facilitates that capability. WordPress will not try to include it again, because it is done with `require_once`: https://github.com/WordPress/WordPress/blob/270f2011f8ec7265c3f4ddce39c77ef5b496ed1c/wp-admin/includes/class-wp-upgrader.php#L953 TGMPA#804
TGMPA is single instance by design. Changing that is not open for discussion. |
Unfortunately, having it be a single instance raises many problems and questions. In particular, what happens when different plugins configure it differently. Using a single global instance should still be possible with the proposed change, because the global functions still use the global instance. The only thing that changed was an architectural improvement. When using the documented way of configuration, i.e. via the A singleton is a good and valid pattern, and if it is desirable for an application to have only one instance of something, then this is what it should use, and this library provides a way to ensure that - by using the Therefore, I encourage you to take a look and verify that everything still works as before. And if you are feeling adventurous, I encourage you to try creating and configuring multiple different instances - and see that it also works, and provides benefits for anyone who would prefer to use it that way. |
Aside from the fact that you clearly didn't think it was necessary to read the discussions about this which have been had previously ? Or that you didn't think such a structural change worth discussing before creating a PR ? Or that you think that making breaking signature changes in public methods which are being used in integrations is something which is acceptable without issue ? |
I didn't think it was necessary or unnecessary. I just made an improvement. It's a contribution, and if it is undesirable, I would appreciate an explanation or a link to such. What signatures did I break, by the way? I don't remember making breaking changes to any public methods, aside from |
Hi @jrfnl! Any news on this? Hoping that perhaps you are more free now that Requests have received an update 😃 Also, just want to remind you that this doesn't change anything at all with regard to regular use of the functionality by other developers, but simply improves the architecture such that the classes can be used more flexibly on their own too. |
The Problem
The functionality of this library is very useful. It allows developers to quickly specify the plugin dependencies of their plugin or theme, and provide users with an intuitive way to quickly install these dependencies.
Unfortunately, having TGMPA in a plugin introduces a risk of conflict with other plugins that use TGMPA. This is especially true with string customization, as one plugin's strings would overwrite TGMPA string configuration done by another plugin.
Suggested Solution
The approach I took to ensure that my TGMPA instance does not conflict with others (apart from different versions of this same lib, which is kinda inevitable for the most part) is to have a separate instance of
TGM_Plugin_Activation
for my plugin. This instance does not overwrite the global instance, and thus all my configuration is preserved separately. This results in an additional notice and menu item per plugin [that uses a separate instance], which is arguably better: each plugin or theme has a different list of dependencies, and updating all dependencies of all plugins in one go is not necessarily what the user wants.However, I've encountered the following issues:
On the "Install Plugins" page, the lists for both plugins to update and install are empty, even if the notice is displayed correctly. This is because
TGMPA_List_Table
, much like all the other parts of this library, uses the global instance ofTGM_Plugin_Activation
, not the one I have configured.This can easily be solved by dependency-injecting
TGM_Plugin_Activation
into the list.Custom strings have no effect, at least on the notice. This is because the
TGM_Plugin_Activation
instance is configured with default strings after its initialization, and therefore normal configuration, is complete. So, if I configure my instance with strings right after instantiating it, those strings would be overwritten later with defaults - because the default strings are assigned on init.This can easily be solved by assigning default strings in the constructor, and then letting initialization overwrite them (as defaults should work IMHO), subsequently using that configuration as is. Arguably, all configuration should be done during initialization, preferably by dependency-injecting it, or less favourably by assigning defaults as close as possible to the constructor.
The text was updated successfully, but these errors were encountered: