[DI][Yaml] Add the !yaml_file YAML tag to inject parsed YAML in a service #30446

GaryPEGEOT · 2019-03-04T23:27:41Z

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#30386
License	MIT
Doc PR	symfony/symfony-docs#11121

Allow to inject the content of a YAML file to a service:

services:
  App\AwesomeServiceWithYamlInside:
    arguments:
      - !yaml_file '%kernel.root_dir/some/dir/relative/file.yaml'

Or

<service id="my_service" class="App\ServiceWithYamlInside">
    <argument type="yaml_file">%kernel.root_dir%/some/file.yaml</argument>
</service>

The parsed YAML will here be injected as first argument

OskarStark · 2019-03-05T06:03:48Z

I like the idea 💡

OskarStark

Could you provide a testcase with a json file or an invalid yaml file?

OskarStark · 2019-03-05T06:04:33Z

src/Symfony/Component/DependencyInjection/Tests/Fixtures/yaml/services_with_yaml_file_tag.yml

+services:
+  my_awesome_service_with_yaml_inside:
+    arguments:
+      - !yaml_file '%fixture_dir%/test_yaml_file.yaml'


Please add a newline to remove the red icon

OskarStark · 2019-03-05T06:04:40Z

src/Symfony/Component/DependencyInjection/Tests/Fixtures/yaml/test_yaml_file.yaml

@@ -0,0 +1,3 @@
+foo:
+  bar: true
+  baz: 42


Please add a newline to remove the red icon

nicolas-grekas · 2019-03-05T06:08:09Z

src/Symfony/Component/DependencyInjection/Loader/YamlFileLoader.php

@@ -748,6 +748,11 @@ private function resolveServices($value, $file, $isParameter = false)

                return new Reference($id);
            }
+            if ('yaml_file' === $value->getTag()) {
+                $file = $this->container->resolveEnvPlaceholders($argument, true);


We should define a root for relative files. I suggest using the file where the tag is found when it makes sense (when parsing a file), and to throw an exception otherwise.

Also I don't think this should resolve env vars. Env vars are for runtime parameters, loading yaml is compile time.

I am not sure if this makes sense to use it as root where the file is found, but maybe relative from project dir?

The component doesn't know what a project dir is.

Fixed. Tests are failing on Travis for PHP 7.3, but running on Docker it's working for me. Any idea what's going on?

stof

IMO, this should be implemented as a special kind of Argument, that would be available for all configuration formats.

And this also misses defining the YAML file as a resource, to refresh the cached container automatically in dev when you edit the YAML file. This should be fixed, otherwise DX suffers.

stof · 2019-03-06T11:51:31Z

src/Symfony/Component/DependencyInjection/Loader/YamlFileLoader.php

+                    throw new InvalidArgumentException("Unable to locate file \"$argument\". Please provide a path relative to \"$rootDir\" or an absolute path.");
+                }
+
+                return $this->yamlParser->parseFile($file, Yaml::PARSE_CONSTANT);


The fact that the loader itself is parsing the file means that this feature is available only in the YamlFileLoader, not in other formats. That's quite unexpected (all other features can be configured in all formats).

@stof are you suggesting creating a new kind of argument implementing Symfony\Component\DependencyInjection\Argument\ArgumentInterface?

Well in PHP this can be easily done by parsing thanks to the YAML component directly, but xml would need a new argument type too I guess, something like:

<service id="my_awesome_service_with_yaml_argument" class="foo"> <argument type="yaml_file">%kernel_project_dir%/data/some_file.yaml</argument> </service>

Well, using XML, my comment does not make sense anymore, yaml_file is better 👍

stof · 2019-03-06T11:52:10Z

src/Symfony/Component/DependencyInjection/Loader/YamlFileLoader.php

@@ -748,6 +748,16 @@ private function resolveServices($value, $file, $isParameter = false)

                return new Reference($id);
            }
+            if ('yaml_file' === $value->getTag()) {
+                $rootDir = \dirname($file);
+                $file = \is_file($argument) ? $argument : "$rootDir/$argument";


this won't support using %kernel.project_dir%/some_file.yaml, as it won't be resolved yet here.

It was the case before but @nicolas-grekas suggested that we should not resolve parameters (see above)

he suggested not to resolve env placeholders. Resolving parameters is not the same.

HeahDude

I love this feature, thank you Gary!

Since the tag is meant to be used when writing YAML why not just call it !yaml?
There should not be any confusion between passing a string or a path in this context.

HeahDude · 2019-03-10T02:00:31Z

src/Symfony/Component/DependencyInjection/Loader/schema/dic/services/services-1.0.xsd

@@ -209,6 +209,7 @@
    <xsd:attribute name="name" type="xsd:string" />
    <xsd:attribute name="on-invalid" type="invalid_sequence" />
    <xsd:attribute name="tag" type="xsd:string" />
+    <xsd:attribute name="yaml_file" type="xsd:string" />


In XML it should be yaml-file (same below)

OskarStark · 2019-03-11T17:19:35Z

Since the tag is meant to be used when writing YAML why not just call it !yaml?

If yes, please update the PR title accordingly 👍

HeahDude · 2019-03-10T02:32:45Z

src/Symfony/Component/DependencyInjection/Loader/XmlFileLoader.php

+                        throw new \InvalidArgumentException('You need to install the YAML component to parse YAML files.');
+                    }
+
+                    $filePath = $this->container->getParameterBag()->resolveValue((string) $arg->nodeValue);


