Allow user to set their own cache_enabler_constants_file to bypass hardcoded path usage

[nlemoine]

The changes introduced in https://github.com/keycdn/cache-enabler/pull/260 now set a $cache_enabler_constants_file variable with a hardcoded path.

I'm using Deployer, which uses a Capistrano like structure to deploy a project. I currently end up with something like: /var/www/releases/5/public/wp-content/plugins/cache-enabler/constants.php

But this path changes each time I deploy a new release. I tried to set it manually but the file contents is overwritten upon deactivation/activation.

Solution : introduce a new constant which, when defined, will be used instead. It will fallback on the hardcoded path if not defined.

[coreykn]

The example path is replaced by the dynamic path stored in the CACHE_ENABLER_DIR constant when the advanced-cache.php file is created in the wp-content directory. The CACHE_ENABLER_DIR constant is defined when the constants.php file is loaded. It is the path to the directory that the constants.php file is located in.

To understand your issue better, does the path stored in the $cache_enabler_constants_file variable located in the wp-content/advanced-cache.php file need to be updated when you deploy a new release? Or does it always need to be static and is using a value that is incorrect?

If it needs to be updated when you deploy a new release you can have a couple options, like using the method to create a new advanced-cache.php file or even just deleting it (which currently will allow a new one to be created when an admin page is visited). Alternatively, deactivating and then activating the plugin will trigger a new advanced-cache.php file to be created.

If it need to always be static then does defining the CACHE_ENABLER_DIR constant work?

[nlemoine]

Thanks for your reply!

To understand your issue better, does the path stored in the $cache_enabler_constants_file variable located in the wp-content/advanced-cache.php file need to be updated when you deploy a new release? Or does it always need to be static and is using a value that is incorrect?

The point is I shouldn't have to update it. I understand you're doing this because some of the WP constants (WP_PLUGIN_DIR, etc.) are not available yet when advanced-cache.php is loaded. But setting an absolute hardcoded path should be avoided whenever possible. Take another example, what if I move my website from one host to another? Or if I want to run it locally? I will then run into hardcoded path issues.

The purpose of this PR is to have a real constant, not a hardcoded path string. Plus, It's totally safe for all users using the hardcoded path because it will fallback on the hardcoded string if the constant is not defined.

[coreykn]

Yeah, I agree. That was what I came up with at the time and was an exception due to the perceived limitations. When looking at it again now I do see how it can be improved to resolve the issues you're describing. I do not think a new constant needs to be introduced though.

How about the following instead:

1. Update the cache-enabler/advanced-cache.php file:

   $cache_enabler_constants_file = ABSPATH . 'wp-content/plugins/cache-enabler/constants.php';

   Maybe even add a clean up function if the constants file does not exist (like if the plugin was manually deleted but the wp-content/advanced-cache.php drop-in file was still included by the WP_CACHE constant), such as:

    else {
       @unlink( __FILE__ );
   }

2. Update the Cache_Enabler_Disk::create_advanced_cache_file() method:

   $advanced_cache_file_contents = str_replace( 'wp-content/plugins/cache-enabler', str_replace( ABSPATH, '', CACHE_ENABLER_DIR ), $advanced_cache_file_contents );

[nlemoine]

Unfortunately, it's not that simple.

In a bedrock case: https://www.tehplayground.com/aX6K5heeV71CdBq2

There's many potential issues here:

• plugins, content, etc. are not always within ABSPATH
• Almost every directory can have a custom location (and a custom name) if those constants are set in a custom way.

If you really want to use the ABSPATH constant in the advanced-cache.php file (which is the only path available at this time of the load cycle), you will have to resolve the cache-enabler directory relatively to the ABSPATH, when the file is created.

[nlemoine]

Note that this applies as well for the CACHE_ENABLER_INDEX_FILE but I was able to fix ii by setting a constant in my config file. That's why I chose the same way for this issue.

[coreykn]

Interesting, that is very helpful to know. Thank you. Just so you're aware, I like to first exhaust all of the options that don't require user intervention as the goal is no configuration required. That is all I'm doing here. 🙂

I understand you're doing this because some of the WP constants...

The WordPress constants are available, like for the WP content, plugins, or MU directories (included in wp-includes/default-constants.php before wp-content/advanced-cache.php is). For example, versions 1.5.0-1.7.2 used some of those. This is no longer used due to making the plugin more MU friendly in 1.8 (but that was more about making the plugin not care where it is installed as long as it's being loaded into the site).

With how we are now setup in 1.8+ along with what is to come, I think deleting the wp-content/advanced-cache.php file if the constants file doesn't exist is the best option. (That also helps get rid of that file if it's considered abandoned.) Then a new wp-content/advanced-cache.php file will be created automatically with the updated value as soon as an admin page is hit by a user that has the manage_options capability. Then once the new requirements class is added shortly it would be deleted and then created later in the load cycle with nearly any admin or frontend request.

If that somehow doesn't cover all use cases, or if a constant is still wanted, we can add it in the way you've requested. Only downside I can see to the PR as it currently is would be a rare potential error in the event that constant becomes no longer defined after the creation of the wp-content/advanced-cache.php, so if that was introduced we would maybe want to check its existence in the file itself as well.

[nlemoine]

I like to first exhaust all of the options that don't require user intervention as the goal is no configuration required

I like them too ;) But in that case, the easiest (and consistent considering the development practice in this plugin) way was to provide a constant.

The WordPress constants are available, like for the WP content, plugins, or MU directories (included in wp-includes/default-constants.php before wp-content/advanced-cache.php is)

They're not. The file containing the methods that define constants is indeed included before. But these methods are called far after the advanced-cache.php inclusion.

This would have simplify everything otherwise, including taking care of the "mu" loading.

I think deleting the wp-content/advanced-cache.php file if the constants file doesn't exist is the best option...

I strongly disagree. Relying on an admin with the right permissions visiting an admin page? This would be a really weird behavior. I'm lighting candles so you won't go down that way 😅

This would even apply for a front-end request, for the same reasons explained here (e.g. cache must be, as far as possible, always hot).

Given that usage of this constant is probably an edge case (non typical WP install like Bedrock, etc.), constant option is not that bad. Users who set it will probably know what they're doing. Other 90% will leave the absolute hardcoded path as it is.

[coreykn]

They're not.

You're right, I was partially wrong. Only WP_CONTENT_DIR is available. Sorry for my oversight there.

I strongly disagree. Relying on an admin with the right permissions visiting an admin page? This would be a really weird behavior.

Agreed. Looks like you missed the sentence that followed?

Then once the new requirements class is added shortly it would be deleted and then created later in the load cycle with nearly any admin or frontend request.

Past PRs have detailed the need to improve the requirements handling. Please understand that just because something is added now does not mean it's the final form. We build in iterations and sometimes certain actions are not possible until something else has been introduced. I understand there are weak points that need to be improved, but only so much can be done at a time. For this edge case waiting for the new requirements class to allow that to occur anywhere, instead of only on pages considered to be an admin interface page, is not that big of deal, right?

Anyway, I'll get something added to account for what you've brought up with all of the feedback you've provided. Thanks!

[nlemoine]

Looks like you missed the sentence that followed?

I did miss it but I don't fully get what you mean by "new requirements class".

For this edge case waiting for the new requirements class to allow that to occur anywhere, instead of only on pages considered to be an admin interface page, is not that big of deal, right?

As you probably understood, I'm really not a fan of that kind of pattern 😉 WordPress provides events like activation and deactivation for that purpose. As i previously said, using a constant that you don't define leaves the responsibility of a bug to the user who has set it which seems like a fair way to deal with this to me.