-
Notifications
You must be signed in to change notification settings - Fork 3.2k
Allow force ignoring of global caching groups in WP_Object_Cache #2149
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
base: trunk
Are you sure you want to change the base?
Allow force ignoring of global caching groups in WP_Object_Cache #2149
Conversation
…dding constant in wp-config.php
|
Where would the required constant be placed best in the Unit tests? In the |
Co-authored-by: Till Krüss <[email protected]>
Co-authored-by: Till Krüss <[email protected]>
Co-authored-by: Till Krüss <[email protected]>
|
@tillkruss Thanks for your feedback. The QS check made me remember, why I did it this way: |
Right, that's annoying. Not sure what the core standards are here. Maybe supporting comma separated strings? |
Hold the thought for now, @hellofromtonya and I have had some very preliminary discussions about whether to re-propose a minimum version bump.
I'm not sure there is one. A few popular memcached plugins use a simple global variable for defining multiple servers in the config file. A comma seperated value exploded and trimmed could work but doesn't seem the most optimal code. I was going to propose a filter but then remembered that would be too late for the object cache. A constant is most likely the best option. |
As written in the ticket, this feature is a very edge case and will most probably only be used by people who "know what they do" and thus do not use PHP5 anymore for a long time ;-)
Exactly ;-) Tricky timing there... What about this: I will revert back to default the constant to |
|
Summarizing the chat @JanThiel and I had in Slack DM: The goal: give advanced devs the ability to configure which global groups they want to use. Limitations:
Risks:
A constant is not testableA PHP constant is not testable as it can only be defined once and never changed during the execution lifecycle. This includes running tests in a separate process. Why not testable? Because a constant's value cannot be reset between tests, meaning it could impact other tests. How to make it testable and available to devs across all supported PHP versions?Use a global variable instead of a constant. I don't like global variables either because they are easily changeable in plugins and themes and therefore require more guarding considerations. But in this case, it does fit the need. Why?
Recommendation and discussionI'm leaning towards recommending a global variable to be added to the @peterwilsoncc @JanThiel what do you think? |
|
An optional global variable in the format of the existing |
|
Thank you very much @hellofromtonya. I will change the PR accordingly. |
|
@JanThiel can you rebase your PR against |
|
Whoops, didn't mean to close it. Re-opening 🤦♀️ |
…_cache_allow_global_group_filtering
|
@hellofromtonya Trunk is merged and the open codestyle fix is also applied |
|
@hellofromtonya Is there anything we can do to push this forward? @spacedmonkey Any feedback? |
|
So I am having some half-thoughts here... The plugin API is a really neat standalone library that can be included in any PHP code base, not just WordPress. As such, it can even be bootstrapped really early in the /* wp-config.php */
// Load the plugin API (like add_action etc) early.
require_once __DIR__ . '/wp-includes/plugin.php';
// ** Database settings - You can get this info from your web host ** //
/** The name of the database for WordPress */
define( 'DB_NAME', 'database_name_here' );
/** Database username */
define( 'DB_USER', 'username_here' );
/** Database password */
define( 'DB_PASSWORD', 'password_here' );
// ETC, ETCWordPress currently has the As Jan mentioned above, this is a feature likely to be used by people who know what they are doing. Is it worth making the global and persistent groups filterable only if the site owner includes the plugin API in their wp-config file? (Aside: please do not do this unless you know what you are doing.) With the WordPress Performance focus, there is a chance some of the non-persistent and global groups will be changing (for example Jonny has me reviewing #2478) so a filter would be more robust if additional groups are added to the existing default. If a variable is set to Using a filter will allow a developer who knows what they are doing to add or remove groups without risk. Thoughts? |
|
@peterwilsoncc: I'm with you on the logic, but the |
|
@tillkruss Isn't the idea to allow users to define the default values in these lines, they only run once during wordpress-develop/src/wp-includes/load.php Lines 732 to 735 in f2697ea
Placing a filter within |
|
That would be great! Thanks for clarifying 🤙 |
Allow force ignoring of global caching groups in WP_Object_Cache by adding constant in wp-config.php
Trac ticket: https://core.trac.wordpress.org/ticket/54303
This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.