-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[2.0] [HttpKernel] Add check to Store::unlock to ensure file exists #5376
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.
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
Conversation
|
||
if (!is_file($file)) { | ||
return false; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indentation is wrong
return false; | ||
} | ||
|
||
return @unlink($file); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return is_file($file) ? @unlink($file) : false;
Fix indentation and shorten code
@henrikbjorn done and rebased. Thanks. |
@henrikbjorn any news on this one? It's currently not possible to use the HTTP Cache without the first request failing. |
ping @fabpot sorry to keep pushing this, but any chance you could take a look at this? |
This PR was squashed before being merged into the master branch (closes #5381). Commits ------- 0f3126f Added lockExists to Store interface, fixed locking bugs, added tests. Discussion ---------- Added lockExists to Store interface, fixed locking bugs, added tests. While working on Drupal's HttpCache implementation, I discovered that the base HttpCache class does an is_file to check for a lock, which assumes a file-based cache is being used. This seems like a mistake since the rest of the Store interface is easily swappable. I added a lockExists method so that this is properly abstracted. I also noticed there were no tests for the change I made, so I added some very basic locking tests. While adding those I found that the existing lock method is a bit broken. This line here: ```php <?php if (false !== $lock = @fopen($path = $this->getPath($this->getCacheKey($request).'.lck'), 'x')) { ``` will return false if the file couldn't be written for any reason, but the rest of the method assumes that if $lock == false, the lock exists already. So if the file couldnt be written due to the parent directory not existing, $path will be returned as if it exists, which is clearly not the desired behavior. I changed this to return false if the file couldnt be written and doesn't exist, $path if it exists, and true if the lock was created. It still doesn't feel great to have bool|string return values, but that's the best I could come up with atm. I also added a check for the parent directory that creates it if it doesn't exist. The new tests fail without it. I also broke out that code a bit as it was very difficult to read. --------------------------------------------------------------------------- by henrikbjorn at 2012-08-30T09:11:16Z Symfony have a editorconfig file which set the correct indentation settings. http://editorconfig.org/ --------------------------------------------------------------------------- by msonnabaum at 2012-08-30T13:00:20Z Updated based on stof's feedback. --------------------------------------------------------------------------- by msonnabaum at 2012-08-30T13:21:40Z Fixed based on code style feedback. --------------------------------------------------------------------------- by jonathaningram at 2012-09-05T12:29:47Z @msonnabaum, this seems to be distantly related to my recent PR too: #5376. --------------------------------------------------------------------------- by stof at 2012-10-13T20:35:55Z @fabpot anything left to merge this ? --------------------------------------------------------------------------- by catch56 at 2012-10-23T16:42:10Z This looks great to me, Couldn't find anything to complain about.
This PR was merged into the 2.0 branch. Commits ------- a094f7e Add check to Store::unlock to ensure file exists Discussion ---------- [2.0] [HttpKernel] Add check to Store::unlock to ensure file exists Bug fix: yes Feature addition: no Backwards compatibility break: no Symfony2 tests pass: yes I was seeing this error in my logs when using an `AppCache`: ``` Error 2: /var/www/beta.example.com/shared/vendor/symfony/symfony/src/Symfony/Component/HttpKernel/HttpCache/Store.php line 92: unlink(/var/www/beta.example.com/releases/20120827020525/app/cache/beta/http_cache/md/c2/88/66a911b5266a57bdd55131a47895b8861dfd.lck): No such file or directory ``` It was only occurring when the `http_cache` file was being primed (i.e. first load). I've added a simple check to ensure that the file is a valid file before trying to unlink. I also added a missing `@return` docblock. Note: I've chosen to return `false` if the file does not exist as this seems to be the behaviour of the `purge` method. --------------------------------------------------------------------------- by jonathaningram at 2012-08-29T06:46:52Z @henrikbjorn done and rebased. Thanks. --------------------------------------------------------------------------- by jonathaningram at 2012-09-17T22:38:47Z @henrikbjorn any news on this one? It's currently not possible to use the HTTP Cache without the first request failing. --------------------------------------------------------------------------- by jonathaningram at 2012-09-25T01:28:38Z ping @fabpot sorry to keep pushing this, but any chance you could take a look at this?
Bug fix: yes
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
I was seeing this error in my logs when using an
AppCache
:It was only occurring when the
http_cache
file was being primed (i.e. first load).I've added a simple check to ensure that the file is a valid file before trying to unlink. I also added a missing
@return
docblock. Note: I've chosen to returnfalse
if the file does not exist as this seems to be the behaviour of thepurge
method.