Wp-rocket: Don't add WP_CACHE in wp-config.php if the constant is already defined somewhere else

[vmanthos]

Currently, we don't have a way to skip adding the WP_CACHE in the wp-config.php file.

This could be useful if that file is under version control.

Related ticket: https://secure.helpscout.net/conversation/1124360039/154873?folderId=2135277
Slack discussion: https://wp-media.slack.com/archives/C43T1AYMQ/p1585745468055100

[GeekPress]

This is something we won't do.

Why?
This constant is mandatory to have to have WP Rocket working.

If someone needs to « protect » their wp-config.php file to not be edited, the solution is to set the properly writing permission.

This is what we are doing on our own website where the constant is defined on another file.

[ethanclevenger91]

I haven't seen the linked discussion, but I feel like this warrants more of it. If someone has already defined WP_CACHE, foisting that seems invasive. This really has less to do with wp-config.php and more to do with respecting existing constants - doesn't matter what files a plugin is trying to edit.

In set_rocket_wp_cache_define, it sure seems like this could be more respectful:

if ( ! rocket_valid_key() || ( $turn_it_on && defined( 'WP_CACHE' ) && WP_CACHE ) ) {
    return;
}

Rather...

if ( ! rocket_valid_key() || defined( 'WP_CACHE' ) ) {
    return;
}

[ethanclevenger91]

@vmanthos The code implies that if you set the IS_PRESSABLE constant to true, you can get around this for now, but I don't know what other implications that has off the top of my head.

[alfonso100]

Related ticket: https://secure.helpscout.net/conversation/1137962533/158322?folderId=2683093

[arunbasillal]

As noted in the ticket brought up by Alfonso and Ethan, the constant might be defined elsewhere. We already have a custom check for pressable in here - https://github.com/wp-media/wp-rocket/blob/00cf7a1c5e0b2e7354b0aa76ca86603ee4420eae/inc/functions/files.php#L360

Instead of doing this host by host, we could add a boolean filter and then switch it on a compatibility file for Pressable. This way, other configs that defines WP_CACHE elsewhere can use WP Rocket without issues. All they have to do is set the boolean filter to off somewhere else (helper plugin or functions.php)

@GeekPress @Tabrisrp What do you think of this? We aren't losing anything. While at the same time, we will be standardizing our compatibility (by moving PRESSABLE to it's dedicated file).

[GeekPress]

Correct me if I'm wrong, but if WP_CACHE is set to false, then advanced-cache.php file isn't included which will prevent all our optimizations. Or it's not the case anymore, it is?

If not, I will let @Tabrisrp and @hellofromtonya decided what will be the best approach

[arunbasillal]

@GeekPress For the cases that Alfonso and Ethan brought up, the WP_CACHE is still defined, just not in wp-config.php. Defining it again in wp-config.php is causing errors.

Correct me if I'm wrong, but if WP_CACHE is set to false, then advanced-cache.php file isn't included which will prevent all our optimizations

You are right - https://github.com/WordPress/WordPress/blob/463232238228d3eb111b0d4ceccc80e1e4e33c7a/wp-settings.php#L93

But in the case mentioned above, WP_CACHE is still defined.

[FlyingDR]

As an author of original request (made through WP Rocket support request form) I can comment on this request too:

A way proposed by @GeekPress with use of write protection permissions does not cover all cases: for example Windows machines does not respect Unix file permissions that are possible to store in Git and Git is unable to store NTFS permissions. It will result into requirement for all developers on Windows machines to either protect this file by themselves or control commit contents. Apart from this it sounds pretty strange: "We will forcibly change your files, but you can try to use some external hacks to try to avoid it".

I understand that currently implemented approach is targeted users who uses WordPress and its "official" way, but there are others. As a minimum WP Rocket is currently incompatible with e.g. Bedrock and probably other projects that tries to utilize modern approaches into WordPress development.

Since in "official" WordPress way there is obviously no chance for WP Rocket to override WP_CACHE variable besides patching wp-config.php my original request was to provide a hook to disable this feature. From my point of view it is safe because people who manages WordPress site through admin will not use this hook anyhow, but people who wants more control will get an ability to control this feature correctly without need to hack plugin itself or to change its behavior by programmatically remove plugin's action that may result into calling unwanted function.

[GeekPress]

Ok thanks @arunbasillal @FlyingDR for the clarification, I better understand the need. I thought the need was to delete the constant. Now it's clear, we don't want to re-add into wp-config.php if the constant has been defined already somewhere else.

I re-open this issue to be able to work on it ;)

