Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Config] Fix resource tracking with new GlobResource#22621
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| */ | ||
| publicfunction__toString() | ||
| { | ||
| return'glob.'.$this->prefix.$this->pattern.(int)$this->recursive; |
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.
you should add a delimiter between these, to avoid having similar identifiers for different prefixes and patterns (using a delimiter unlikely to appear in a glob)
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.
In fact, that's on purpose, two resources with the same concat for (prefix, pattern) should be considered the same, because they'll end up listing the same files.
| if (!is_string($resource) ||false ===strpbrk($resource,'*?{[')) { | ||
| $ret[] =$this->doImport($resource,$type,$ignoreErrors,$sourceResource); | ||
| }else { | ||
| foreach ($this->glob($resource,false,$_,$ignoreErrors)as$path =>$info) { |
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.
the fact that we ignore the resource here means that we may miss some cases for reloading the container when using a glob import.
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.
Yes, that's how things are now: the Config component doesn't know anything about the container so this cannot trivially be otherwise. I guess $sourceResource is here for that purpose. Not something this PR tries to improve.
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.
But if the import is done by a DI loader extending this abstract class, it should be given a chance to register the resource somewhere (the child class in the component knows what it wants to do with the resource)
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.
and I know this is not what this PR tries to improve. But I detected it while reviewing it. And this looks like one of the cases that we still need to polish before the stable release including glob support.
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.
Would you be able to give it a try after this PR is merged?
| return$this; | ||
| } | ||
| if ($resourceinstanceof GlobResource &&$this->inVendors($resource->getPrefix())) { |
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.
instead of having a special handling of this resource class here (while others are not filtered), should be haveContainerBuilder::glob() like other methods?
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.
Constructing a GlobResource is done after some non trivial logic in FileLoader so I'm not sure we want to expose helper for that to me. This special handling is fine to me (no BC break by definition, and aligned with the purpose of resource tracking for the container).
| foreach ($this->glob($pattern,true,$resource)as$path =>$info) { | ||
| if (null ===$prefixLen) { | ||
| $prefixLen =strlen($prefix); | ||
| $prefixLen =strlen($resource->getPrefix()); |
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.
broken if$resource gets set to an array of è FileExistenceResource` instances
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.
which is not possible inside the foreach (the collection is returned only when the iterator is empty).
nicolas-grekas commentedMay 3, 2017
PR ready, comments addressed :) |
weaverryan commentedMay 3, 2017
I just tested: this DOES fix the underlying issue. PhpStorm is one of the editors that updates the modified time of thedirectory on each file save (probably due to a tmp file). With this fix, it's notalways rebuilding the cache whenever I makeany change. Vim is another editor that this affects. |
fabpot commentedMay 3, 2017
Thank you@nicolas-grekas. |
…las-grekas)This PR was merged into the 3.3-dev branch.Discussion----------[Config] Fix resource tracking with new GlobResource| Q | A| ------------- | ---| Branch? | 3.3| Bug fix? | yes| New feature? | yes| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets | -| License | MIT| Doc PR | -Right now, resource tracking is done via tracking of directories mtimes, which means a container is rebuilt each time a file is either removed or added, but not when an existing file is modified.This looks nice on the surface.BUT.Most code editors do create a temporary file when you open your code, thus change the parent dir mtime, thus trigger a container rebuild.When working with PSR-4 loaders, this means each time you just open a file, most of you will trigger a container rebuild.This is bad :(Here is a new GlobResource to fix this issue.Commits-------9190e10 [Config] Fix resource tracking with new GlobResource
Uh oh!
There was an error while loading.Please reload this page.
Right now, resource tracking is done via tracking of directories mtimes, which means a container is rebuilt each time a file is either removed or added, but not when an existing file is modified.
This looks nice on the surface.
BUT.
Most code editors do create a temporary file when you open your code, thus change the parent dir mtime, thus trigger a container rebuild.
When working with PSR-4 loaders, this means each time you just open a file, most of you will trigger a container rebuild.
This is bad :(
Here is a new GlobResource to fix this issue.