-
-
Notifications
You must be signed in to change notification settings - Fork 6.9k
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
FileCache: rebuild cache file before touch when different file owner #16120
FileCache: rebuild cache file before touch when different file owner #16120
Conversation
Slamdunk
commented
Apr 18, 2018
•
edited
Loading
edited
Q | A |
---|---|
Is bugfix? | yes |
New feature? | no |
Breaks BC? | no |
Tests pass? | yes |
Fixed issues | #16050 |
- Prove the bug: https://travis-ci.org/yiisoft/yii2/builds/368030555
- Implement fix
framework/caching/FileCache.php
Outdated
// If ownership differs the touch call will fail, so we try to | ||
// rebuild the file from scratch by deleting it first | ||
// https://github.com/yiisoft/yii2/pull/16120 | ||
if (is_file($cacheFile) && fileowner($cacheFile) !== posix_geteuid()) { |
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.
posix_geteuid()
may not be available. For example, it's not available under Windows.
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.
If so this library needs an AppVeyor build.
It would be easy to add a function_exists('posix_geteuid')
here, but I don't like making changes that aren't tested.
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.
Merged AppVeyor. It fails in some tests: https://ci.appveyor.com/project/samdark/yii2/build/dev-1
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.
I think you need to merge with master in order for build to be triggered...
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.
https://ci.appveyor.com/project/samdark/yii2/build/dev-6#L274
Now that I have a failing test, I'll fix the code
7f4a171
to
b8f4b71
Compare
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.
Looks good to me. Please add changelog and we'll merge it.
2eefef2
to
5597f02
Compare