[DependencyInjection] XmlFileLoader: enforce tags to have a name #16956
Conversation
| Q | A |
|---|---|
| Bug fix? | yes |
| New feature? | no |
| BC breaks? | no |
| Deprecations? | no |
| Tests pass? | yes |
| Fixed tickets | #16908 |
| License | MIT |
| Doc PR |
5ef1f26 to
266f389
Compare
| { | ||
| try { | ||
| $container = new ContainerBuilder(); | ||
| $loader = new XmlFileLoader($container, new FileLocator(self::$fixturesPath.'/xml')); |
There was a problem hiding this comment.
this should not be inside the try...catch. or you could also use @expectedException and @expectedExceptionMessage directly
There was a problem hiding this comment.
We cannot can use it here as we are interested in the message of the wrapped exception.
|
Could you change https://github.com/symfony/symfony/blob/master/src/Symfony/Component/DependencyInjection/Tests/Fixtures/yaml/badtag2.yml#L6 to use because it was confusing me. In theory with a hash structure like in
|
| $loader = new XmlFileLoader($container, new FileLocator(self::$fixturesPath.'/xml')); | ||
| $loader->load('invalid_tag.xml'); | ||
|
|
||
| $this->fail('The XmlFileLoader is expected to throw an exception when the name attribute is missing or a tag.'); |
There was a problem hiding this comment.
... missing or a tag.
Should be ... missiong on a tag.?
|
@Tobion this would be a bad idea, because it is perfectly valid to have multiple tags with the same name on a service |
| $this->fail('The XmlFileLoader is expected to throw an exception when the name attribute is missing or a tag.'); | ||
| } catch (InvalidArgumentException $e) { | ||
| $this->assertInstanceOf('\InvalidArgumentException', $e->getPrevious()); | ||
| $this->assertRegExp("/The attribute 'name' is required but missing/", $e->getPrevious()->getMessage()); |
There was a problem hiding this comment.
this is asserting an error message coming from the PHP xml validation, right ? This may be a bad idea as it makes the code fragile
There was a problem hiding this comment.
@stof Can you think of a reason for the XSD validation to fail?
There was a problem hiding this comment.
Well, your change is precisely about making the XSD validation fail when the attribute is not there. What I mean is that we should probably avoid relying on the exact wording of the error message in the testsuite (new versions of PHP may alter the message)
There was a problem hiding this comment.
Oh indeed, that makes sense.
|
@stof it would not be a bad idea because it would be optional and most of the time you don't have the same tag multiple times. |
|
We should also add a check to ensure that the loading fails when the tag name is present but an empty string. Status: Needs work |
|
I would say no to tags:
foo_tag: { foo: bar }It's a new feature that adds nothing special, and introduces an inconsistency. |
|
|
||
| $this->fail('The XmlFileLoader is expected to throw an exception when the name attribute is missing or a tag.'); | ||
| } catch (InvalidArgumentException $e) { | ||
| $this->assertInstanceOf('\InvalidArgumentException', $e->getPrevious()); |
There was a problem hiding this comment.
I propose to throw the previous exception here, and use a phpunit annotation instead of all the assertions here
266f389 to
9a7167a
Compare
|
Tests are updated and I added some more checks for empty tag names. |
| @@ -185,6 +185,10 @@ private function parseDefinition($id, $service, $file) | |||
| $parameters[$name] = SimpleXMLElement::phpize($value); | |||
| } | |||
|
|
|||
| if ('' === trim($tag['name'])) { | |||
| throw new \InvalidArgumentException(sprintf('The tag name for service "%s" must not be an empty string in %s.', $id, $file)); | |||
There was a problem hiding this comment.
I think this should throw the components InvalidArgumentException
|
Why this strange naming and numbering |
|
@xabbuh Can you finish this one? |
a3c45e3 to
1e78f53
Compare
|
I addressed all comments. Status: Needs review |
| @@ -186,6 +186,10 @@ private function parseDefinition($id, $service, $file) | |||
| $parameters[$name] = SimpleXMLElement::phpize($value); | |||
| } | |||
|
|
|||
| if ('' === trim($tag['name'])) { | |||
There was a problem hiding this comment.
IMO the trim should not be done and is also not tested.
Since we do not trim the tag when it is added, it should also not validate with trimming. Otherwise seems inconsistent.
1e78f53 to
81e7779
Compare
81e7779 to
d86a3a7
Compare
|
👍 |
|
Thank you @xabbuh. |
…a name (xabbuh) This PR was merged into the 2.3 branch. Discussion ---------- [DependencyInjection] XmlFileLoader: enforce tags to have a name | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #16908 | License | MIT | Doc PR | Commits ------- d86a3a7 [DependencyInjection] enforce tags to have a name