Fix load cache #2043
Fix load cache #2043
Conversation
In PHP7 @ do not works and it is resource expensive, switching to if statement.
|
Can you tell us why |
| @@ -46,7 +46,7 @@ public function generateKey($name, $className) | |||
| */ | |||
| public function load($key) | |||
| { | |||
| @include_once $key; | |||
| if (file_exists($key)) include_once $key; | |||
There was a problem hiding this comment.
this is not safe to race conditions. so the @ would need to be kept anyway
There was a problem hiding this comment.
Could you explain me how would a race condition be affected by that if?
I though php include was synchronous.
http://php.net/manual/en/function.include.php:
"(PHP 4, PHP 5, PHP 7)
The include statement includes and evaluates the specified file.
The documentation below also applies to require.
Files are included based on the file path given or, if none is given, the include_path specified. If the file isn't found in the include_path, include will finally check in the calling script's own directory and the current working directory before failing. The include construct will emit a warning if it cannot find a file; this is different behavior from require, which will emit a fatal error.
If a path is defined — whether absolute (starting with a drive letter or \ on Windows, or / on Unix/Linux systems) or relative to the current directory (starting with . or ..) — the include_path will be ignored altogether. For example, if a filename begins with ../, the parser will look in the parent directory to find the requested file.
For more information on how PHP handles including files and the include path, see the documentation for include_path.
When a file is included, the code it contains inherits the variable scope of the line on which the include occurs. Any variables available at that line in the calling file will be available within the called file, from that point forward. However, all functions and classes defined in the included file have the global scope."
There was a problem hiding this comment.
Very simple, another process can remove the file between the file_exists() call and the include_once() one.
There was a problem hiding this comment.
The time lapsus between the check and the include is in the nano seconds range.
If you are able to reproduce that condition even once, tell me, I will stop using my computer.
You can check how the world resolve this:
http://stackoverflow.com/questions/2252577/check-if-an-include-or-require-exists
There was a problem hiding this comment.
You can stop now :)
Take any high-loaded website like symfony.com, and try to clear the cache live. Bam!
Anyway, it looks like your error handler is misbehaving here.
There was a problem hiding this comment.
Ok, so, I'm a javascript developer, I don't have @, so have a good day, I need to go find a if(exists)openfile substitute for all my projects.
Meanwhile I will begin with the safe and wellcoded try{ openfile }catch(e){}, but don't worry about unknown catched errors, they never happens.
|
Be aware of using error control operator in statements before include() like this: This cause, that error reporting level is set to zero also for the included file. So if there are some errors in the included file, they will be not displayed. Error suppression should be avoided if possible as it doesn't just suppress the error that you are trying to stop, but will also suppress errors that you didn't predict would ever occur. This will make debugging a nightmare. It is far better to test for the condition that you know will cause an error before preceding to run the code. This way only the error that you know about will be suppressed and not all future errors associated with that piece of code. |
|
Yes, it works, but @ is captured for exception handler and fill my error logs. |
That's true, but only if you can do it without race conditions, which is not the case here |
| @@ -46,7 +46,7 @@ public function generateKey($name, $className) | |||
| */ | |||
| public function load($key) | |||
| { | |||
| @include_once $key; | |||
| if (file_exists($key)) @include_once $key; | |||
There was a problem hiding this comment.
please use multiline if with braces
There was a problem hiding this comment.
this should've been reported by fabbot (https://github.com/FriendsOfPHP/PHP-CS-Fixer/blob/1.11/Symfony/CS/Tests/Fixer/PSR2/BracesFixerTest.php#L166), I wander how far it is behind on the fixer (or if there is an issue with the fixer).
|
I'm 👍 to merge this as it's consistent with #1866 when auto_reload = false. |
|
Thank you @MaCroTux. |
Similar issue: |
In PHP7 @ do not works and it is resource expensive, switching to if statement.