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

Warning: mkdir(): File exists [...] src/includes/traits/Plugin/InstallUtils.php on line 356 #786

Closed
raamdev opened this Issue Jun 15, 2016 · 5 comments

Comments

Projects
None yet
2 participants
@raamdev
Member

raamdev commented Jun 15, 2016

Originally reported in the community forum, and here.

Warning: mkdir(): File exists in /home/www/wp-content/plugins/comet-cache/src/includes/traits/Plugin/InstallUtils.php on line 356

Here's src/includes/traits/Plugin/InstallUtils.php#L356.


Quoting private discussion from Slack.

@raamdev writes...

How do you feel about suppressing Warnings on this line: https://github.com/websharks/comet-cache/blob/160521/src/includes/traits/Plugin/InstallUtils.php#L356

Related support thread where some users, who unknowingly have PHP display_errors = On, are seeing a warning occasionally show up on the front-end of their site: https://wordpress.org/support/topic/php-warning-just-showed-up-on-my-published-site

WordPress seems to suppress such mkdir() warnings in the WordPress Filesystem Class: https://github.com/WordPress/WordPress/blob/4.5/wp-admin/includes/class-wp-filesystem-direct.php#L434

@jaswsinc writes...

Suppressing that warning... Well, I think it depends on the context in which a call to mkdir() is being made, and also on the arguments being passed to mkdir(). In the line you referenced in the WP Filesystem class, it's a function that returns a value true or false so a caller can test for failure. The warning is suppressed there for a good reason. A caller may try to create something and ​_expect_​ a possible failure, in which case no warning should be triggered. The caller can decide what to do.

As for the arguments, the big one would be the -p flag in the last argument. If that's true and all parents should be created too, then a warning becomes slightly more appropriate.

So looking at the context and arguments we are using in Comet Cache, I'm seeing that a call to mkdir() here looks like an attempt to rectify a rather strange case where the /cache directory does not exist. Normally, that should not occur, because the cache directory is created when the plugin is installed and unless I'm mistaken it's not even deleted when we do a full wipe. Only when the plugin is uninstalled is that directory removed.

Of course, it could be removed by a site owner in some way by mistake, or it may not exist after an automated deployment of some sort.

Looking just above that line I'm seeing a similar failure where we just return false and do nothing instead of failing hard with a warning. So I suppose we could do the same for the mkdir() call also. Something like @mkdir(...); return false; .

Having said that, if there's anything we can do to diagnose this I'd be curious to know why that's happening. It seems like there is another problem on the site if that call fails.

@raamdev writes...

Looking just above that line I'm seeing a similar failure where we just return false and do nothing instead of failing hard with a warning. So I suppose we could do the same for the mkdir() call also. Something like @mkdir(...); return false;

Yeah, that sounds like a good idea to me. We also check if !is_dir($cache_dir) again a few lines after that, so we have some redundancy in there to make sure that the cache directory exists and is writable before we return true.

@jaswsinc writes...

I'd try to look closer at this and see if we can figure out why that might be occurring. If you're correct, then maybe it has something to do with the stat cache and we can prevent that scenario from occurring altogether by running clearstatcache() in PHP in more places than we do currently.

PHP is supposed to maintain the stat cache for us automatically. More recent versions of PHP have improved in this regard too. However, there have been a few cases over the years where I have noticed that some functions do not handle this properly. For instance, in older PHP versions < 5.3 it was possible to unlink() a file and the stat cache was ​_NOT_​ cleared implicitly at all times. I had to do it by calling clearstatcache() explicitly.

To illustrate that point...

if (is_file($file)) {
    unlink($file);
}
if (is_file($file)) {
  // true? ... nope, gotta clear the stat cache so PHP knows what just happened.
}

So I'd start by doing a review of this article at PHP.net to see if anyone has documented anything recently that is related to this sort of behavior.
http://php.net/manual/en/function.clearstatcache.php

Also do a review of any functions in Comet Cache that deal with file deletions, renames, etc. Anything that might trick PHP into thinking the cache directory does not exist when it does. Or, you can also look at functions that deal with directory creation and see if there is something that tricks PHP into thinking it doesn't exist yet when it does, because we created the directory in the same process.

@raamdev raamdev added the bug label Jun 15, 2016

@raamdev raamdev added this to the Next Release milestone Jun 15, 2016

@raamdev

This comment has been minimized.

Member

raamdev commented Jun 15, 2016

Noting that I also requested PHP version info and phpinfo() from the two users who originally reported the issue. I'll update here if/when I hear back.

@raamdev raamdev modified the milestones: Next Release, Future Release Jun 20, 2016

@raamdev

This comment has been minimized.

Member

raamdev commented Jun 30, 2016

@jaswsinc writes...

PHP is supposed to maintain the stat cache for us automatically.

Right, but if a system call modifies things outside of PHP, the stat cache can become outdated; see this comment: http://php.net/manual/en/function.clearstatcache.php#116554

I also came across this interesting comment about why using is_dir() is not the best way to check if a directory exists before creating it:

This is problematic on unix systems because everything is a file, even directories. If a file exists with the same name, said directory cannot be added. So, simply checking if a "file" is a directory isn't sufficient. Instead you should check if(!file_exists($path)).

Here's what we're currently doing in Comet Cache:

        if (!is_dir($cache_dir)) {
            mkdir($cache_dir, 0775, true);
        }

So, if $cache_dir is, for some reason, a file instead of a directory, that conditional will pass and Comet Cache will attempt to create the directory, but the mkdir() call will fail with a warning (because the "file exists").


