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

fatal error with Beaver Builder #131

Closed Pross closed 3 years ago

Pross commented 4 years ago

Beaver Builder uses \Cache_Enabler_Disk::delete_asset( site_url() ); to clear the cache on layout updates, recently cache enabler changed the args count for that method ad in recent php thats a fatal error.

[04-Sep-2020 21:13:02 UTC] PHP Fatal error: Uncaught ArgumentCountError: Too few arguments to function Cache_Enabler_Disk::delete_asset(), 1 passed in /var/www/yourdomain.com/html/wp-content/plugins/bb-plugin/extensions/fl-builder-cache-helper/plugins/cacheenabler.php on line 13 and exactly 2 expected in /var/www/yourdomain.com/html/wp-content/plugins/cache-enabler/inc/cache_enabler_disk.class.php:125
Stack trace:
0 /var/www/yourdomain.com/html/wp-content/plugins/bb-plugin/extensions/fl-builder-cache-helper/plugins/cacheenabler.php(13): Cache_Enabler_Disk::delete_asset()
1 /var/www/yourdomain.com/html/wp-includes/class-wp-hook.php(289): FLCacheClear\Cacheenabler::run()
2 /var/www/yourdomain.com/html/wp-includes/class-wp-hook.php(311): WP_Hook->apply_filters()
3 /var/www/yourdomain.com/html/wp-includes/plugin.php(478): WP_Hook->do_action()
4 /var/www/yourdomain.com/html/wp-content/plugins/bb-plugin/classes/class-fl-builder-model.php(4784): do_action()
5 /var/www/yourdomain.com/html/wp in /var/www/yourdomain.com/html/wp-content/plugins/cache-enabler/inc/cache_enabler_disk.class.php on line 125
[04-Sep-2020 21:26:27 UTC] PHP Fatal error: Uncaught ArgumentCountError: Too few arguments to function Cache_Enabler_Disk::delete_asset(), 1 passed in /var/www/yourdomain.com/html/wp-content/plugins/bb-plugin/extensions/fl-builder-cache-helper/plugins/cacheenabler.php on line 13 and exactly 2 expected in /var/www/yourdomain.com/html/wp-content/plugins/cache-enabler/inc/cache_enabler_disk.class.php:125
coreykn commented 4 years ago

Both arguments in Cache_Enabler_Disk::delete_asset() are required. I understand that we don't have a hook available yet for clearing the cache by URL, which means if anything Cache_Enabler::clear_page_cache_by_url() is a much better choice to use when using a URL to clear the cache. We are planning on evaluating all of our hooks and improving them shortly.

At what point did you start using the callback posted above? If added at or before version 1.4.6 (Aug 31, 2020) that would have cleared the complete cache (as the entire site_url() directory would have been cleared). If that was the desired action it would be better to use the ce_clear_cache action hook instead. As of version 1.4.7 it will only clear the page cache of site_url() (#110). Using site_url() over home_url() is not recommended in the event they are different because home_url() determines the cache structure as it's what the user receives on the front end.

What page(s) cache are you attempting to clear? If it's just any post type that has been updated I would actually recommend completely removing what you have added as they will automatically be cleared in version 1.5.0 (#129). If it's the complete cache use the ce_clear_cache action hook.

Pross commented 4 years ago

If there is an alternative method to empty the cache when a Beaver Builder layout is updated I will change our code and use that, but for the millions of users we have right now, simply clicking publish will throw a fatal error with your plugin. I cant give you exact numbers but there is a fair chunk of people who simply don't update and are using older versions of beaver builder, if they install your plugin they will get the fatal error. If a layout I edited ( which is a cpt ) the layout can be used anywhere, on any pages... simply clearing the cache for a non public cpt isn't going to work, which is why we clear the cache totally.

coreykn commented 4 years ago

Thanks for the additional information. I'm unsure what lead to Cache_Enabler_Disk::delete_asset() even being used then. You'll want to use the proper ce_clear_cache action hook instead to clear the complete cache.

Pross commented 4 years ago

Well it was what I found at the time and seemed to work.

So like I said I can convert our code to use that new action when layouts are updated but it isn't going to fix the fatal error for existing users, seems like a simple change to add a default for the argument?

coreykn commented 4 years ago

This is why hooks are used to avoid these types of issues. I will need to think about what will be best going forward. With the current direction Cache Enabler is heading I don't think I want default parameter values set in Cache_Enabler_Disk::delete_asset(). They are both intentionally required to explicitly set what is being cleared and what type of clearing is being performed. Even if I come to the conclusion that defaults are better set in this method instead it would not be dir as proposed in #132 because clearing the entire directory (which could consist of any number of subpages) will never be the default behavior again.

Pross commented 4 years ago

Here is our current code, seems to be only multisite users affected.

The run method is triggered when a beaver builder layout is published in the frontend.

<?php
namespace FLCacheClear;
class Cacheenabler {

    var $name = 'Cache Enabler';
    var $url  = 'https://wordpress.org/plugins/cache-enabler/';

    static function run() {

        if ( class_exists( '\Cache_Enabler' ) ) {
            if ( ! is_multisite() ) {
                \Cache_Enabler::clear_total_cache();
            } else {
                \Cache_Enabler_Disk::delete_asset( site_url() );
            }
        }
    }
}
coreykn commented 3 years ago

I've updated this (now deprecated) method to be as it was before the change in version 1.4.7, so calling what you have above will continue to work. I'll provide new hooks to use instead shortly.

coreykn commented 3 years ago

@Pross I'd recommend making use of the cache_enabler_clear_site_cache action hook that was introduced in version 1.6.0 now that a large portion of installs have been updated to this version. This hook will clear the site cache for the current blog ID, whether that is a single installation or a multisite. For backwards compatibility one option would be checking the plugin version, like with the get_file_data() function. (The CE_VERSION constant was only added in version 1.5.0 so that would not work for versions before this.)