Skip to content

Conversation

@JanThiel
Copy link

@JanThiel JanThiel commented Jan 12, 2022

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.

@JanThiel
Copy link
Author

Where would the required constant be placed best in the Unit tests? In the bootstrap.php ?

@JanThiel
Copy link
Author

@tillkruss Thanks for your feedback. The QS check made me remember, why I did it this way:
array() as constant content is a PHP7+ feature. I believe we must not assume PHP7 as a min requirement for WordPress at the moment.

@tillkruss
Copy link
Member

The QS check made me remember, why I did it this way: array() as constant content is a PHP7+ feature. I believe we must not assume PHP7 as a min requirement for WordPress at the moment.

Right, that's annoying. Not sure what the core standards are here. Maybe supporting comma separated strings?

@peterwilsoncc
Copy link
Contributor

I believe we must not assume PHP7 as a min requirement for WordPress at the moment.

Hold the thought for now, @hellofromtonya and I have had some very preliminary discussions about whether to re-propose a minimum version bump.

Not sure what the core standards are here. Maybe supporting comma separated strings?

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.

@JanThiel
Copy link
Author

Hold the thought for now, @hellofromtonya and I have had some very preliminary discussions about whether to re-propose a minimum version bump.
That would be a good thing to do for a 6.0 major release :-)

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.

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 ;-)
If PHP7 will be the new min version, the current version of the patch is the way to go.

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.

Exactly ;-) Tricky timing there...

What about this: I will revert back to default the constant to false allowing anyone with PHP7 to add an array to their wp-config. When the WP min PHP version gets bumped, I will change this code accordingly to the correct type. This would allow a short term merge and avoid relying on something that might possibly not happen at all. Although I would fully support the PHP min version bump personally.

@hellofromtonya
Copy link
Contributor

hellofromtonya commented Feb 22, 2022

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.
Frequency: edge case

Limitations:

  • Sites running < PHP 7.0 cannot use this feature. Why? PHP 7 adds the ability to define an array in a constant.
    • This means developers who need to support < PHP 7.0 will not be able to use this feature.
  • A constant is not testable with automated tests. This code change must be manually tested. See reasons below.

Risks:

  • The configuration mechanism must be implemented in each 3rd party object cache implementation. This means a dev may configure it in wp-config.php but it might not be used by their or their web host's 3rd party object cache.
  • Risk of not working, side effect, or regression. Why? No automated tests to validate the code functions as expected with both default value and a group array dataset. (see the reasons below)

A constant is not testable

A 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?

  1. This is a low level area that runs very early in the execution cycle, meaning less risk of improper manipulation by a plugin or theme.
  2. No limitations for devs to use it. Why? An array in a global variable works on all of WordPress' current supported PHP versions including PHP 5.6. And for those projects that need to support earlier PHP versions, it works there too.
  3. A global variable is testable. This means automated tests can be built and run to ensure (a) it works with the default value and a group dataset, (b) there are no future regressions, and (c) no impact on other tests.

Recommendation and discussion

I'm leaning towards recommending a global variable to be added to the wp-config.php file instead of a PHP constant (for the reasons above).

@peterwilsoncc @JanThiel what do you think?

@tillkruss
Copy link
Member

An optional global variable in the format of the existing $wp_object_cache_xyz makes sense.

@JanThiel
Copy link
Author

Thank you very much @hellofromtonya.
Your arguments are very compelling and I do not see any reason against the global variable as this mechanism is used by many plugins for configs as well.

I will change the PR accordingly.

@hellofromtonya
Copy link
Contributor

@JanThiel can you rebase your PR against trunk too please? Once done and the global implemented to replace the constant, then the failing tests can be debugged.

@hellofromtonya
Copy link
Contributor

Whoops, didn't mean to close it. Re-opening 🤦‍♀️

@hellofromtonya hellofromtonya reopened this Mar 1, 2022
@JanThiel
Copy link
Author

JanThiel commented Mar 1, 2022

@hellofromtonya Trunk is merged and the open codestyle fix is also applied
The change to the global as discussed is still missing.

@tillkruss
Copy link
Member

@JanThiel: I'm just looking at #2368, should we maybe rename the constant from WP_OBJECT_CACHE_IGNORE_GLOBAL_GROUPS to WP_CACHE_IGNORE_GLOBAL_GROUPS?

@tillkruss
Copy link
Member

@hellofromtonya Is there anything we can do to push this forward?

@spacedmonkey Any feedback?

@peterwilsoncc
Copy link
Contributor

peterwilsoncc commented Jun 8, 2022

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 file

/* 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, ETC

WordPress currently has the enable_loading_advanced_cache_dropin filter that runs before plugins, mu-plugins, themes or anything a common configuration can use. However, it is possible to make use of it by including the plugin API in your wp-config file and adding a filter. (Aside: please do not do this unless you know what you are doing.)

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 group_1, group_2 and WP then adds group_3, it is impossible to know the developers intent if the group is absent.

Using a filter will allow a developer who knows what they are doing to add or remove groups without risk.

Thoughts?

@tillkruss
Copy link
Member

@peterwilsoncc: I'm with you on the logic, but the enable_loading_advanced_cache_dropin filter is only checked once. Using a filter here allow changes to the "global groups" to change during each request, which will cause havoc, a constant wouldn't 😕

@peterwilsoncc
Copy link
Contributor

@tillkruss Isn't the idea to allow users to define the default values in these lines, they only run once during wp_start_object_cache() too?

if ( function_exists( 'wp_cache_add_global_groups' ) ) {
wp_cache_add_global_groups( array( 'users', 'userlogins', 'usermeta', 'user_meta', 'useremail', 'userslugs', 'site-transient', 'site-options', 'blog-lookup', 'blog-details', 'site-details', 'rss', 'global-posts', 'blog-id-cache', 'networks', 'sites', 'blog_meta' ) );
wp_cache_add_non_persistent_groups( array( 'counts', 'plugins' ) );
}

Placing a filter within wp_cache_add_global_groups() or wp_cache_add_non_persistent_groups() could potentially be problematic (or at least need some really considered architecture) so, you are correct, it's probably best avoided.

@tillkruss
Copy link
Member

tillkruss commented Jun 9, 2022

That would be great! Thanks for clarifying 🤙

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.

4 participants