Skip to content

Fix crash on PHP 8.3 when a file is missing #20358

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix crash on PHP 8.3 when a file is missing #20358

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion framework/caching/FileCache.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ protected function getValue($key)
{
$cacheFile = $this->getCacheFile($key);

if (@filemtime($cacheFile) > time()) {
if (file_exists($cacheFile) && @filemtime($cacheFile) > time()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

$fp = @fopen($cacheFile, 'r');
if ($fp !== false) {
@flock($fp, LOCK_SH);
Expand Down