Added lockExists to Store interface, fixed locking bugs, added tests. #5381
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| @@ -92,6 +97,11 @@ public function unlock(Request $request) | |||
| return @unlink($this->getPath($this->getCacheKey($request).'.lck')); | |||
| } | |||
|
|
|||
| public function lockExists(Request $request) | |||
|
Symfony have a editorconfig file which set the correct indentation settings. http://editorconfig.org/ |
|
Updated based on stof's feedback. |
| if (false !== $lock = @fopen($path = $this->getPath($this->getCacheKey($request).'.lck'), 'x')) { | ||
| $path = $this->getPath($this->getCacheKey($request).'.lck'); | ||
| if (!is_dir(dirname($path)) && false === @mkdir(dirname($path), 0777, true)) { | ||
| return false; |
|
Fixed based on code style feedback. |
|
@msonnabaum, this seems to be distantly related to my recent PR too: #5376. |
|
@fabpot anything left to merge this ? |
|
This looks great to me, Couldn't find anything to complain about. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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:
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.