Skip to content

[DependencyInjection] Improved the validation of the service names for XML files #18855

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

Closed
wants to merge 1 commit into from

Conversation

marcoalbarelli
Copy link

Q A
Branch? 2.8
Bug fix? no
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets
License MIT
Doc PR reference to the documentation PR, if any

Added a regex constraint for the service id in the xsd

Added a regex constraint for the service id in the xsd
@stof
Copy link
Member

stof commented May 24, 2016

what is the reasonning for this regex ?

Btw, I'm quite sure this forbids some values which are accepted today.

@javiereguiluz
Copy link
Member

@marcoalbarelli improving the XML validation is always a good thing, but I'd like to ask you to please provide a link to the code that restricts the service names to have these chars. I remember a recent discussion (https://github.com/symfony/symfony/pull/18028/files) where we tried to restrict the list of allowed characters but at the end we decided to not change anything.

@javiereguiluz javiereguiluz changed the title [DependencyInjection] [DependencyInjection] Improved the validation of the service names May 24, 2016
@javiereguiluz javiereguiluz changed the title [DependencyInjection] Improved the validation of the service names [DependencyInjection] Improved the validation of the service names for XML files May 24, 2016
@marcoalbarelli
Copy link
Author

@stof the idea behind this is to enforce the service id naming convention at the xsd level.
I'm quite positive that the regex should accept all that is valid for the convention.
Of course if there is something that's left out I can tune the regex.

@stof
Copy link
Member

stof commented May 24, 2016

#18028 was deprecating some chars which prevented dumping the container to an optimized PHP file (and so making such ids unusable in the context of the fullstack framework which performs such dumping). And we indeed removed such limitation in the dumper.

In Symfony 3.1+, any PHP string is allowed as service id (including weird ids, like the empty string or emojis)

@marcoalbarelli
Copy link
Author

@javiereguiluz I proposed this PR because it would be very convenient to have IDE perform the validation of service ids on its own.
The issue popped up today in a partially afferent problem, regarding the aliasing of ConstraintValidators mapped as services

I can expand on this but it's quite a long story :)

@stof
Copy link
Member

stof commented May 24, 2016

@marcoalbarelli XML performing a broken validation is way worse than the current state, as it breaks existing projects.

@marcoalbarelli
Copy link
Author

marcoalbarelli commented May 24, 2016

@stof define broken :)
That regexp should be the equivalent of the service ids naming convention
But I get your point, plus if in 3.1+ service ids will accept any character I guess this PR has no real meaning

@stof
Copy link
Member

stof commented May 24, 2016

@marcoalbarelli a convention for the naming does not mean that using something else is forbidden. It just means that it is recommended to use such naming. Projects are free to use something else than the recommended naming convention even in 2.x versions (as long as they stick to chars compatible with the PhpDumper or avoid dumping their container).
Your PR forbids using valid ids in the XML (note that using the same id is still possible in YAML in your PR, making the behavior even worse as you forbid things only in some places)

@marcoalbarelli
Copy link
Author

In any case I think #18167 makes this PR useless if not harmful
Closing

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.

4 participants