[GeekPress]

I updated the issue title to be in relation with our discussion and our current needs.

[hellofromtonya]

If I understand correctly, this ticket seeks to provide a way to opt-out of Rocket modifying the wp-config.php file by adding define('WP_CACHE', true); // Added by WP Rocket to it. Let's talk about it.

Correct me if I'm wrong, but if WP_CACHE is set to false, then advanced-cache.php file isn't included which will prevent all our optimizations.

@GeekPress You are correct. By default, WordPress will set WP_CACHE to false if it's not already defined and will not load advanced-cache.php file. All of this is triggered when WordPress first boots up, i.e. loaded from the wp-config.php file.

For Rocket to provide all the benefits, we need WP_CACHE set to true _before_ WordPress runs its wp_initial_constants() function.

Filtering to disable Rocket from defining WP_CACHE can present a risk of it being turned off. But it also provides a filterable way for 3rd parties to use their own way of turning that constant on and it can eliminate a potential conflict.

We'll need to think about how we will handle:

• The potential risk of a 3rd party disabling WP_CACHE, i.e. setting it to false
• Removing the define in the wp-config.php if we've already modified the file and then a 3rd party callback opts out of Rocket adding it

[GeekPress]

I guess there are two discussions and problems on this issue:

• a filter to avoid WP Rocket to add WP_CACHE into wp-config.php
• Don't try to add WP_CACHE into wp-config.php if the constant has been declared somewhere else

[hellofromtonya]

And also:

• Remove WP_CACHE from wp-config.php if we've already added it and now a 3rd party is filtering Rocket not to add it.
• What if it is already defined or the 3rd party filters Rocket not to add it, but WP_CACHE is set to false?

[GeekPress]

@hellofromtonya can I let you talk / brainstrom about this with the developer team?

[Tabrisrp]

Additional context about how it currently works:

• The function rocket_maybe_set_wp_cache_define() is hooked on admin_init, so it's running on every admin page load
• This function checks for the WP_CACHE constant, and if it's set to true
• If not, it calls set_rocket_wp_cache_define() to set the constant to true in wp-config.php
• set_rocket_wp_cache_define() is also called in the activation/deactivation callbacks, to set the constant to true/false
• And it's called after saving the options, performing the same check as rocket_maybe_set_wp_cache_define()

[Tabrisrp]

Scope a solution ✅

• Move old code to new architecture: everything related to wp-config.php/WP_CACHE into WP_Rocket\Engine\Cache\WPCache
• Use WP_Rocket\Engine\Cache\AdminSubscriber to hook any required callbacks
• In the method writing to wp-config.php, check if the constant is already defined (but not in this file) and if yes, bail-out. If it’s set in this file and the value is false, we can update it as usual.
• Add a HealthCheck verification for the value of the constant. If false, show an error

Effort ✅
Effort [S] to [M] because of the architecture change and the need to write all tests from scratch

[Tabrisrp]

Blocked by PR #2740

[Tabrisrp]

Unblocking, PR #2740 is now merged

[ethanclevenger91]

Hey @Tabrisrp (dunno if this is your project or not, but wanted to ping someone to keep the closed issue from going unnoticed).

Looks like this update still doesn't respect me setting WP_CACHE to false somewhere else. For example, in a Bedrock-driven WordPress setup, I'd probably put that into a configuration file loaded before wp-config.php. If, in that file, I set WP_CACHE to false, WP Rocket will still try to set it to true in wp-config.php.

@GeekPress specifically mentioned that this disables advanced-cache.php, and that's _the whole point_. Working locally, for example, I may not want caching enabled, then on staging and prod I may leave it enabled, all handled by environment variables (once again, see Bedrock).

Seems to me that true/false shouldn't matter - if WP_CACHE is defined already, someone did that on purpose, so don't write anything to wp-config.php. If the issue is WP-Rocket changing that constant when it's activated/deactivated, then the line it adds needs annotated in some way so you can discern if WP-Rocket automated that decision or someone else made that decision.

I do appreciate the new rocket_set_wp_cache_constant filter however, which I'll be utilizing in the meantime.

[Tabrisrp]

The issue we ran into when working on that is that WordPress itself sets the WP_CACHE constant to false during load time if it hasn't been already defined earlier (like in wp-config.php).

So in WP Rocket, we can't check for the constant to be defined or not, it's always defined. And we can't know if it's false because of an user choice, or because of WP default.

That's why we defaulted to the rocket_set_wp_cache_constant filter instead, as that gives the user control over what WP Rocket does during its own initialization.