Skip to content
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

#46 - applying correct chmod() to generated cache file #49

Merged

Conversation

Ocramius
Copy link
Member

See #46 ( #46 (comment) )

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@Ocramius Ocramius added the bug label Dec 18, 2014
@Ocramius Ocramius added this to the v1.2.3 milestone Dec 18, 2014
@Ocramius Ocramius self-assigned this Dec 18, 2014
@bjoernhaeuser
Copy link
Contributor

Oh shit, never thought about that :-(

@Ocramius
Copy link
Member Author

@bjoernhaeuser no big deal, this happens all the time.

@stof
Copy link
Member

stof commented Dec 19, 2014

👍

@@ -208,6 +210,7 @@ private function saveCacheFile($path, $data)
throw new \RuntimeException(sprintf('Unable to rename %s to %s', $tempfile, $path));
}

@chmod($path, 0666 & ~umask());

Choose a reason for hiding this comment

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

In this way the chmod is done after the rename, making the operation not atomic. Consider moving it to before the rename block.

chmod($tempfile, 0666 & ~umask());
if (false === rename($tempfile, $path)) {
    throw new \RuntimeException(sprintf('Unable to rename %s to %s', $tempfile, $path));
}

Not sure why the @ would be needed, so removed it.

Copy link
Member

Choose a reason for hiding this comment

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

Windows cannot chmod. So if you don't silence the error, things will break on Windows

@nicoschoenmaker
Copy link

👍, encountered the same bug.

@edouardkombo
Copy link

You have an error with your commit !
You unlink a file that doesn't exists because it has been renamed.

This is your code:

if (false === rename($tempfile, $path)) {
    throw new \RuntimeException(sprintf('Unable to rename %s to %s', $tempfile, $path));
}
@unlink($tempfile);
The correct fix would be:

$newTempFile = rename($tempfile, $path);
if (false === $newTempFile) {
    throw new \RuntimeException(sprintf('Unable to rename %s to %s', $tempfile, $path));
}
@unlink($newTempFile);
The error generated with your commit is this:
 [ErrorException]
 unlink(/var/www/headoo/app/cache/dev/annotations/54940ee3e1d910.826327572T4l9C): No such file or directory

@Ocramius
Copy link
Member Author

@edouardkombo in case the rename() operation failed, @unlink() cleans up the temporary file.

Ocramius added a commit that referenced this pull request Dec 20, 2014
…missions

#46 - applying correct `chmod()` to generated cache file
@Ocramius Ocramius merged commit 433906a into doctrine:master Dec 20, 2014
@Ocramius Ocramius deleted the hotfix/#46-file-cache-reader-permissions branch December 20, 2014 20:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants