Try to use is_file() instead of file_exists() in DrupalKernel::findSitePath()

So let's make the differences clear: a) both use the stat() cache. b) if the file is a symbolic link, file_exists resolves it. This can result in more file system hits if the file wasn't and won't be accessed otherwise. This is very unlikely. We do not run file_exists just for the heck of it. If it gets used then the system needs to resolve the link anyways c) in all other cases, the speed difference will be a few C instructions inside PHP which is negligible.

I conclude that we dont need to worry about the speed of these or the speed difference.

"I conclude that we dont need to worry about the speed of these or the speed difference."
Agreed, this is not a speed issue.

However, I think there's a consistency issue, which is more what this issue is about addressing.
My thought is that we should use the right function for the job, not because it's faster, but because it makes more sense. Less potential "hidden features" :)

Of course but before we embark on some rewrite it needed to be clarified whether it's a journey worth taking or it's futile.

is_file just checks that it is a proper file name while, file_exists checks if file really exists in file system. If is_file returns false then file_exists will return false. The side effects of using is_file - errors thrown when drupal attempts to include not existent file, the side effect of file_exists - to slow. And it is a real problem! it is to slow.

So how can we solve this? Write own function similar to, here is pseudo code:

function drupal_is_file($filename) {
  $db_loaded = CONDITION ? true : false;
  if (!db_loaded) {
    return is_file($filename);
  } else {
    drupal_file_exists($filename);
  }
}

function drupal_file_exists($filename) {
  // check if file exits in cache first
}

So before database is loaded it could be very fast, after it could be fast too cause of using cache. Advantage: it will be easier to update this check in one place, rather then in all places.

What is too slow? file_exists? data to prove?

sorry, my last comment was wrong.

is_file checks if file exists and it is a file and not directory, while file_exists checks that there is a file or folder or anything else in the system. Regarding performance I was wrong too, however on my machine is_file working 10% faster then file_exists. Here is a prove:

$n = 1000000;
$t1 = microtime(true);

for ($i=0; $i <= $n; $i++) {
  is_file($i . '_1.txt');
}

$t2 = microtime(true);

for ($i=0; $i <= $n; $i++) {
  file_exists($i . '_2.test.txt');    
}

$t3 = microtime(true);

print  '<br>is_file: ' . ($t2-$t1);
print  '<br>file_exists: ' .($t3-$t2);
print  '<br>t1: ' . $t1 . ', t2:' .  $t2 . ', t3:' . $t3;

P.S.
I have XFS as file system, however my friend having results opposite with NTFS.

<?php
$n = 1000000;
$t1 = microtime(true);

for ($i=0;$i<$n;$i++) {
  is_file('a.txt');
}

$t2 = microtime(true);

for ($i=0;$i<$n;$i++) {
  file_exists('a.txt');
}

$t3 = microtime(true);

print  'is_file: ' . ($t2-$t1);
print  "\nfile_exists: " .($t3-$t2);
print  "\nt1: " . $t1 . ', t2:' .  $t2 . ', t3:' . $t3;
?>

So, I ran this for when,

a.txt exists with apc enabled:
is_file: 0.67443084716797
file_exists: 2.1848950386047

a.txt does not exist with apc enabled:
is_file: 1.3890597820282
file_exists: 2.1207160949707

Just for fun, disabled apc:
a.txt exists with apc disabled:
is_file: 0.66702103614807
file_exists: 2.1579539775848

a.txt does not exist with apc disabled:
is_file: 1.3979690074921
file_exists: 2.085746049881

PHP 5.3.5-1ubuntu7.3 with Suhosin-Patch (cli) and ext4 fs.

So I'd say when a file exists, is_file() is about 3x faster, when it doesn't, it's about 35% faster.

Now, how many times is file_exists() called, that could be replaced by is_file()?
I'm guessing it's not enough to make any measurable performance difference :)
Still interested to see though...

I realized later that maybe my testing was a little flawed in not testing separate files... don't have time to test this more thoroughly right now though.
I.e. maybe caching is skewing some of the results.

There is a huge performance difference between file_exists() and is_file() for non-existing files.

Related issues:

- CTools: #1234410: file_exists should be replaced with is_file (better performance) - Patch attached
- Omega: #1348820: Reduce alpha_invoke performance impact

So not a lot of movement on this in the last 6 months. Just wanted to post a related link - http://stackoverflow.com/questions/4099103/is-file-file-exists-performan...

Let's pick one way for D8 & roll it out across the board.

In grepping for instances of the use, there are lots of places where file_exists is used in Core right now compared to is_file:

quickstart@qs10:/home/dm8$ wc ~/file_exists.txt
376 2618 47061 /home/quickstart/file_exists.txt
quickstart@qs10:/home/dm8$ wc ~/isfile.txt
73 449 9745 /home/quickstart/isfile.txt

