Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[DependencyInjection] fix ambiguous services schema#18246
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
backbone87 commentedMar 21, 2016
| Q | A |
|---|---|
| Branch? | 2.3 |
| Bug fix? | yes |
| New feature? | no |
| BC breaks? | no |
| Deprecations? | no |
| Tests pass? | |
| Fixed tickets | #17460#2122 |
| License | MIT |
javiereguiluz commentedApr 1, 2016
This issue seems very important. Could anyone who uses XML config verify it? Thanks! After all, people use XML because it validates the config ... so we must ensure that the validation is correct. |
xabbuh commentedApr 5, 2016
Best imo would be to have additional tests that ensure that an XML file that wasn't valid before now is parsed properly. |
stof commentedApr 7, 2016
@backbone87 can you add such a test ? |
backbone87 commentedApr 9, 2016
if the XmlFileLoader is validating the loaded xml, then the schema should be covered by its test cases already? |
backbone87 commentedApr 9, 2016
actually all the XmlFileLoader tests should fail without this ticket resolved, because the schema is invalid and therefore the xml cant be validated. i am not that familar with the validation behavior of libxml |
backbone87 commentedApr 10, 2016
ok i have played around with the test cases of the XmlFileLoader and it seems like libxml doesnt care about unique particle attribution and therefore "succeeds" validating the services.xml files with the old (invalid) schema. in general the existing testcases cover that this PR is working as intented. |
xabbuh commentedApr 10, 2016
@backbone87 The thing is, if we cannot find an example that would fail without this change, there is no need to do it. So what I meant is that we need to find a failing example and have to add it on top of the already existing ones. |
backbone87 commentedApr 12, 2016
@xabbuh it cant fail, because libxml doesnt seem to care about the problem in the XSD, but every other standards conforming tool reports this XSD as invalid without the fix the "benefit" from this PR is that you can now have your extension config between parameter and service config, which was invalid before: <parameters>...</parameter><project:config>...</project:config><services>...</services> |
backbone87 commentedApr 12, 2016
i mean we could add a XSD linting test to the test suite, but that needs to use another XML tool than the ones bundled with PHP. every decent XSD editor lints automatically, so idk if its worth the hassle. |
backbone87 commentedApr 21, 2016
whats missing here? any opinions on the latest comments? |
xabbuh commentedApr 21, 2016
The change itself looks good to me. 👍 But yeah I think we should use some external validation tool if that doesn't work properly with PHP. |
fabpot commentedApr 28, 2016
Thank you@backbone87. |
…one87)This PR was merged into the 2.3 branch.Discussion----------[DependencyInjection] fix ambiguous services schema| Q | A| ------------- | ---| Branch? | 2.3| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? || Fixed tickets |#17460#2122| License | MITCommits-------9828f23 bug#17460 [DI] fix ambiguous services schema