Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.6k
[DI] Add ContainerBuilder::fileExists() for checking/tracking resource existence#21408
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
772970b to9328b37CompareAccording to#20189 (comment), it seems the |
| @@ -900,6 +915,7 @@ private function registerTranslatorConfiguration(array $config, ContainerBuilder | |||
| // Register translation resources | |||
| if ($dirs) { | |||
| foreach ($dirs as $dir) { | |||
| $container->addResource(new DirectoryExistenceResource($dir)); | |||
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.
this is wrong. It must be done earlier, because the directories are not added in$dirs if they don't exist, and this is precisely the case where we need the DirectoryExistenceResource
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.
Indeed
| $files['yml'][] = $file; | ||
| $container->addResource(new FileResource($file)); | ||
| } | ||
| if (is_file($file = $dirname.'/Resources/config/validation.xml')) { | ||
| $container->addResource(new FileExistenceResource($file = $dirname.'/Resources/config/validation.xml')); |
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.
We could optimize things by adding it only when the file does not exist (as we already have a FileResource when it exists, and removing the file will also invalidate the FileResource).
Or we should remove the FileResource if the content of the file does not impact the container itself (which may well be the case for this particular case)
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.
These paths being all passed to a definition through a method call, don't they impact it?
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.
well, if the paths are passed, the content of the file does not impact the container definition (it cannot impact it if you never read the content during the container building). So a FileResource is useless. Only FileExistenceResource is needed.
And it avoids invalidating the container cache when adding a new validation constraint while the container does not care about them and so would rebuild exactly the same one
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.
Thanks for the explanation.
@ogizanagi no. If you need to detect the addition of new files inside a directory, you must use DirectoryResource. This is exactly what it does. And IIRC, we used FileExistenceResource to account for cases checking for directory existence in the past (the only case where it would fail is if you replace a file with a directory with the same name, but this is weird) |
ogizanagi commentedJan 25, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
chalasr commentedJan 25, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@stof Sure, FileExistenceResource would work except for the edge case you mentioned. IMHO the distinction is not bad to have, but I'll revert if you think it's not worth it. Btw, I updated the places you're talking about for make them use |
cfc5524 tof901285CompareComments addressed |
| @@ -105,15 +114,16 @@ public function load(array $configs, ContainerBuilder $container) | |||
| // default in the Form and Validator component). If disabled, an identity | |||
| // translator will be used and everything will still work as expected. | |||
| if ($this->isConfigEnabled($container, $config['translator']) || $this->isConfigEnabled($container, $config['form']) || $this->isConfigEnabled($container, $config['validation'])) { | |||
| if (!class_exists('Symfony\Component\Translation\Translator') && $this->isConfigEnabled($container, $config['translator'])) { | |||
| $container->addResource(new ClassExistenceResource(Translator::class)); | |||
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.
Why should we track classes for throwing cases? Isn't the containernot dumped when an exception is thrown? Thus there is nothing to track? (same below)
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're right. Removed
| if (is_dir($dir)) { | ||
| $dirs[] = $dir; | ||
| } else { | ||
| throw new \UnexpectedValueException(sprintf('%s defined in translator.paths does not exist or is not a directory', $dir)); | ||
| } | ||
| } | ||
| if (is_dir($dir = $rootDir.'/Resources/translations')) { | ||
| $container->addResource(new FileExistenceResource($dir = $rootDir.'/Resources/translations')); |
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.
this is already tracked a few lines below
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.
maybe not when the dir is not found in fact - then should be added in an "else" case of the following "if", isn't it?
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.
Indeed, moved in a else
| } | ||
| if (is_file($file = $dirname.'/Resources/config/validation.xml')) { | ||
| $container->addResource(new FileExistenceResource($file = $dirname.'/Resources/config/validation.xml')); |
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.
FileExistence is not enoug when the file is found - FileResource is needed then
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.
Please see#21408 (comment) from@stof
ad9b26f to0026c67Compare| @@ -105,15 +112,15 @@ public function load(array $configs, ContainerBuilder $container) | |||
| // default in the Form and Validator component). If disabled, an identity | |||
| // translator will be used and everything will still work as expected. | |||
| if ($this->isConfigEnabled($container, $config['translator']) || $this->isConfigEnabled($container, $config['form']) || $this->isConfigEnabled($container, $config['validation'])) { | |||
| if (!class_exists('Symfony\Component\Translation\Translator') && $this->isConfigEnabled($container, $config['translator'])) { | |||
| if (!class_exists(Translator::class) && $this->isConfigEnabled($container, $config['translator'])) { | |||
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.
as usual, all these should be reverted: they only create merge conflicts with no practical benefit
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.
Oops, done
0026c67 tof6122eaCompare| if ($this->isConfigEnabled($container, $config['form'])) { | ||
| $this->formConfigEnabled = true; | ||
| $this->registerFormConfiguration($config, $container, $loader); | ||
| $config['validation']['enabled'] = true; | ||
| if (!class_exists('Symfony\Component\Validator\Validation')) { | ||
| if (!class_exists(Validation::class)) { |
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.
all 'Foo\Bar' => Bar::classchanges in this PR should be reverted :)
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.
There was 2 chances :) I'm ok to revert, but fabpot started like this (seehttps://github.com/symfony/symfony/pull/20189/files) and IMHO it's a good opportunity to make the code more readable
nicolas-grekas commentedJan 29, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Status: needs work |
d7ee7d6 to5877987Compare@nicolas-grekas Updated TwigExtension and DoctrineValidationPass, other places do not seem relevant (either they are about Status: needs review |
9b39dca tocfe850bCompare| if ($trackContents) { | ||
| if (is_file($path)) { | ||
| $this->addResource(new FileResource($path)); | ||
| } elseif (is_dir($path)) { |
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.
what but just an "else" here? otherwise, it looks strange to has a potential case were we don't do anything (the implicit else here)
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.
agreed, done
2e4ced3 tocece6d4CompareUpdate TwigExtension
076dec5 to6b556b8CompareThere 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.
👍
| @@ -1006,19 +1001,16 @@ private function getValidatorMappingFiles(ContainerBuilder $container, array &$f | |||
| foreach ($container->getParameter('kernel.bundles_metadata') as $bundle) { | |||
| $dirname = $bundle['path']; | |||
| if (is_file($file = $dirname.'/Resources/config/validation.yml')) { | |||
| if ($container->fileExists($file = $dirname.'/Resources/config/validation.yml', false)) { | |||
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.
Why did you passfalse here (same for similar cases below)?
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.
These files do not impact the container (see#21408 (comment))
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.
Ah yeah, just found the hidden discussion too.
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.
Maybe we should open a PR for older branches to make use of theFileExistenceResource there.
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.
👍
Thank you@chalasr. |
…racking resource existence (chalasr)This PR was merged into the 3.3-dev branch.Discussion----------[DI] Add ContainerBuilder::fileExists() for checking/tracking resource existence| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | yes| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#20189,#20189 (comment)| License | MIT| Doc PR | n/a~~Finishes#20189 Adds a convenient `ContainerBuilder::fileExists()` method as suggested by Nicolas and use it to track resources in the FrameworkExtension, adding some missing ones.Commits-------6b556b8 [DI] Add ContainerBuilder::fileExists()
| * | ||
| * @final | ||
| */ | ||
| public function fileExists($path, $trackContents = true) |
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 know I'm late to the party, but this name doesn't suggest any side effects. The method actually adds resources and could be better named to reflect this fact.
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.
@jakzal This follows up#21452 (comment) which gives the arguments against the need for reflecting the tracking effect in the method name.
Uh oh!
There was an error while loading.Please reload this page.
Finishes#20189Adds a convenientContainerBuilder::fileExists()method as suggested by Nicolas and use it to track resources in the FrameworkExtension, adding some missing ones.