Skip to content
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

Allow Per-Instance Configuration #805

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

XedinUnknown
Copy link

@XedinUnknown XedinUnknown commented Apr 27, 2021

Fixes #804.

In general, this change simply shifts things to where they should be in order to properly configure multiple instances of TGMPA.

  • TGMPA string configuration now overridable during configuration (initialization).
  • Update/install/activate lists reflect requirements of the correct configuration.
  • Updating/installing/activating in bulk works on the correct configuration.

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:/WordPress/WordPress/blob/270f2011f8ec7265c3f4ddce39c77ef5b496ed1c/wp-admin/includes/class-wp-upgrader.php#L953

TGMPA#804
@XedinUnknown
Copy link
Author

The build seems to be failing due to misconfigured CI: some packages require Composer v1, while Composer v2 is used on CI.

@jrfnl
Copy link
Contributor

jrfnl commented Apr 27, 2021

As per #804 (comment):

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

I suggest closing this PR.

@XedinUnknown
Copy link
Author

As I have explained, nothing should change for the current users with this PR. Everyone should be able to continue using the single instance. Also, a class is not single-instance by design; the single-instance-ness is incidental, in that you can force a class to only have a single instance through high coupling etc, but this has no benefits while simply limiting consumers for no apparent reason. Your component (a part of an application, like a plugin or this lib) surely can use only a single instance of something, and can provide to its consumers ways to maintain consistency through the single instance - like by initializing an object, caching it, and always retrieving from cache. In this case, the tgmpa global is being used for that cache. So your component remains the same in the sense that it continues using only a single instance, and all of the consumers that follow your docs will thus use a single instance.

Given the above, I do not understand the relevance of your comment, sorry. Would you mind explaining exactly what the problem is, e.g. where in any code in this lib, including my PR, you see that it is no longer using a single instance?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve Separation of Concerns
2 participants