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

1.7 - PHP Warning: chmod(): No such file or directory #220

Closed janvitos closed 3 years ago

janvitos commented 3 years ago

Hi,

After installing Cache Enabler 1.7, I noticed there is a warning message that floods the NGINX error log whenever I clear the cache with the cache_enabler_clear_complete_cache hook.

Here's the error I am getting:

PHP Warning:  chmod(): No such file or directory in /home/wordpress/wp-admin/includes/class-wp-filesystem-direct.php on line 173

Thanks.

coreykn commented 3 years ago

I'm unable to replicate this issue on a fresh installation of WordPress (5.7) and Cache Enabler (1.7.0 and 1.7.1). No errors occur when the cache_enabler_clear_complete_cache hook is fired and the cache is cleared as expected. When and where are you firing this action hook?

janvitos commented 3 years ago

Hi @coreykn, thanks for looking into the issue.

I've included a php file inside functions.php for clearing the Cache Enabler cache and Cloudflare cache when clicking on a button.

This is the code that fires cache_enabler_clear_complete_cache:

add_action( 'wp_ajax_purge_the_cache_enabler', 'purge_cache_enabler', 10, 0 );
function purge_cache_enabler() {
    if ( has_action( 'cache_enabler_clear_complete_cache' ) ) {
        do_action( 'cache_enabler_clear_complete_cache' );
    }
}

As you can see, it is a pretty basic function that is called by an ajax request.

janvitos commented 3 years ago

As a test, I will try hooking into cache_enabler_site_cache_cleared instead of cache_enabler_complete_cache_cleared. This means I will NOT be using ajax and my custom button to call cache_enabler_clear_complete_cache.

This way, when I click on the existing "Clear Site Cache" button in the admin toolbar, it should also fire my script, and we can see if it's a problem related to the ajax call (or not).

I will get back with the results.

janvitos commented 3 years ago

Well the results were NOT conclusive.

Clicking on Cache Enabler's "Clear Site Cache" button generated the same errors in the NGINX log.

This means I have essentially bypassed the cache_enabler_clear_complete_cache hook and used Cache Enabler's own cache purge system (I assume it uses the cache_enabler_clear_site_cache hook for the button).

I will do more testing tonight.

janvitos commented 3 years ago

I believe I found the issue.

There is a section of code in cache_enabler_disk.class.php where the plugin checks / sets directory permissions to 755 using chmod. It starts at "private static function mkdir_p" on line 921.

With my setup, I don't use 755 permissions, I use 775. So I believe once the directories are set to 755, they cannot be read / changed properly by the plugin.

So what I did was to change all 755 instances to 775, and I completely stopped getting errors in the NGINX error log. I tried purging the cache many times and did not encounter a single error.

I think many people use 775 permissions instead of 755 for different reasons. A check should be added to see if the permissions are 755 or 775.

Cheers.

janvitos commented 3 years ago

As I was writing some code to check if directory permissions are 755 or 775 and set them accordingly if they aren't, I realized there is some logic that will be hard to figure out.

Right now, the plugin checks if the directory permissions are NOT 755, and if they aren't, it sets them to 755. But how will we know if we need to set them to 755 or 775?

Depending on the server setup, we need to figure out somehow if the plugin needs to set the permissions to 755 or 775.

janvitos commented 3 years ago

I guess we could check the wp-content directory for proper permissions with getchmod and use those for setting the proper permissions on the cache directories.

janvitos commented 3 years ago

Here's the code I have written. It works perfectly with my 775 setup and should work with 755 as well.

    private static function mkdir_p( $dir ) {

        $parent_dir = dirname( $dir );
        $fs = self::get_filesystem();
        $content_perms = '0'.$fs->getchmod( WP_CONTENT_DIR );

        // check if directory and its parent have 755 or 775 permissions
        if ( $fs->is_dir( $dir ) && ( $fs->getchmod( $dir ) === '755' || $fs->getchmod( $dir ) === '775' ) && ( $fs->getchmod( $parent_dir ) === '755' || $fs->getchmod( $parent_dir ) === '775' ) ) {
            return true;
        }

        // create any directories that do not exist yet
        if ( ! wp_mkdir_p( $dir ) ) {
            return false;
        }

        // check parent directory permissions
        if ( ( $fs->getchmod( $parent_dir ) !== '755' ) && ( $fs->getchmod( $parent_dir ) !== '775' ) ) {
            return $fs->chmod( $parent_dir, $content_perms, true );
        }

        // check directory permissions
        if ( ( $fs->getchmod( $dir ) !== '755' ) && ( $fs->getchmod( $dir ) !== '775' ) ) {
            return $fs->chmod( $dir, $content_perms );
        }

        return true;
    }

