Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[DependencyInjection] fix ambiguous services schema #18246

Merged
merged 1 commit into from
Apr 28, 2016

Conversation

backbone87
Copy link
Contributor

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 javiereguiluz changed the title bug #17460 [DI] fix ambiguous services schema [DependencyInjection] fix ambiguous services schema Mar 21, 2016
@javiereguiluz
Copy link
Member

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
Copy link
Member

xabbuh commented Apr 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
Copy link
Member

stof commented Apr 7, 2016

@backbone87 can you add such a test ?

@backbone87
Copy link
Contributor Author

if the XmlFileLoader is validating the loaded xml, then the schema should be covered by its test cases already?

@backbone87
Copy link
Contributor Author

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
Copy link
Contributor Author

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 i have noticed some wacky applications of the whole validation, but i will open another issue for it.

in general the existing testcases cover that this PR is working as intented.

@xabbuh
Copy link
Member

xabbuh commented Apr 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
Copy link
Contributor Author

@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
Copy link
Contributor Author

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
Copy link
Contributor Author

whats missing here? any opinions on the latest comments?

@xabbuh
Copy link
Member

xabbuh commented Apr 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
Copy link
Member

fabpot commented Apr 28, 2016

Thank you @backbone87.

@fabpot fabpot merged commit 9828f23 into symfony:2.3 Apr 28, 2016
fabpot added a commit that referenced this pull request Apr 28, 2016
…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       | MIT

Commits
-------

9828f23 bug #17460 [DI] fix ambiguous services schema
@fabpot fabpot mentioned this pull request Apr 29, 2016
This was referenced May 9, 2016
@backbone87 backbone87 deleted the ticket/17460 branch September 29, 2016 21:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants