Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Commented out WP_CACHE definition in wp-config.php being incorrectly ignored #591

Closed
davidfavor opened this issue Oct 21, 2015 · 19 comments
Closed
Labels
bug
Milestone

Comments

@davidfavor
Copy link

@davidfavor davidfavor commented Oct 21, 2015

:::: Summary

This relates specifically when moving from WP Super Cache to ZenCache. Likely also effects other caching plugins besides WP Super Cache.

A slight code modification should fix this problem. See details below.

The bottom line is this bug prohibits ZenCache from ever really being enabled, as WP_CACHE remains commented out.

This means ZenCache appears to be slow, as it's code never really executes.

:::: Detailed Explanation

ZenCache appears to sometimes missing updating wp-config.php with the WP_CACHE directive.

Here's how to reproduce this on WP-4.3.1 with all updates installed.

  1. Starting point is clean wp-config.php - no caching plugins installed + no wp-config.php directives. Site speed is 45 reqs/sec

  2. Install + Activate WP Super Cache. Site speed increases to 2500 reqs/sec with wp-config.php of...
    define('WP_CACHE', true); //Added by WP-Cache Manager
    define( 'WPCACHEHOME', '/sites/drf/fastseohosting.com/wordpress/wp-content/plugins/wp-super-cache/' ); //Added by WP-Cache Manager

  3. Deactivate WP Super Cache + wp-config.php changes to...
    //define('WP_CACHE', true); //Added by WP-Cache Manager
    define( 'WPCACHEHOME', '/sites/drf/fastseohosting.com/wordpress/wp-content/plugins/wp-super-cache/' ); //Added by WP-Cache Manager

  4. Install + Activate ZenCache + wp-config.php no changes (as expected)

  5. In ZenCache settings enable + save config + wp-config.php does not change.
    //define('WP_CACHE', true); //Added by WP-Cache Manager

WP_CACHE still appears to be commented out.

:::: Suggested Fix

In zencache.inc.php it appears tests for WP_CACHE require being anchored to left, rather than unanchored (floating).

Test likely should be - ^define... - rather than - define... - which will match the commented out define cruft left over from WP Super Cache.

@davidfavor davidfavor changed the title Slight problem moving from other caching plugin to ZenCache Slight problem moving from other caching plugins to ZenCache which prohibits ZenCache code from ever executing Oct 21, 2015
@raamdev
Copy link
Contributor

@raamdev raamdev commented Oct 21, 2015

@davidfavor Thanks for the report. This should have been addressed in v150821 (ZenCache Pro) and v150930 (ZenCache Lite). See #509

I'm going to run a few tests to see if I can reproduce this issue.

Can you tell me which version of ZenCache you are running?

@raamdev
Copy link
Contributor

@raamdev raamdev commented Oct 22, 2015

I have confirmed this bug following @davidfavor's steps above with both ZenCache Lite v150930 and ZenCache Pro (dev branch).

If the wp-config.php file contains the following line, Enabling ZenCache (ZenCache → Plugin Options → Enable/Disable) does not uncomment or otherwise set WP_CACHE to true in wp-config.php, so despite ZenCache being enabled, nothing on the site is actually being cached.

//define('WP_CACHE', true); //Added by WP-Cache Manager

@jaswsinc It looks like this regex is not taking into consideration that define('WP_CACHE', true); may actually be commented out in wp-config.php.

@raamdev raamdev removed the needs testing label Oct 22, 2015
@raamdev raamdev added this to the Next Release (Pro) milestone Oct 22, 2015
@raamdev raamdev changed the title Slight problem moving from other caching plugins to ZenCache which prohibits ZenCache code from ever executing Commented out WP_CACHE definition in wp-config.php being incorrectly ignored Oct 22, 2015
@raamdev
Copy link
Contributor

@raamdev raamdev commented Oct 22, 2015

The above comment only referenced the ZenCache Pro codebase. Noting here that this should also be fixed in ZenCache Lite (via this line).

@jaswrks
Copy link

@jaswrks jaswrks commented Oct 22, 2015

@jaswsinc It looks like this regex is not taking into consideration that define('WP_CACHE', true); may actually be commented out in wp-config.php.

Good catch guys! I agree, there is room for improvement here.

@raamdev You could try playing with a negative lookbehind in that line you referenced.

i.e., (?<!\/\/\s*)

