fix for PHP warning "is_file(): open_basedir restriction in effect", round 2

[rindeal]

This reverts commit d234b8e in PR #47 and returns back the is_dir() check.

I was working with too many hosts simultaneously and checked the file_exists() method on a host that worked perfectly even before the fix and as I updated the files on the problematic host the problems returned and the warning message changed to:

PHP Warning: file_exists(): open_basedir restriction in effect. File(/home/users/.../web/user/plugins/.gitkeep/.gitkeep.yaml) is not within the allowed path(s): (/home/users/.../:/usr/share/pear/) in .../web/system/src/Grav/Common/Config.php:237

I'm sorry for this and have triple-checked that this code really finally fixes the issue.

[w00fz]

Question: how come the user/plugins in question is out of scope of your open_basedir?

Can you explain how is your setup? It is a bit strange it fails like that when trying to open a file that should be child of the root app.

[rhukster]

Let's back up a bit here. I think there is another issue that is the root cause here. The fact that this file is outside the basedir restriction is more the problem. This build() method loops over all the files in the PLUGINS_DIR. If it cannot access the first file, it won't be able to access any plugins either.

/home/users/.../web/user/plugins/.gitkeep/.gitkeep.yaml

This is not right for you, not sure why that would be. Can you debug in your system and see what PLUGINS_DIR is? perhaps we can work out what's going on here.

[rindeal]

PLUGINS_DIR resolves normally to /home/users/.../web/user/plugins/. Please note that the three dots notes a censored content. The whole issue I see in this is that the DirectoryIterator iterates over everything in the PLUGINS_DIR, including files. And as the file_exists()/is_file() tries to check for a file in a file, the warning is triggered. I have also googled a bit and found out that this is actually a PHP bug and was repeatedly reported since 2010-06-12 14:58 though still not fixed. So just do checks for a directory before you start checking for a file and everything will be OK.

[w00fz]

Then it should be like this:

/** @var \DirectoryIterator $plugin */
foreach ($iterator as $plugin) {
    if (!$iterator->isFile() || $iterator->isDot()) continue;

    $name = $plugin->getBasename();
    $file = $plugin->getPathname() . DS . $name . YAML_EXT;

    $modified = filemtime($file);
    $plugins["plugins/{$name}"] = $modified;
}

Since the DirectoryIterator has already its own isFile method check and it will also skip the dot files.

Can you try?

[rhukster]

I think that the @w00fz code should solve the case of this .gitkeep file sitting in the plugins/ folder from tripping it up, but I think we still need that file_exist() check to ensure the plugin has an appropriate configuration file in the format pluginname/pluginname.yaml. What do you think @w00fz ?

[w00fz]

Yeah you are right. I also have a typo up there in the check for isFile():

/** @var \DirectoryIterator $plugin */
foreach ($iterator as $plugin) {
    if ($iterator->isFile() || $iterator->isDot()) continue;

    $name = $plugin->getBasename();
    $file = $plugin->getPathname() . DS . $name . YAML_EXT;

    if (!file_exists($file)) continue;

    $modified = filemtime($file);
    $plugins["plugins/{$name}"] = $modified;
}

[rindeal]

Yes the second snippet is OK, it var_dumped only non-dot directories and thus triggered no warning.

[w00fz]

Ok that's cool. I just realized we can just revert that file_exists back to is_file, since it's faster and we are skipping undesired files already.

[rindeal]

Well, I benchmarked it now and file_exists() has been always slightly faster...

$glob=glob(__DIR__ . '/*');
$t = microtime(true);
for ($i = 0; $i < 10000; $i++) {
    foreach ($glob as $file) {
        file_exists($file);
    }
}
$t=microtime(true)-$t;
echo round($t*1000,2);
echo "\n";
$t = microtime(true);
for ($i = 0; $i < 10000; $i++) {
    foreach ($glob as $file) {
        is_file($file);
    }
}
$t=microtime(true)-$t;
echo round($t*1000,2);

2269.7
2356.83

[rindeal]

most probably because it only checks if inode exists and doesnt look at it's content.

[w00fz]

You need to use clearstatcache() if you run a benchmark like that or it will be cached see here.

But the thing is file_exists checks for both files and folders, is_file only for files (and i think symlinks too). Since we know we are expecting a file it is more appropriate to use is_file i think.

[rindeal]

Right, now its

2771.75
2413.48

So should I commit it to this branch or will you create a new one?

[w00fz]

Commit in here and we can merge.

[rindeal]

Committed

[w00fz]

Cheers!