keycdn / cache-enabler

A lightweight caching plugin for WordPress that makes your website faster by generating static HTML files.
https://wordpress.org/plugins/cache-enabler/
123 stars 46 forks source link

Don't hard-code "/cache-enabler" in advanced-cache.php #195

Closed stevegrunwell closed 3 years ago

stevegrunwell commented 3 years ago

When the advanced-cache.php file is loaded, it was previously looking for WP_PLUGIN_DIR . '/cache-enabler', which would prevent page caching from working if the plugin was not installed here for whatever reason.

This PR enables the path to be adjusted by defining a CACHE_ENABLER_PLUGIN_DIR constant in a site's wp-config.php file.

coreykn commented 3 years ago

To get an idea of what this resolves, do you have an example when Cache Enabler wouldn't be installed there? I ask because I know the CE_DIR isn't defined yet when advanced-cache.php is included, but it could be a little confusing as they would have the same value (if the user defined constant has the correct path). Or is more related to the directory name and not the directory path?

We're going to be changing the constants in cache-enabler.php to CACHE_ENABLER_* here shortly, so maybe if this is actually required we could first check if the directory related constant is defined beforehand in case anyone needs to customize it. That would prevent having two constants that have the same value. My only concern in that event would be the user actually knowing the correct remote path.

stevegrunwell commented 3 years ago

The main reason would be if Cache Enabler were to be bundled within, for example, a platform-wide MU plugin by a host; under these circumstances, advanced-cache.php would need to load files from mu-plugins/some-amazing-host/plugins/cache-enabler/, not plugins/cache-enabler/.

With regards to the duplicates, it was absolutely done knowing that the CE_DIR constant wouldn't be available this early in the bootstrapping process, but anything in the wp-config.php file would. As to your point about knowing the remote path, I'd view this as a "you can set this if you really know what you're doing, but if you're planning on running it out of plugins/cache-enabler, let the plugin take care of it for you.

coreykn commented 3 years ago

Ah, great point. 🙂 When checking I see the activation hooks that are registered won't work when used as an MU plugin. I'll need to improve some handling to allow Cache Enabler to function better in that type of environment.

Anyway, I think this is a great idea and am glad you've opened this pull request. I'd say let's change the proposed constant to CACHE_ENABLER_DIR instead of CACHE_ENABLER_PLUGIN_DIR. When deprecating the old constants and replacing them with the new naming structure I'll add a conditional to check if it's been defined before trying to define it. What do you think?

stevegrunwell commented 3 years ago

Sounds great, I've updated the PR with the new convention.