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

Version 1.8 breaks Bedrock setup #286

Closed ouun closed 3 years ago

ouun commented 3 years ago

Hi there, many useful updates in the new version, thanks for the ongoing updates! However I have a issue with the latest version as it removes content from wp-config.php and therefore WP breaks completely.

One main issue with touching wp-config.phpautomatically is that in some setups the file just holds references and is not really used for defining stuff. E.g. as in this file ofg Roots Bedrock: https://github.com/roots/bedrock/blob/master/web/wp-config.php

So here I am now ending in Fatal error: Uncaught Error: Call to undefined function nocache_headers() in /wp-admin/admin.php:36 error.

Kind regards

coreykn commented 3 years ago

Glad to hear they've been useful! Cache Enabler only sets the WP_CACHE constant in the wp-config.php if the /** Sets up WordPress vars and included files. */ line is found:

https://github.com/keycdn/cache-enabler/blob/e94c1afe4231485b0faa65def2340fc0fa552c92/inc/cache_enabler_disk.class.php#L1373

This is the current way of deciding if the config file is considered "default" and hasn't been customized, like when using Bedrock. Are you experiencing a different behavior? If yes, can you please let me know what Cache Enabler has removed from the wp-config.php file?

ouun commented 3 years ago

@coreykn thank you for getting back to me. I just tested it with another Bedrock environment and again Cache Enabler removes all content from wp-config.php. I got the following errors:

Warning: file_put_contents(/.../web/app/settings/cache-enabler/subdomain.example.test.php): failed to open stream: No such file or directory in /.../web/app/plugins/cache-enabler/inc/cache_enabler_disk.class.php on line 422 Fatal error: Allowed memory size of 2147483648 bytes exhausted (tried to allocate 221184 bytes) in /.../web/wp/wp-includes/functions.php on line 6679

The original content ofg wp-config.phpwas:

<?php
/**
 * Do not edit this file. Edit the config files found in the config/ dir instead.
 * This file is required in the root directory so WordPress can find it.
 * WP is hardcoded to look in its own directory or one directory up for wp-config.php.
 */
require_once dirname(__DIR__) . '/vendor/autoload.php';
require_once dirname(__DIR__) . '/config/application.php';
require_once ABSPATH . 'wp-settings.php';

This is all removed when activating the plugin. Then when fixing the file and as long as the plugin is loaded, WP breaks completely.

ouun commented 3 years ago

@coreykn solved the issue: I am using a filter 'pre_option_cache_enabler' to set the default options in a Multisite environment. Just removed that for now and the installation passed. Worked with both networks I had issues with.

ouun commented 3 years ago

As soon as I add the following snippet back, the site breaks again:

add_filter('pre_option_cache_enabler', function ($settings) {
    $default_settings = array(
        'cache_expires'                      => 1,
        'cache_expiry_time'                  => 48,
        'clear_site_cache_on_saved_post'     => 1,
        'convert_image_urls_to_webp'         => 1,
        'compress_cache'                     => 1,
        'minify_html'                        => 1,
        'minify_inline_css_js'               => 1,
        'permalink_structure'                => '',

        'compress_cache'                     => 1,
        'minify_html'                        => 1,
        'minify_inline_css_js'               => 1,

        'excluded_cookies'                   => "/^(custom_logged_in|wp-postpass|comment_author|comment_author)_?/",
    );

    return wp_parse_args($default_settings, $settings);
});
coreykn commented 3 years ago

Thanks @ouun for the details and research you've put into debugging this, I really appreciate it. Let me troubleshoot this on my end to gather more information. Updates to follow.

coreykn commented 3 years ago

This is occurring because the pre_option_cache_enabler hook is being used incorrectly: https://github.com/WordPress/wordpress-develop/blob/92b8387b4ce3ec76aa015cbb636f3d5c8d312ae8/src/wp-includes/option.php#L108-L128

The $settings variable in the snippet above is always false, so you're not merging your user default settings with the plugin default settings. Instead, you're only adding the settings in the snippet above. That means Cache Enabler then believes the plugin is out of date (because version is not set), so it updates the plugin. Upon page refresh the same thing occurs and we end up in a fun update loop. This can easily exhaust a server's resources.

The reason you didn't experience this before is because the backend would always be updated when you sent that through (by calling Cache_Enabler::update_backend()). The update handling has been changed in 1.8 to avoid the issues that we used to deal with when previously relying on the upgrader_process_complete hook.

> 1.8 https://github.com/keycdn/cache-enabler/blob/5f3424bccb3398a978fa3c27e9717b977d1ede5f/inc/cache_enabler.class.php#L725-L737

< 1.8 https://github.com/keycdn/cache-enabler/blob/45db4c70580c3d03f13ec7bc1e627ad2ebc53135/inc/cache_enabler.class.php#L440-L451

I can definitely see how this is one of the downsides of using one database option as an array instead of separate options. With this now in mind we may have to consider your old PR from last year that was related to filtering the Cache Enabler database option, but for now you'll want to ensure all settings are provided instead.

ouun commented 3 years ago

Oh wow! Thanks a lot for clarification @coreykn. So I think I've had a wrong understanding of the pre_option_{$option} hook and I am really surprised that the usage had the effect to update the DB with every pageload. However I strongly feel that we need a possibillity to merge the Plugins defaults and actual configuration with possible overwrites. I am happy that you consider using hooks as suggested in https://github.com/keycdn/cache-enabler/pull/173 and if you want, I can update that PR to match the latest version. In the meantime I follow your suggestion and update my pre_option_{$option} hook to contain all settings.

coreykn commented 3 years ago

You're welcome! In this case I'll add the filter when ready (should be shortly), but thank you for the offer. 🙂 If you'd like to try it, I'm thinking of adding the following at the top of the Cache_Enabler::validate_settings() method:

/**
 * Filters the plugin settings before being validated and maybe saved.
 *
 * @since  1.x.x
 *
 * @param  array  $settings  Plugin settings.
 */
$settings = (array) apply_filters( 'cache_enabler_settings', $settings );

The $settings array is not always all of the settings (depending on the form submitted in the plugin settings page). However, that still allows any setting to be defined and still be validated, for example:

add_filter( 'cache_enabler_settings', 'filter_cache_enabler_settings' );

function filter_cache_enabler_settings( $settings ) {

    $default_settings = array(
        'cache_expires'                  => 1,
        'cache_expiry_time'              => 48,
        'clear_site_cache_on_saved_post' => 1,
        'convert_image_urls_to_webp'     => 1,
        'compress_cache'                 => 1,
        'minify_html'                    => 1,
        'minify_inline_css_js'           => 1,
        'excluded_cookies'               => '/^(custom_logged_in|wp-postpass|comment_author|comment_author)_?/',
    );

    $settings = wp_parse_args( $default_settings, $settings );

    return $settings;
}

If you have any feedback or think something else would work better please let me know.