[DI] Added safeguards against invalid config in the YamlFileLoader #11374

stof · 2014-07-11T12:00:18Z

Q	A
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#11333
License	MIT
Doc PR	n/a

Exceptions explaining the mistake are better than fatal errors or weird notices appearing when trying to deal with such invalid data.

The XML file loader is not affected by this because the data are validated with the XSD before being processed

romainneutron · 2014-07-11T23:53:14Z

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

        foreach ($content['imports'] as $import) {
+            if (!is_array($import)) {
+                throw new InvalidArgumentException(sprintf('The values in "imports" key should contain be arrays in file "%s"', $file));


typo here should contain be

we could perhaps output also what was given instead to give more hints of what the error is?

Dumping values in the exception message is a bad idea IMO. It can make it totally unreadable

@stof Can you fix the typo?

romainneutron · 2014-07-11T23:54:37Z

Some of the exceptions you added end with a dot, some other not.

weaverryan · 2014-07-13T22:47:10Z

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

@@ -55,6 +55,10 @@ public function load($file, $type = null)

        // parameters
        if (isset($content['parameters'])) {
+            if (!is_array($content['parameters'])) {
+                throw new InvalidArgumentException(sprintf('The "parameters" key should contain an array in file "%s"', $file));


What about adding one additional quick note at the end: Check your YAML syntax.

I think beginners don't think about YAML in terms of the data structure it will compile down to - so I a little extra pointer that they have a syntax issue might help. Great PR btw :).

The messages I added are matching the existing safeguards (some places were guarded in the loader, but not all)

I see! If we like this little addition (i.e. Check your YAML syntax), then why not add it everywhere? :)

stof · 2014-07-15T11:40:59Z

Some of the exceptions you added end with a dot, some other not.

each code throwing exceptions is copy-pasted from similar safeguards already existing in the class. So this means that these inconsistencies already exist in the current exception messages

fabpot · 2014-07-23T14:31:36Z

@stof Can you fixed the typo and add dots everywhere?

stof · 2014-09-02T01:25:37Z

PR updated.

xabbuh · 2014-09-02T06:55:18Z

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

        foreach ($content['imports'] as $import) {
+            if (!is_array($import)) {
+                throw new InvalidArgumentException(sprintf('The values in "imports" key should be arrays in %s. Check your YAML syntax.', $file));


[...] in the "Imports" [...]?

Exceptions explaining the mistake are better than fatal errors or weird notices appearing when trying to deal with such invalid data. Closes symfony#11333

apfelbox · 2014-09-02T07:47:16Z

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

@@ -55,6 +55,10 @@ public function load($file, $type = null)

        // parameters
        if (isset($content['parameters'])) {
+            if (!is_array($content['parameters'])) {
+                throw new InvalidArgumentException(sprintf('The "parameters" key should contain an array in %s. Check your YAML syntax.', $file));


Shouldn't the text be The "parameters" key should be an array in %s.?

Because parameters does not need to contain an array, but just be one? Even the code above says so.

no, the key is not an array. The key is a string

Ah, I actually missed the key part. Still early in the morning. 😉

stof · 2014-09-04T13:00:19Z

ping @symfony/deciders

fabpot · 2014-09-04T17:17:45Z

👍

fabpot · 2014-09-10T13:15:12Z

Thank you @stof.

…leLoader (stof) This PR was merged into the 2.3 branch. Discussion ---------- [DI] Added safeguards against invalid config in the YamlFileLoader | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #11333 | License | MIT | Doc PR | n/a Exceptions explaining the mistake are better than fatal errors or weird notices appearing when trying to deal with such invalid data. The XML file loader is not affected by this because the data are validated with the XSD before being processed Commits ------- 5183501 [DI] Added safeguards against invalid config in the YamlFileLoader

JakubKohout · 2014-09-18T17:41:46Z

This is BC break for this bundle https://github.com/janmarek/autowiring-bundle

If you follow coding style where service name reflects namespace+classname then you don't need to declare class again. It would be nice if you can overpass this check.

e.g.

buzz.message.factory.factory:
    class: Buzz\Message\Factory\Factory

can be simplified into

buzz.message.factory.factory:

stof · 2014-09-18T18:48:40Z

what is the BC break exactly ?

pavelkucera · 2014-09-18T18:54:54Z

Services configuration may look like

services:
    buzz.message.factory.factory:

    some.other.class:

which leads to an InvalidArgumentException as the definition isn't an array when being loaded.

romainneutron reviewed Jul 11, 2014
View reviewed changes

romainneutron added the DependencyInjection label Jul 12, 2014

weaverryan reviewed Jul 13, 2014
View reviewed changes

stof force-pushed the fix_yaml_di_validation branch from 0c2cb00 to 908fe5c Compare September 2, 2014 01:25

xabbuh reviewed Sep 2, 2014
View reviewed changes

[DI] Added safeguards against invalid config in the YamlFileLoader

5183501

Exceptions explaining the mistake are better than fatal errors or weird notices appearing when trying to deal with such invalid data. Closes symfony#11333

stof force-pushed the fix_yaml_di_validation branch from 908fe5c to 5183501 Compare September 2, 2014 07:31

apfelbox reviewed Sep 2, 2014
View reviewed changes

fabpot merged commit 5183501 into symfony:2.3 Sep 10, 2014

stof deleted the fix_yaml_di_validation branch September 10, 2014 13:18

stof mentioned this pull request Sep 19, 2014

Extracted the DI loading of an array from the YamlFileLoader #11954

Closed

Uh oh!

[DI] Added safeguards against invalid config in the YamlFileLoader #11374

[DI] Added safeguards against invalid config in the YamlFileLoader #11374

Uh oh!

Conversation

stof commented Jul 11, 2014

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!

romainneutron commented Jul 11, 2014

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!

stof commented Jul 15, 2014

Uh oh!

fabpot commented Jul 23, 2014

Uh oh!

stof commented Sep 2, 2014

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!

stof commented Sep 4, 2014

Uh oh!

fabpot commented Sep 4, 2014

Uh oh!

fabpot commented Sep 10, 2014

Uh oh!

JakubKohout commented Sep 18, 2014

Uh oh!

stof commented Sep 18, 2014

Uh oh!

pavelkucera commented Sep 18, 2014

Uh oh!

Uh oh!