Improved FileResource and DirectoryResource #17598
Closed
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
Member
javiereguiluz
commented
Jan 29, 2016
| Q | A |
|---|---|
| Bug fix? | no |
| New feature? | yes |
| BC breaks? | yes? |
| Deprecations? | no |
| Tests pass? | yes |
| Fixed tickets | #15946 |
| License | MIT |
| Doc PR | - |
| */ | ||
| public function __construct($resource) | ||
| { | ||
| $this->resource = realpath($resource); | ||
|
|
||
| if (!file_exists($this->resource)) { |
There was a problem hiding this comment.
realpath already returns false if it doesn't exist. so this is useless.
Status: Needs Work |
|
@Tobion I've fixed everything you said. Thanks for the review! |
| */ | ||
| public function __construct($resource) | ||
| { | ||
| $this->resource = realpath($resource); | ||
|
|
||
| if (false === $this->resource) { | ||
| throw new \InvalidArgumentException(sprintf('The "%s" file does not exist.', $resource)); |
There was a problem hiding this comment.
usually you write it the other way round: The file "%s" does not exist. (same for directory)
There was a problem hiding this comment.
You are right. Changed. Thanks.
|
FileResource::isFresh is not safe against race conditions (when it's deleted between file_exists and filemtime). So it probably need error supression instead with |
|
Should I replace this: public function isFresh($timestamp)
{
if (!file_exists($this->resource)) {
return false;
}
return filemtime($this->resource) <= $timestamp;
}by this? public function isFresh($timestamp)
{
return file_exists($this->resource) && filemtime($this->resource) <= $timestamp;
} |
|
This PR just needs a final boost to finish it. Should I do the change mentioned in my previous comment? Thanks! |
|
@javiereguiluz I say go for it! Personally, I find the one liner more readable. |
b6f5428 to
5097d14
Compare
|
@jakzal OK. Changed. Thanks. |
| } | ||
|
|
||
| return filemtime($this->resource) <= $timestamp; | ||
| return file_exists($this->resource) && filemtime($this->resource) <= $timestamp; |
There was a problem hiding this comment.
I think @Tobion meant: return file_exists($this->resource) && @filemtime($this->resource) <= $timestamp;.
|
Thank you @javiereguiluz. |
| } | ||
|
|
||
| return filemtime($this->resource) <= $timestamp; | ||
| return file_exists($this->resource) && @filemtime($this->resource) <= $timestamp; |
There was a problem hiding this comment.
filemtime returns false on error which means isFresh evaluetes to true in this case (false < timestamp) which is wrong.
This was referenced Feb 3, 2016
nicolas-grekas
added a commit
that referenced
this pull request
Feb 4, 2016
…rce objects (xabbuh) This PR was merged into the 2.3 branch. Discussion ---------- [DependencyInjection][Routing] add files used in FileResource objects | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #17598 (comment) | License | MIT | Doc PR | Starting with Symfony 3.1, the constructor of the `FileResource` class will throw an exception when the passed file does not exist. Commits ------- 4f865c7 add files used in FileResource objects
nicolas-grekas
added a commit
that referenced
this pull request
Feb 4, 2016
This PR was merged into the 2.8 branch. Discussion ---------- [Routing] add files used in FileResource objects | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #17598 (comment) | License | MIT | Doc PR | Starting with Symfony 3.1, the constructor of the `FileResource` class will throw an exception when the passed file does not exist. Commits ------- 73afd0f add files used in FileResource objects
symfony-splitter
pushed a commit
to symfony/routing
that referenced
this pull request
Feb 4, 2016
This PR was merged into the 2.8 branch. Discussion ---------- [Routing] add files used in FileResource objects | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | symfony/symfony#17598 (comment) | License | MIT | Doc PR | Starting with Symfony 3.1, the constructor of the `FileResource` class will throw an exception when the passed file does not exist. Commits ------- 73afd0f add files used in FileResource objects
fabpot
added a commit
that referenced
this pull request
May 23, 2016
This PR was merged into the 3.1 branch. Discussion ---------- [Config] Allow schemed paths in FileResource | Q | A | ------------- | --- | Branch? | 3.1 | Bug fix? | yes | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #17598 | License | MIT | Doc PR | - This is a small new feature fixing a BC break that has been introduced in #17598 on 3.1. It happens that 3.1 is breaking a `phar` app on our side where we end up doing something like `new FileResource('phar://...')`. Ping @xabbuh and @javiereguiluz esp. Commits ------- c73f34d [Config] Allow schemed path in FileResource
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.