search

TGMPA / TGM-Plugin-Activation

TGM Plugin Activation is a PHP library that allows you to easily require or recommend plugins for your WordPress themes (and plugins). It allows your users to install, update and even automatically activate plugins in singular or bulk fashion using native WordPress classes, functions and interfaces. You can reference bundled plugins, plugins from the WordPress Plugin Repository or even plugins hosted elsewhere on the internet.
http://tgmpluginactivation.com/
GNU General Public License v2.0
1.76k stars 428 forks source link

Improve Separation of Concerns #804

Open XedinUnknown opened 3 years ago

XedinUnknown commented 3 years ago

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:

  1. 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 of TGM_Plugin_Activation, not the one I have configured.

    This can easily be solved by dependency-injecting TGM_Plugin_Activation into the list.

  2. 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.

jrfnl commented 3 years ago

TGMPA is single instance by design. Changing that is not open for discussion.

XedinUnknown commented 3 years ago

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 tgmpa() function, a single instance is used, and nothing changes. What has changed is that the dependencies between classes are now explicit, and string configuration happens earler - as early as possible. What exactly is wrong with this?

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 TGMPA_Plugin_Activation::get_instance() method. But that doesn't mean that the classes have to be inherently singletons, thus preventing multiple independent instances from being created and used. It's just architecture, regardless of how many instances you need to have in your app - whether one or many. It's an app's decision, ultimately.

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.

jrfnl commented 3 years ago

What exactly is wrong with this?

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 ?

XedinUnknown commented 3 years ago

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 TGMPA_Bulk_Installer::__construct() and TGMPA_List_Table::__construct(), which are used in this library exclusively internally. Even then, it would be trivial to make the new arguments optional, defaulting to the global instance. This is exactly the kind of discussion aimed for improvement that I was hoping to have 😃. Would that help get this approved?

XedinUnknown commented 3 years ago

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.