Improve check on loading cached file if the file not found

[lubosdz]

Q	A
Is bugfix?	❌
New feature?	❌
Improvement?	✔️
Breaks BC?	❌

This fixes annoying errors in debug mode when attempting to load from cache non-existing file (filecache only). Adding simple check whether the file exists fixes the issue.

[samdark]

1. It may result in additional stat call.
2. It becomes non-atomic operation.

[bizley]

Yes, why do we want to avoid it at all costs?

[lubosdz]

@filemtime assumes that file already exists - which is already wrong assumption. Then it solves situation with non-existing file by suppressing error - which is also wrong approach. PHP has dedicated function(s) to check whether the file exists - so why not to use it properly? It is not adding extra stat call since stat data are memory cached on first attempt. The currently implemented mechanism is also not 100% atomic anyway since there is microtime between @filemtime and @fopen and @flock in the block (BTW would not file_get_contents() be better?). Should you wish I can add extra suppress operator @file_exists($path) even though I don't think it'd be necessary.

[samdark]

@lubosdz because there's a time gap between file_exists and filemtime. file_get_contents() with locking flags might be, indeed, better in terms of atomicity.

[rob006]

This fixes annoying errors in debug mode when attempting to load from cache non-existing file (filecache only).

Why do you have these errors in logs? They're suppressed, they should not be reported by PHP.

[lubosdz]

@rob006 It's the debugger setting, which overrides PHP environmental settings. I could configure debugger to ignore any error/warning/level, but I don't wanna do that, since I want to catch off all issues in development.
Anyway guys - the decision is utterly yours. If you don't wanna change it, feel free to close this issue/PR. I just wanted to point out what I consider faulty implementation from the very beginning. Of course, you may have different view. This is the only place in the framework which throws alike error. Perhaps some inspiration could be taken from other frameworks - Laravel also checks if the file exists before retrieving it - no suppression operator in the code. With Symfony I am not familiar.

[rob006]

It's the debugger setting, which overrides PHP environmental settings. I could configure debugger to ignore any error/warning/level, but I don't wanna do that, since I want to catch off all issues in development.

Can you configure debugger to ignore only suppressed errors/warnings? This is default PHP behavior and the whole point of @ operator.

Perhaps some inspiration could be taken from other frameworks - Laravel also checks if the file exists before retrieving it - no suppression operator in the code. With Symfony I am not familiar.

This is not Laravel implementation, this is some random 3-party package.
Symfony checks if file exists and then suppress error for filemtime() call and reading: https://github.com/symfony/cache/blob/01a36b32f930018764bcbde006fbbe421fa6b61e/Traits/FilesystemTrait.php#L80

Suppressing errors here is a must. file_exists() could be added, but it would be practically redundant, since we can't rely on result anyway. So it will be a minor improvement for some debug environments at the cost of minor performance regression affecting everyone.

[lubosdz]

@rob006 As I wrote above - yes, I could configure dbg to silently ignore those errors. But that's not the point here - the motivation is to fix faulty implementation with faulty assumption. The decision is up to core developers - feel free to close issue or leave open for some time if there would be some related feedback .. whatever you do :-)

[rob006]

But that's not the point here - the motivation is to fix faulty implementation with faulty assumption.

What faulty assumption? It is expected that filemtime() may fail (and you can't prevent this), that is why there is a suppression and proper handling of such case.

[lubosdz]

Yeah, your subjective view of what's faulty differs from my view. That's normal. Do whatever you do, I don't wanna waste my time arguing.

[diego-mathis]

#17556

[bizley]

Ah, all in all I understand now and since it was already discussed and agreed upon, I'm closing this PR. I'm afraid @lubosdz that you need to handle that logs on your side.