Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
See discussion in #655190: Allow hook_install_tasks() to be called from a profile's .install file (or document that it can't be) for background.
Also see #752730: Remove file_exists() during bootstrap which is trying to remove some unnecessary file_exists() + require_once() from bootstrap to replace them with just include_once() - that would remove most file_exists() in the critical path altogether.
Comment | File | Size | Author |
---|---|---|---|
#18 | drupal-is_file-1333940-18.patch | 164.22 KB | YesCT |
#18 | interdiff-14-18.txt | 2.73 KB | YesCT |
#14 | drupal-is_file-1333940-14.patch | 163.08 KB | YesCT |
Comments
Comment #1
chx CreditAttribution: chx commentedSo 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.
Comment #2
pingers CreditAttribution: pingers commented"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" :)
Comment #3
chx CreditAttribution: chx commentedOf course but before we embark on some rewrite it needed to be clarified whether it's a journey worth taking or it's futile.
Comment #4
dealancer CreditAttribution: dealancer commentedis_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:
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.
Comment #5
chx CreditAttribution: chx commentedWhat is too slow? file_exists? data to prove?
Comment #6
dealancer CreditAttribution: dealancer commentedsorry, 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:
P.S.
I have XFS as file system, however my friend having results opposite with NTFS.
Comment #7
pingers CreditAttribution: pingers commentedSo, 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...
Comment #8
pingers CreditAttribution: pingers commentedI 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.
Comment #9
sunThere 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
Comment #10
mgiffordSo 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-...
Comment #11
YesCT CreditAttribution: YesCT commented(the slash in the i/o tag breaks the autocomplete from adding new tags)
Comment #12
longwaveIn some cases, is file_exists()/is_file() enough? Should we also consider is_readable()/is_writable()?
Comment #13
pingers CreditAttribution: pingers commentedThat's a separate issue altogether. Create a new issue if you want to for that please.
Comment #14
YesCT CreditAttribution: YesCT commentedfind . -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.
(install gets through requirement checks at this point)
(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.
Comment #16
YesCT CreditAttribution: YesCT commented#1884854: install.core.inc uses the conf dir as $file instead of the $settings_file it should found with the patch from #14 applied.
Comment #17
YesCT CreditAttribution: YesCT commentedcross post with myself and testbot
Comment #18
YesCT CreditAttribution: YesCT commentedthis 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
Comment #20
YesCT CreditAttribution: YesCT commentedNote to self, there might be conflict with #1848490: Import translations automatically during installation
Comment #21
YesCT CreditAttribution: YesCT commentedComment #22
Jeff Veit CreditAttribution: Jeff Veit commentedA 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...
And
And least importantly,
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.
Comment #23
YesCT CreditAttribution: YesCT commentedThanks, 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.