[Config] Fix resource tracking with new GlobResource #22621
Conversation
| */ | ||
| public function __toString() | ||
| { | ||
| return 'glob.'.$this->prefix.$this->pattern.(int) $this->recursive; |
There was a problem hiding this comment.
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.
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.
| $ret[] = $this->doImport($resource, $type, $ignoreErrors, $sourceResource); | ||
| } else { | ||
| foreach ($this->glob($resource, false, $_, $ignoreErrors) as $path => $info) { |
There was a problem hiding this comment.
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.
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.
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.
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.
Would you be able to give it a try after this PR is merged?
| @@ -249,6 +250,10 @@ public function addResource(ResourceInterface $resource) | |||
| return $this; | |||
| } | |||
|
|
|||
| if ($resource instanceof GlobResource && $this->inVendors($resource->getPrefix())) { | |||
There was a problem hiding this comment.
instead of having a special handling of this resource class here (while others are not filtered), should be have ContainerBuilder::glob() like other methods?
There was a problem hiding this comment.
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).
| if (null === $prefixLen) { | ||
| $prefixLen = strlen($prefix); | ||
| $prefixLen = strlen($resource->getPrefix()); |
There was a problem hiding this comment.
broken if $resource gets set to an array of è FileExistenceResource` instances
There was a problem hiding this comment.
which is not possible inside the foreach (the collection is returned only when the iterator is empty).
|
PR ready, comments addressed :) |
|
I just tested: this DOES fix the underlying issue. PhpStorm is one of the editors that updates the modified time of the directory on each file save (probably due to a tmp file). With this fix, it's not always rebuilding the cache whenever I make any change. Vim is another editor that this affects. |
|
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
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.