Should be something liek this to catch them all (although that doesn't see to work):
find ./ -type f -exec sed -i 's/file_exists/is_file/g' {} \;

http://stackoverflow.com/questions/6758963/find-and-replace-with-sed-in-...

(the slash in the i/o tag breaks the autocomplete from adding new tags)

In some cases, is file_exists()/is_file() enough? Should we also consider is_readable()/is_writable()?

That's a separate issue altogether. Create a new issue if you want to for that please.

find . -type f -exec sed -i "" 's/file_exists/is_file/g' {} \;
works on a mac (had to add the "". see http://forums.freebsd.org/showthread.php?t=12235 )
wait. that messed up my .git
find . -name '.git' -prune -o -type f -exec sed -i "" 's/file_exists/is_file/g' {} \;
oops. need to ignore .png .jpg .ico .gif files
Note this resaves files, so fixes the no new line at end of file problem. is that ok? if not, I ... can edit the patch by hand to take out those changes.
find . -name '.git' -prune -o -name '*.png' -prune -o -name '*.jpg' -prune -o -name '*.gif' -prune -o -name '*.ico' -prune -o -type f -exec sed -i "" 's/file_exists/is_file/g' {} \;
and need to ignore .patch files or my patchs get corrupted

ok. that is not a good approach. too many No newline changes in .js .css .yml etc.
gee. maybe I should have stuck with excluding stuff like .js .css .gz.

Just do it *for* .php .inc .module .sh .engine .install files.

find . -name '.git' -prune -o -type f -name '*.engine' -exec sed -i "" 's/file_exists/is_file/g' {} \;
find . -name '.git' -prune -o -type f -name '*.sh' -exec sed -i "" 's/file_exists/is_file/g' {} \;
find . -name '.git' -prune -o -type f -name '*.php' -exec sed -i "" 's/file_exists/is_file/g' {} \;
find . -name '.git' -prune -o -type f -name '*.install' -exec sed -i "" 's/file_exists/is_file/g' {} \;
find . -name '.git' -prune -o -type f -name '*.module' -exec sed -i "" 's/file_exists/is_file/g' {} \;

(install gets through requirement checks at this point)

find . -name '.git' -prune -o -type f -name '*.inc' -exec sed -i "" 's/file_exists/is_file/g' {} \;

(at this point it does not.)

Still results in 82 "No newline" changes.

did a git add for sites/default/default.settings.php since it has a file_exists but was ignored

replaces file_exists with is_file (for performance reasons with not existing files see #9

Here is the problem.. after this (after the .inc files), install does not work. file_exists returns true if it's a directory that exists. Maybe there is a check where it's checking a directory and before that was fine, and now it causes a problem.

I'll post the patch anyway while I work on the fix.

patch attached.

#1884854: install.core.inc uses the conf dir as $file instead of the $settings_file it should found with the patch from #14 applied.

cross post with myself and testbot

this at least gets to the setup database screen.

maybe make a followup to change the other $return = FALSE; in the for loop to just return FALSE;
there is no point in continuing to loop through other masks if one of them is false already.

and... this one really does have the fix to default.settings.php

Note to self, there might be conflict with #1848490: Import translations automatically during installation

A quick note since I was passing by and saw this issue...

http://php.net/manual/en/function.is-file.php has some notes that you may want to verify before they cause problems...

is_file doesn't recognize files whose filenames contain strange characters like czech ů or russian characters in general.

I've seen many scripts that take it for granted that a path is a directory when it fails is_file($path). When trying to determine whether a path links to a file or a dir, you should always use is_dir after getting false from is_file($path). For cases like described above, both will fail.

And

have noticed that using is_file on windows servers (mainly for development) to use a full path c:\ doesn't always work.

I have had to use
C:/foldertowww/site/file.ext

so I preform an str_replace('\\', '/', $path)
Sometimes I have had the \ instead of / work. (this is using apache2 on XP)

but for sure you cannot have mixed separators.

And least importantly,

Note that is_file() returns false if the parent directory doesn't have +x set for you; this make sense, but other functions such as readdir() don't seem to have this limitation. The end result is that you can loop through a directory's files but is_file() will always fail.

which I take with a pinch of salt - the bit about readdir not being subject to the same limitations; I'd expect them all to obey using file access rights.

Thanks, those are good notes. :)

Since before we were using both. We would probably run into those issues sometimes anyway, so I think we can continue.

Thank you for creating this issue to improve Drupal.

We are working to decide if this task is still relevant to a currently supported version of Drupal. There hasn't been any discussion here for 8+ years which suggests that this has either been implemented or is no longer relevant. Your thoughts on this will allow a decision to be made.

Since we need more information to move forward with this issue, the status is now Postponed (maintainer needs more info). If we don't receive additional information to help with the issue, it may be closed after three months.

Thanks!

Updated IS a bit and made an MR. Tests are running.

Correct the counts in the issue summary.

I'm willing to bet this is random and unrelated

 Json Api Functional (Drupal\Tests\jsonapi\Functional\JsonApiFunctional)
     ✘ Read
       ┐
       ├ Behat\Mink\Exception\ExpectationException: Current response status code is 200, but 503 expected.
       │

Searched DrupalKernel and all instances have been replaced.

Is it correct that this means settings.php is no longer allowed to be a symlink? If so, would be nice to get a change notice for this. I do find that particular setup to be useful for local development, i.e. I have a generic settings.php that sets up e.g. the database name and things like that based on the site name and then (along with the respective webserver config) I can easily "spin up" an additional multi-site by just running mkdir sites/foo; ln -s '../settings.site.php' sites/foo/settings.php; and I'm off to the races. Although slightly less neat, not a big deal to change that setup to copy those files around, but, yeah, would be good to have an actual change notice for that, if that is in fact the case.

If #49 is correct then I think we should roll this back, there are surely numerous hosting setups where settings.php is a symlink and there's no technical reason for this to be disallowed.

Ahh OK, sorry for the noise in that case. No just noticed this issue and apparently I misread the (ancient) discussion above about is_file() vs. file_exists().

The check on is_file for checking the existence of a site directory, when $require_settings = FALSE fails now and always returns sites/default.

is_file returns FALSE for directories, where file_exists returned TRUE.

The check must be:

if (is_file($app_root . '/sites/' . $site_id . '/settings.php') || (!$require_settings && is_dir($app_root . '/sites/' . $site_id))) {

-- edit --
This also results in Drush not finding the correct site directory anymore.