This will set the permissions on the new cache directories based on the WP_CONTENT_DIR permissions, which should reflect the WordPress installation's overall permissions.

But if we want to go a step further, we could simply remove the IFs and just always set the directory permissions based on the WP_CONTENT_DIR permissions.

But maybe you have those checks in place for other reasons, so I will let you figure that part out ;)

janvitos commented 3 years ago

Hi,

Any update on this?

Thank you.

coreykn commented 3 years ago

The Cache_Enabler_Disk::mkdir_p() method was added in PR #194 to begin explicitly setting the permissions of directories that are created by Cache Enabler, fixing the linked issue. It purposely avoids inheriting the access permissions from the wp-content directory due to the example provided in the PR.

I haven't been able to replicate this specific issue so I don't have an exact reason for why this is occurring. If this is related to the Cache_Enabler_Disk::mkdir_p() method then what you're experiencing would not be directly related to clearing the cache because this method is not called when clearing the cache. Instead, based on what you've provided it sounds like the issue is occurring for you when a cache directory is actually created? (This would occur when a page is being cached as this method is primarily called when generating static HTML files.) For example, if you or a visitor visits a page right after clearing the cache or if you potentially had some sort of cache warming setup. Are you able to confirm if that is the case?

If needed, we can improve this handling. Ideas on the top of my head would be allowing the permissions explicitly being set to be customized. This could be done through the FS_CHMOD_DIR constant or something similar. Another idea would be only changing the permissions if less than what we specify (e.g. 0755).

janvitos commented 3 years ago

Hi,

Thanks for getting back to me.

I do have a cache warming mechanism, but I'm not sure how that could cause the issue? It is simply a script that uses my website's sitemap to visit all URLs from my website. This is done through curl requests and works just like any other regular visit to my website. It goes through PHP and Wordpress just like any other regular visit would.

Like I mentioned previously, the problem seems to be related to the 775 permissions. I already provided some code that fixed the issue for me. I never talked about the Cache_Enabler_Disk::mkdir_p() method. It is related to the getchmod function.

coreykn commented 3 years ago

I do have a cache warming mechanism, but I'm not sure how that could cause the issue?

It was never mentioned it was the cause of the issue as I don't have enough information to make that assessment. I was simply trying to gather more information to try and understand what's occurring.

Like I mentioned previously, the problem seems to be related to the 775 permissions. I already provided some code that fixed the issue for me.

Right, I see that. However, I may be wrong in this assumption but I would think the error would be something related to "Operation not permitted in ..." if it was related to the permissions not "No such file or directory in ..." like you're receiving. This is why I'm asking questions to ensure we come up with the right solution.

I never talked about the Cache_Enabler_Disk::mkdir_p() method.

This is the method that you've modified above and referenced more than once.

Anyways, as for the next step, does this error still occur if you do the following:

  1. Install the latest version of Cache Enabler to ensure there are no modifications made to the plugin.
  2. Temporarily disable any extensions made to Cache Enabler, as in any custom interactions made with hooks and/or methods, like your cache warming or any custom cache clearing.
janvitos commented 3 years ago

Hi,

  1. I already tried uninstalling / reinstalling the latest version of Cache Enabler but to no avail.

  2. As mentioned here https://github.com/keycdn/cache-enabler/issues/220#issuecomment-807529390, I also tried this and it didn't work. I completely disabled all of my code related to cache purging / priming and all other interactions with the plugin and the error was still shown many times in the log when clicking on Clear Site Cache.

davelit commented 3 years ago

I had the same issue (wp-content directory is intentionally write protected) and worked around it by setting the FS_CHMOD_DIR constant to 'direct' in wp-config.php.

coreykn commented 3 years ago