if (!\is_file($filePath = $this->container->getParameterBag()->resolveValue((string) $arg->nodeValue))) { $rootDir = \dirname($file); if (!\is_file($filePath = "$rootDir/$filePath")) { throw new InvalidArgumentException("Unable to locate file \"{$arg->nodeValue}\". Please provide a path relative to \"$rootDir\" or an absolute path."); } }

Shorter indeed

HeahDude · 2019-03-22T19:37:35Z

src/Symfony/Component/DependencyInjection/Loader/XmlFileLoader.php

+
+                    $this->container->addResource(new FileResource($filePath));
+
+                    $arguments[$key] = Yaml::parseFile($filePath, Yaml::PARSE_CONSTANT);


What about

$arguments[$key] = $this->container->getParameterBag()->resolveValue( Yaml::parseFile($filePath, Yaml::PARSE_CONSTANT) );

?
That would allow using nested parameters.

not needed. Parameters in arrays are already resolved, and at a better time (late enough that parameter overrides are already applied)

stof · 2019-03-28T16:34:08Z

This should also be supported when using the PHP configurators (and saying that they can resolve the file path and parse it manually is not a good argument, as they would also have to track the resources, etc...)

GaryPEGEOT · 2019-05-07T13:31:44Z

This should also be supported when using the PHP configurators (and saying that they can resolve the file path and parse it manually is not a good argument, as they would also have to track the resources, etc...)

@stof are you suggesting to create a nezw argument impleting Symfony\Component\DependencyInjection\Argument\ArgumentInterface, or do you have something else in mind?

nicolas-grekas · 2019-05-24T11:09:12Z

create a nezw argument impleting Symfony\Component\DependencyInjection\Argument\ArgumentInterface

yes, this should be the way to go - and these should be resolved by an early compiler pass

nicolas-grekas · 2019-05-24T13:34:01Z

We discussed this with @GaryPEGEOT on Slack, and the conclusion is that we should save ourselves from this. 👎 from me.

The reasoning is that this serves no use case that is not covered already by either:

a parameter
an import statement
an edit to the Kernel of the app to load a specific file
a mix of the 3 above

But the added complexity is real (code+doc+teaching+maintaince+etc)

carsonbot added Status: Needs Review Feature labels Mar 4, 2019

GaryPEGEOT changed the title ~~Add the !yaml_file YAML custom tag~~ [DI][Yaml] Add the !yaml_file YAML custom tag Mar 4, 2019

GaryPEGEOT changed the title ~~[DI][Yaml] Add the !yaml_file YAML custom tag~~ [DI][Yaml] Add the !yaml_file YAML tag to inject parsed YAML in a service Mar 4, 2019

OskarStark reviewed Mar 5, 2019

View reviewed changes

nicolas-grekas reviewed Mar 5, 2019

View reviewed changes

carsonbot removed the Status: Needs Review label Mar 6, 2019

stof requested changes Mar 6, 2019

View reviewed changes

carsonbot added the Status: Needs Work label Mar 6, 2019

nicolas-grekas added this to the next milestone Mar 7, 2019

HeahDude reviewed Mar 9, 2019

View reviewed changes

GaryPEGEOT mentioned this pull request Mar 10, 2019

Document the yaml_file usage symfony/symfony-docs#11121

Closed

HeahDude reviewed Mar 10, 2019

View reviewed changes

HeahDude added a commit to HeahDude/symfony that referenced this pull request Mar 22, 2019

[DI] handle yaml file with configurator, ref symfony#30446

4ce2007

HeahDude reviewed Mar 22, 2019

View reviewed changes

GaryPEGEOT added 10 commits March 23, 2019 19:03

Add the !yaml_file YAML custom tag

15b9fb4

Add new line at the end of the file

6930973

Drop parameters resolution

952b1db

Drop useless code

aa82afc

Add test case for non-YAML

0c99163

Allow parameters resolution

903eaac

Add YAML file as resource

cc2ac58

Add support for XML

a8af3e6

Rename XML type

bfb4aa9

Allow nested parameters

34dc1e1

GaryPEGEOT force-pushed the feature/yaml-file-tag branch from 871eea1 to 34dc1e1 Compare March 23, 2019 19:04

xabbuh added the DependencyInjection label Apr 27, 2019

GaryPEGEOT closed this May 24, 2019

nicolas-grekas mentioned this pull request May 24, 2019

Parse YAML file and inject it at build time #30386

Closed

nicolas-grekas modified the milestones: next, 4.4 Oct 27, 2019


		$this->container->addResource(new FileResource($filePath));

		$arguments[$key] = Yaml::parseFile($filePath, Yaml::PARSE_CONSTANT);

Uh oh!

[DI][Yaml] Add the !yaml_file YAML tag to inject parsed YAML in a service #30446

[DI][Yaml] Add the !yaml_file YAML tag to inject parsed YAML in a service #30446

Uh oh!

Conversation

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!