[DI] Add ContainerBuilder::fileExists() for checking/tracking resource existence #21408

chalasr · 2017-01-25T17:14:46Z

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.

ogizanagi · 2017-01-25T18:02:57Z

According to #20189 (comment), it seems the DirectoryExistenceResource implementation should allow to detect new files in the directory. Not only check if the dir exists (which already worked with the FileExistenceResource but here you ensure it's a dir).

stof · 2017-01-25T18:00:26Z

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php

@@ -900,6 +915,7 @@ private function registerTranslatorConfiguration(array $config, ContainerBuilder
        // Register translation resources
        if ($dirs) {
            foreach ($dirs as $dir) {
+                $container->addResource(new DirectoryExistenceResource($dir));


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

stof · 2017-01-25T18:02:18Z

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php

                $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'));


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)

These paths being all passed to a definition through a method call, don't they impact it?

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

Thanks for the explanation.

stof · 2017-01-25T18:05:34Z

@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 · 2017-01-25T18:12:01Z

@stof : You're right. Thanks for the head up. I misinterpreted @fabpot comment and didn't even realized I was talking about the existing DirectoryResource implementation... -.-'

chalasr · 2017-01-25T18:26:45Z

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)

@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 DirectoryExistenceResource instead.

chalasr · 2017-01-25T20:54:51Z

Comments addressed

nicolas-grekas · 2017-01-29T14:24:33Z

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php

@@ -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));


Why should we track classes for throwing cases? Isn't the container not dumped when an exception is thrown? Thus there is nothing to track? (same below)

I think you're right. Removed

nicolas-grekas · 2017-01-29T14:25:18Z

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php

            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'));


this is already tracked a few lines below

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?

Indeed, moved in a else

nicolas-grekas · 2017-01-29T14:27:55Z

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php

            }

-            if (is_file($file = $dirname.'/Resources/config/validation.xml')) {
+            $container->addResource(new FileExistenceResource($file = $dirname.'/Resources/config/validation.xml'));


FileExistence is not enoug when the file is found - FileResource is needed then

Please see #21408 (comment) from @stof

nicolas-grekas · 2017-01-29T15:08:06Z

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php

@@ -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'])) {


as usual, all these should be reverted: they only create merge conflicts with no practical benefit

nicolas-grekas · 2017-01-29T15:13:23Z

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php

        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)) {


all 'Foo\Bar' => Bar::class changes in this PR should be reverted :)

There was 2 chances :) I'm ok to revert, but fabpot started like this (see https://github.com/symfony/symfony/pull/20189/files) and IMHO it's a good opportunity to make the code more readable

nicolas-grekas · 2017-01-29T15:16:46Z

Note that this PR might be superseded by #21452
So maybe better to submit a PR on my fork if you see anything missing there?
Or keep here only the parts that are not already covered in #21452?

chalasr · 2017-01-30T11:02:03Z

Status: needs work

chalasr · 2017-01-30T12:49:36Z

@nicolas-grekas Updated TwigExtension and DoctrineValidationPass, other places do not seem relevant (either they are about RouteCollection::addResource() or do not look for the resource existence).

Status: needs review

nicolas-grekas · 2017-01-30T12:59:45Z

src/Symfony/Component/DependencyInjection/ContainerBuilder.php

