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

WP_CACHE constant #136

Closed nlemoine closed 4 years ago

nlemoine commented 4 years ago

Hi again,

I think the wp-config.php file should only be edited by hand. It should not be in the plugin's scope.

I handle the WP_CACHE constant in another file than wp-config.php.

However, If WP_CACHE is defined (which isn't on a standard WordPress install). The user has probably already edited his wp-config.php file. There is consequently no reason to write it because you can't how a wp-config.php is like. It may be split into multiple files, etc.

 // set WP_CACHE if not already set
if ( defined( 'WP_CACHE' ) && ! WP_CACHE ) {
    self::_set_wp_cache();
}

Should be (if you want to keep writing the wp-config.php file).

 // set WP_CACHE if not already set
if ( !defined( 'WP_CACHE' ) ) {
    self::_set_wp_cache();
}

Sorry for being such annoying with my issues. I love your plugin but recently, each update breaks something in my stack.

coreykn commented 4 years ago

Your feedback is always welcome and appreciated. I wish it wasn't, but the WP_CACHE constant is actually defined as false by WordPress in wp-includes/default-constants.php, which means checking if it's defined is unnecessary and would result in Cache_Enabler::_set_wp_cache() never being called (#102). That means the constant would never be set as true by Cache Enabler.

There is a reason to set this because when set as false caching is disabled. As you're likely aware, we develop for the majority of users and we always have to keep in mind the technical ability they have. For most editing a wp-config.php can be intimidating and that is even if they know how to find that file. I do plan on introducing a simple link in our WP_CACHE constant warning to offer the ability to overwrite the constant if found in wp-config.php, which could be expanded to fix this, but I still don't think it's a good idea to not automatically set this constant. I believe the less that the typical user has to do the better.

coreykn commented 4 years ago

I've had a little bit more time to investigate this further. I'm unsure how the few changes made to setting the WP_CACHE constant would break something in your stack. The logic behind calling Cache_Enabler::_set_wp_cache() on activation hasn't changed as the old and new if statements rely on WP_CACHE returning false. As far as I'm aware that means whatever may be occurring now would have also occurred before the change. Please correct me if I'm mistaken.

I love your plugin but recently, each update breaks something in my stack.

I understand that there has been many changes lately, all of which have been with the intention of significantly improving Cache Enabler. I'm sure new issues could arise that I didn't consider or was aware of previously as the flexibility of WordPress does create a lot of scenarios to consider. We'll find those issues and fix them right away. After all, that is how great products are built. It would be good to be more specific though, because there has been nine updates in the last four weeks so I'm unsure of the exact updates you're referring to. I'll assume the changes you consider breaking are the issues that you've opened on GitHub.

I will be reviewing, adding, and updating our current hooks here shortly. I'll look into adding a filter hook to allow the ability to disable Cache Enabler from handling the WP_CACHE constant.

nlemoine commented 4 years ago

Thanks a lot for your constructive feedback, it's very much appreciated!

I'm unsure how the few changes made to setting the WP_CACHE constant would break something in your stack

It breaks on activation only (constant already defined error). On some other sites I'm working on, I didn't have the issue because the plugin was already activated and the activation hook wasn't triggered.

Maybe a simple defined check would be enough. Instead of writing:

define(WP_CACHE, true);

You could write:

if(!defined('WP_CACHE')) {
    define(WP_CACHE, true);
}

I'll look into adding a filter hook to allow the ability to disable Cache Enabler from handling the WP_CACHE constant.

I think a warning with a "Set the constant in my wp-config.php" button would be better (or both). I my case, the plugin was updated without triggering the activation hook. It would give more flexibility, allowing standard users to easily set the WP_CACHE constant and developers to not care about it.

It would be good to be more specific though

I'll try to add more detailed issues next time! 😉

coreykn commented 4 years ago

Thanks for the detailed feedback, @nlemoine, it's all very helpful. You're right, it might be enough to just check if it has been defined (like WordPress does) before we define it as true. That's a fantastic suggestion as that would not require a filter hook.

Let me think about this and tinker with it a little bit more. Updates to follow.