@janvitos, is what @davelit shared a solution that worked for you? I'm finding this issue to be quite rare but do want to come up with a good solution for installations like yours. I appreciate what you've shared, however, it won't work for Cache Enabler as we would need something more dynamic (complete control instead of only 0755 and 0775 for example). If it's not a solution would being able to customize the permissions set by Cache Enabler be the preferred solution?

janvitos commented 3 years ago

I will try @davelit's solution and get back to you with my findings.

janvitos commented 3 years ago

Hey @davelit, are you talking about the FS_METHOD constant? Because you cannot set FS_CHMOD_DIR as 'direct'.

If so, then I already have FS_METHOD set as 'direct', which means this wouldn't be the solution for my issue.

Thanks.

janvitos commented 3 years ago

Hey @coreykn, out of curiosity, why does the mkdir_p() function need to check / set the directory permissions? You are using the native Wordpress function wp_mkdir_p() to create the directories, and that function already assigns proper directory permissions when creating new directories. In my case, it assigns 775 permissions.

So basically, if permissions are already managed by Wordpress, why bother checking them / setting them with your own functions? If you'd remove permission checking / setting altogether, then the mkdir_p() function would work with all permissions (755, 775, etc.)

Maybe I'm missing something, so please enlighten me.

coreykn commented 3 years ago

why does the mkdir_p() function need to check / set the directory permissions?

The Cache_Enabler_Disk::mkdir_p() method was added in PR #194 to begin explicitly setting the permissions of directories that are created by Cache Enabler, fixing the linked issue. It purposely avoids inheriting the access permissions from the wp-content directory due to the example provided in the PR.

The PR previously mentioned provides insight to why this change was implemented. Our commits will always contain what was changed and the reason for why that change was introduced. There isn't a reason to go back and forth on this anymore as I feel like we're just repeating ourselves. If you happen to have a preference on what change we should introduce to allow controlling the access permissions we set let me know.

davelit commented 3 years ago

Hey @davelit, are you talking about the FS_METHOD constant? Because you cannot set FS_CHMOD_DIR as 'direct'.

If so, then I already have FS_METHOD set as 'direct', which means this wouldn't be the solution for my issue.

Thanks.

@janvitos My bad, I meant the FS_METHOD of course. I'm sorry to hear that it didn't help.

janvitos commented 3 years ago

The PR previously mentioned provides insight to why this change was implemented

I just noticed the PR linked in your message and I now understand why you created that function.

There isn't a reason to go back and forth on this anymore as I feel like we're just repeating ourselves

You are right about this @coreykn.

If you happen to have a preference on what change we should introduce to allow controlling the access permissions we set let me know

I believe adding a way (perhaps a filter) to specifiy the directory permissions would be the best way to go forward. Then one could simply use that filter to override the 755 permissions and set them to 775 for example and it would fix the issue.

Thank you.

coreykn commented 3 years ago

Thanks @janvitos. 🙂 I appreciate your patience. I will get something added to resolve this.

janvitos commented 3 years ago

I will get something added to resolve this

Please let me know once the PR is ready and I will happily test it for you.

coreykn commented 3 years ago

@janvitos, if you'd like to test this out, I'm thinking something like this: https://github.com/coreykn/cache-enabler/tree/add-mode-filter

It would introduce cache_enabler_mkdir_mode as a filter hook that would allow the mode that Cache Enabler uses by default (0755) to be filtered, for example:

add_filter( 'cache_enabler_mkdir_mode', 'filter_cache_enabler_mkdir_mode' );

function filter_cache_enabler_mkdir_mode( $mode ) {
    return 0775;
}

It would expect the mode to be in octal format (e.g. having the leading zero).

janvitos commented 3 years ago

That sounds great @coreykn.

I will test it soon and let you know how it goes.

janvitos commented 3 years ago

Hi @coreykn,

I have implemented your code and have added the following filter to my functions.php file:

add_filter( 'cache_enabler_mkdir_mode', function() {
    return 0775;
});

So far so good, there are no more errors in my web server log.

coreykn commented 3 years ago

Excellent! Thank you for testing and confirming that for me. I'll have this filter introduced shortly.

janvitos commented 3 years ago

Just to confirm that I've been running the patch for 3 days now and there has been zero errors in my web server's error log.