@jaswsinc Here's what I propose we do to make that section of code a bit more robust:

        clearstatcache();
        if (!file_exists($cache_dir)) {
            mkdir($cache_dir, 0775, true);
        }
@jaswrks

This comment has been minimized.

Member

jaswrks commented Jul 1, 2016

That looks good to me also.

@raamdev raamdev self-assigned this Aug 24, 2016

raamdev added a commit to websharks/comet-cache-pro that referenced this issue Sep 2, 2016

@raamdev

This comment has been minimized.

Member

raamdev commented Sep 2, 2016

Next Release Changelog:

  • Bug Fix: In some scenarios Comet Cache might produce a false-positive "Warning: mkdir(): File exists" message when checking if the cache directory exists. Comet Cache now calls clearstatcache() and uses file_exists() instead of is_dir() to help make this check more robust. See Issue #786.

@raamdev raamdev closed this Sep 2, 2016

raamdev added a commit that referenced this issue Sep 17, 2016

Phing release of v160917 with the following changes:
- **New Feature** (Lite): The Clear Cache button is now available in the Admin Toolbar for the Lite version of Comet Cache.
- **New Feature** (Pro): Comet Cache Pro is now fully compatible with [WordPress Automatic Background Updates](https://codex.wordpress.org/Configuring_Automatic_Background_Updates#Plugin_.26_Theme_Updates_via_Filter). If you enable automatic background updates for plugins, and you save valid Comet Cache Pro License Credentials in the _Comet Cache Pro → Plugin Options → Update Credentials_ panel, you will automatically receive Pro plugin updates. Props @jaswsinc. See [Issue #289](#289).
- **Bug Fix**: In some scenarios Comet Cache might produce a false-positive "Warning: mkdir(): File exists" message when checking if the cache directory exists. Comet Cache now calls `clearstatcache()` and uses `file_exists()` instead of `is_dir()` to help make this check more robust. See [Issue #786](#786).
- **Bug Fix**: Fixed a bug where the Comet Cache PHP requirements check would fail and produce a fatal error when upgrading from a version of Comet Cache that did not require an extension that is now required by newer releases. This would occur when, for example, the required PHP `mbstring` extension was missing. Props @jaswsinc for finding the bug. See [Issue #817](#817).
- **Bug Fix**: Fixed a bug where upgrading from v160521 would result in the Client-Side Cache option being reset to the default (disabled). If you enabled the Client-Side Cache at some point, now is a good time to double-check that it's still enabled. This bug fix also improves the reliability of all version upgrade routines that Comet Cache runs during upgrades. See [Issue #807](#807).
- **Compatibility / Bug Fix**: The automatic Clear Cache routines that cleared the entire cache automatically whenever _WordPress Dashboard → Settings → General_ was updated, were being too aggressive and not taking into consideration other plugins that might also be using the same `options-general.php` URL. As a result, the entire cache was being unnecessarily cleared when the settings for those other plugins were saved. Props to @futtta from Autoptimize for reporting. See [Issue #825](#825).
- **UI Enhancement:** Adjusted option page font styles for WordPress v4.6 to better match existing style. See [Issue #271](websharks/comet-cache-pro#271).
- **ManageWP Compatibility** (Pro): Comet Cache Pro is now compatible with ManageWP, a service that allows remote management of multiple WordPress sites. Comet Cache Pro Plugin Updates will now appear in the ManageWP dashboard and, assuming you have saved valid license credentials in _Dashboard → Comet Cache Pro → Plugin Options → Update Credentials_, you will be able to upgrade Comet Cache Pro remotely from the ManageWP Dashboard. Props @jaswsinc. See [Issue #465](#465).
- **InfiniteWP Compatibility** (Pro): Comet Cache Pro is now compatible with InfiniteWP, an application that allows you to manage multiple WordPress sites from a single location. Comet Cache Pro Plugin Updates will now appear in the InfiniteWP dashboard and, assuming you have saved valid license credentials in _Dashboard → Comet Cache Pro → Plugin Options → Update Credentials_, you will be able to upgrade Comet Cache Pro remotely from the InfiniteWP Dashboard. See [Issue #394](#394).
- **Rewritten Pro Plugin Updater**: The Comet Cache Pro Plugin Updater has been redesigned to use the built-in WordPress plugin updater system. When a Comet Cache Pro update is available, it now appears in the WordPress Updates section and in the Plugins list, like other WordPress plugins and can be updated normally like other WordPress plugins, as long as you have saved valid Comet Cache Pro license details in the new "Update Credentials" options panel. Props @jaswsinc. See [Issue #272](websharks/comet-cache-pro#272).
- **Code Style**: The `WP_CACHE` line that gets inserted into the `wp-config.php` file to enable caching now follows the [WordPress PHP Code Standards](https://make.wordpress.org/core/handbook/best-practices/coding-standards/php/). Props @szepeviktor. See [Issue #799](#799).
- **Compatibility** (Pro): When the Autoptimize plugin is active, the Comet Cache Pro HTML Compressor panel now shows a friendly notice explaining that both the HTML Compressor and Autoptimize should not be enabled at the same time because they both address the same performance improvements. The rest of Comet Cache works great alongside Autoptimize and whether you use the HTML Compressor or Autoptimize is a matter of preference. Props to @futtta from Autoptimize for the continued collaboration.
@raamdev

This comment has been minimized.

Member

raamdev commented Sep 17, 2016

Comet Cache v160917 has been released and includes changes from this GitHub Issue. See the v160917 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 (#786).

@websharks websharks locked and limited conversation to collaborators Sep 17, 2016

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.