Fix crash on PHP 8.3 when a file is missing #20358
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #20358 +/- ##
============================================
- Coverage 64.85% 64.44% -0.41%
- Complexity 11445 11572 +127
============================================
Files 431 433 +2
Lines 37208 37599 +391
============================================
+ Hits 24132 24232 +100
- Misses 13076 13367 +291 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
How does the crash looks like? Stacktrace? |
|
Error: |
framework/caching/FileCache.php
Outdated
| @@ -110,7 +110,7 @@ protected function getValue($key) | |||
| { | |||
| $cacheFile = $this->getCacheFile($key); | |||
|
|
|||
| if (@filemtime($cacheFile) > time()) { | |||
| if (file_exists($cacheFile) && @filemtime($cacheFile) > time()) { | |||
There was a problem hiding this comment.
It is a bit weird, cause @ should've suppressed the error. Any idea why it doesn't work?
Introducing file_exists make the operation non-atomic which isn't great if we're talking about cache.
A more universal way would've been something like how it is done in Yii3...
There was a problem hiding this comment.
I looked into the issue a bit deeper. In my Craft CMS setup, I had DEV_MODE=true enabled. In this mode, a PHP Warning interrupts program execution just like a PHP Error. So, the problem with the missing cache file must be happening at a different level.
Could you clarify what exactly makes the cache file operations non-atomic in this case? Yes, there are now two file operations: checking if the file exists and checking its modification time. But isn’t that the whole point of this method? First, verify the file exists, and only if it does, check its modification time. With the double check, we’d simply exit at the first step if the file isn’t there.
There was a problem hiding this comment.
If you first check if file exist, and then read mtime from it, this means that file could be deleted by other process between these two operations. So file_exists doesn't completely protect you from such errors, it only makes them appear much less frequently (at the cost of additional call, which is not necessary in 99.99% setups).
There was a problem hiding this comment.
Hi @XOlegator
I had DEV_MODE=true enabled. In this mode, a PHP Warning interrupts program execution just like a PHP Error
If this is the case, isn't Yii behaving normal here? The method filemtime generates a warning when file does not exist which results in a crash in dev mode. Can you explain where Yii goes wrong?
There was a problem hiding this comment.
You're absolutely right that filemtime() itself behaves as expected by emitting a warning when a file doesn't exist. The core issue isn’t with Yii’s behavior, but rather with error handling in contexts like Craft CMS’s DEV mode, where warnings are treated as fatal exceptions.
Is Yii "Wrong"? Not at all! This is more about ecosystem compatibility. Many modern PHP frameworks/libraries treat warnings as strict-mode errors. The change simply makes Yii more resilient in such environments without sacrificing performance.
|
Warning As per code yii\base\ErrorHandler::handleError So the handleError will show error page at the end for PHP ver >= 8.0.0 for example: |
|
I'm not sure, maybe because my error_level still contain E_WARNING But I tried the simple test on 8.2, it is shown the error |
|
Related #19773 :-) |
|
I’ve updated this PR to address the filemtime() warning issue in a more efficient way. The changes:
Why this matters:
Let me know if you’d like any adjustments! |
|
IMO - setting and restoring error handler for each cached file is a terrible approach. Did you check performance penalty? If you are not OK with extra non-atomic check To provide perfect solution this can hardly be solved at user-code level ie. with file lock .. or introduce new non-atomic command ie. |
|
Can someone explain what specific Craft CMS is doing, that silenced warnings are converted to exceptions? |
|
@brandonkelly would you please point us to the error handing routine of CraftCMS? Thanks. |
This fixes a problem I had after migrating Craft CMS (based on Yii2) to PHP 8.3