+        if ($trackContents) {
+            if (is_file($path)) {
+                $this->addResource(new FileResource($path));
+            } elseif (is_dir($path)) {


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)

agreed, done

Update TwigExtension

nicolas-grekas

👍

xabbuh · 2017-02-02T10:36:30Z

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php

@@ -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)) {


Why did you pass false here (same for similar cases below)?

These files do not impact the container (see #21408 (comment))

Ah yeah, just found the hidden discussion too.

Maybe we should open a PR for older branches to make use of the FileExistenceResource there.

xabbuh

👍

nicolas-grekas · 2017-02-02T13:11:52Z

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()

jakzal · 2017-02-09T10:00:31Z

src/Symfony/Component/DependencyInjection/ContainerBuilder.php

+     *
+     * @final
+     */
+    public function fileExists($path, $trackContents = true)


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.

@jakzal This follows up #21452 (comment) which gives the arguments against the need for reflecting the tracking effect in the method name.

chalasr changed the title ~~Missing existence resource~~ [Config][FrameworkBundle] Add missing class/directory/file existence resource Jan 25, 2017

chalasr force-pushed the missing-existence-resource branch 2 times, most recently from 772970b to 9328b37 Compare January 25, 2017 18:02

stof reviewed Jan 25, 2017

View reviewed changes

chalasr force-pushed the missing-existence-resource branch 2 times, most recently from cfc5524 to f901285 Compare January 25, 2017 20:40

chalasr changed the title ~~[Config][FrameworkBundle] Add missing class/directory/file existence resource~~ [FrameworkBundle] Add missing class/directory/file existence resource Jan 25, 2017

nicolas-grekas added this to the 3.3 milestone Jan 26, 2017

javiereguiluz added Feature FrameworkBundle labels Jan 26, 2017

chalasr mentioned this pull request Jan 29, 2017

[WIP] added missing ClassExistenceResource checks #20189

Closed

nicolas-grekas reviewed Jan 29, 2017

View reviewed changes

chalasr force-pushed the missing-existence-resource branch 7 times, most recently from ad9b26f to 0026c67 Compare January 29, 2017 14:59

nicolas-grekas reviewed Jan 29, 2017

View reviewed changes

chalasr force-pushed the missing-existence-resource branch from 0026c67 to f6122ea Compare January 29, 2017 15:10

nicolas-grekas reviewed Jan 29, 2017

View reviewed changes

carsonbot added Status: Needs Work and removed Status: Needs Review labels Jan 30, 2017

chalasr force-pushed the missing-existence-resource branch 2 times, most recently from d7ee7d6 to 5877987 Compare January 30, 2017 12:45

carsonbot added Status: Needs Review and removed Status: Needs Work labels Jan 30, 2017

chalasr force-pushed the missing-existence-resource branch 2 times, most recently from 9b39dca to cfe850b Compare January 30, 2017 12:55

nicolas-grekas reviewed Jan 30, 2017

View reviewed changes

chalasr force-pushed the missing-existence-resource branch 4 times, most recently from 2e4ced3 to cece6d4 Compare January 30, 2017 19:58

[DI] Add ContainerBuilder::fileExists()

6b556b8

Update TwigExtension

chalasr force-pushed the missing-existence-resource branch from 076dec5 to 6b556b8 Compare February 2, 2017 10:26

nicolas-grekas approved these changes Feb 2, 2017

View reviewed changes

xabbuh reviewed Feb 2, 2017

View reviewed changes

xabbuh approved these changes Feb 2, 2017

View reviewed changes

stof approved these changes Feb 2, 2017

View reviewed changes

nicolas-grekas merged commit 6b556b8 into symfony:master Feb 2, 2017

chalasr deleted the missing-existence-resource branch February 2, 2017 13:16

nicolas-grekas mentioned this pull request Feb 3, 2017

[DependencyInjection] Use glob pattern to load config files #21270

Merged

jakzal reviewed Feb 9, 2017

View reviewed changes

javiereguiluz mentioned this pull request Feb 12, 2017

Added ContainerBuilder::fileExists() symfony/symfony-docs#7472

Closed

nicolas-grekas mentioned this pull request Feb 14, 2017

[Config] Allow nonexistent file resources #20836

Closed

fabpot mentioned this pull request May 1, 2017

Release v3.3.0-BETA1 #22603

Merged

[DI] Add ContainerBuilder::fileExists() for checking/tracking resource existence #21408

[DI] Add ContainerBuilder::fileExists() for checking/tracking resource existence #21408

Conversation

chalasr commented Jan 25, 2017 • edited Loading

ogizanagi commented Jan 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stof commented Jan 25, 2017

ogizanagi commented Jan 25, 2017 • edited Loading

chalasr commented Jan 25, 2017 • edited Loading

chalasr commented Jan 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas commented Jan 29, 2017 • edited Loading

chalasr commented Jan 30, 2017

chalasr commented Jan 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xabbuh left a comment

Choose a reason for hiding this comment

nicolas-grekas commented Feb 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chalasr commented Jan 25, 2017 •

edited

Loading

ogizanagi commented Jan 25, 2017 •

edited

Loading

chalasr commented Jan 25, 2017 •

edited

Loading

nicolas-grekas commented Jan 29, 2017 •

edited

Loading