if (preg_match('/(?<!\/\/\s*)define\s*\(\s*([\'"])WP_CACHE ...

Looks like there is at least one instance of this in the removal routine also that needs to be updated.

@raamdev raamdev self-assigned this Oct 22, 2015
@raamdev
Copy link
Contributor

@raamdev raamdev commented Oct 23, 2015

@jaswsinc Thanks! I started playing with negative lookbehinds but preg_match() (and regex101.com) complains that it must be zero-width:

Warning: preg_match(): Compilation failed: lookbehind assertion is not fixed length at offset

https://regex101.com/r/aA0yT1/2

Error: Lookbehinds need to be zero-width, thus quantifiers are not allowed

Which throws a wrench into the mix. It seems like we can check for one particular scenario (//define...) but that won't account for variations such as // define. Ideas?

@jaswrks
Copy link

@jaswrks jaswrks commented Oct 23, 2015

Nice! I see what you mean. Well, I think it's fine to pull the \s* out of the lookbehind.

(?<!\/\/)\s*

See: https://regex101.com/r/tD0hK0/2

@jaswrks
Copy link

@jaswrks jaswrks commented Oct 23, 2015

Ah. Nevermind. Tricky! That doesn't work either.

@raamdev
Copy link
Contributor

@raamdev raamdev commented Oct 23, 2015

Yep, I tried that too!

@raamdev
Copy link
Contributor

@raamdev raamdev commented Oct 23, 2015

Why don't we just remove define('WP_CACHE', true); if we find it in wp-config.php and then re-add it? I realize there's a slight performance hit there by doing work unnecessarily in the event that it's not commented out, but this routine is only running once when ZenCache is activated... it's not like it's running on every page load or something.

@raamdev
Copy link
Contributor

@raamdev raamdev commented Oct 23, 2015

In other words, just remove this entire conditional and just let the next line (removeWpCacheFromWpConfig()) remove it.

@jaswrks
Copy link

@jaswrks jaswrks commented Oct 23, 2015

That's a decent idea. However, the reason it's like that right now, is to cover a scenario where the wp-config.php is not writable (I do that myself). So for instance, if you have this already defined in your wp-config.php file, and ZenCache finds it in there, there is no reason for us to do anything more; i.e., no reason for failure or any notice about us not being able to write to the file.

Here is a modified version of the regex that seems to work. If you can take a closer look I'd appreciate it. See: https://regex101.com/r/fD4wK5/2

(?:\n|\<\?(?:php)?|;)\s* ...

Instead of doing a lookbehind, we can be more specific about what we want to find. The define( fragment should be found right after <? or <?php, or on a new line by itself, or right after a previous statement that ends with ; ... and then followed by any amount of whitespace; i.e., \s*

@jaswrks
Copy link

@jaswrks jaswrks commented Oct 23, 2015

Or, this might be even better. We could strip all comments from the syntax before we check. See: http://php.net/manual/en/function.php-strip-whitespace.php

@raamdev
Copy link
Contributor

@raamdev raamdev commented Oct 23, 2015

it's like that right now, is to cover a scenario where the wp-config.php is not writable

Ah, that makes sense. Yeah, that's an important scenario to consider.

a modified version of the regex

Yep, that seems to work. I ran it through a few tests (including directly in ZenCache) and it's working as expected.

We could strip all comments from the syntax before we check.

Hmm, that would work too. Any preference?

@jaswrks
Copy link

@jaswrks jaswrks commented Oct 23, 2015

My preference would be to strip the comments before we do a check. That seems bulletproof in terms of us being able to accurately detect whether it's really in there or not.

You could then leave the regex as-is. However, while you're in there it's probably a good idea to change:

this:

if (preg_match('/define

to:

if (preg_match('/\bdefine

That avoids a false match on something wild like: my_define('WP_CACHE' ... by requiring that define be preceded by a word boundary.

@raamdev
Copy link
Contributor

@raamdev raamdev commented Oct 23, 2015

Next Pro Release Changelog:

  • Bug Fix: Fixed a bug where a commented-out WP_CACHE definition in wp-config.php (such as what WP Super Cache leaves behind) was being incorrectly ignored and resulted in caching being silently disabled. Props @davidfavor. See Issue #591.
@raamdev
Copy link
Contributor

@raamdev raamdev commented Oct 23, 2015

Next Lite Release Changelog:

  • Bug Fix: Fixed a bug where a commented-out WP_CACHE definition in wp-config.php (such as what WP Super Cache leaves behind) was being incorrectly ignored and resulted in caching being silently disabled. Props @davidfavor. See Issue #591.
@davidfavor
Copy link
Author

@davidfavor davidfavor commented Oct 23, 2015

Thanks for fixing this so fast!

BTW, the only reason I figured this bug out instantly is I've done the same thing in my code several times.

@raamdev
Copy link
Contributor

@raamdev raamdev commented Oct 24, 2015

the only reason I figured this bug out instantly is I've done the same thing in my code several times.

@davidfavor Ha! Always better to have more eyes on the bugs. 🐛 🐞 Thanks again. 😄

@raamdev
Copy link
Contributor

@raamdev raamdev commented Nov 14, 2015

ZenCache v151114 has been released and includes changes from this GitHub Issue. See the v151114 announcement for further details.


This issue will now be locked to further updates. If you have something to add related to this GitHub Issue, please open a new GitHub Issue and reference this one (#591).

@wpsharks wpsharks locked and limited conversation to collaborators Nov 14, 2015
@raamdev raamdev removed their assignment Apr 28, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants