-
-
Notifications
You must be signed in to change notification settings - Fork 9.7k
[DependencyInjection][Routing] Deprecate XML configuration format #60568
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
base: 7.4
Are you sure you want to change the base?
Conversation
2169ea4
to
c5c98cb
Compare
c5c98cb
to
e7849d9
Compare
f21defc
to
d725030
Compare
Let's wait on the discussion on the issue to know what should actually be deprecated. This PR deprecates a lot more than what was mentioned in the issue it fixes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀
Some test cases need some love.
public function testInlineServicesAreNotCandidates() | ||
{ | ||
$this->expectUserDeprecationMessage('Since symfony/dependency-injection 7.4: XML configuration format is deprecated, use YAML or PHP instead.'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't we rewrite this to use another format instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test looks like it’s XML specific: #24491
@@ -32,6 +33,8 @@ | |||
|
|||
class XmlDumperTest extends TestCase |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
side question: do we want to keep the xml dumper?
it's used by some external tools AFAIK, so likely yes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We also have commands loading such XML dumps, like debug:container
. Not sure what we should do about that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See #60597 which might set us free from this concern.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the XML dump of the container is used by every static analysis Symfony Plugin I know, including PHPStan, PhpStorm's Symfony plugin and Psalm. I don't think we can get rid of it (especially considering PhpStorm's plugin isn't written in PHP so probably can't parse the PHP serialized data nicely).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See #61280 to remove the dependency on ext-xml for the XmlDumper
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dumping console data in XML must be kept.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
XmlDescriptor are not affected, so we're good.
src/Symfony/Component/DependencyInjection/Tests/Loader/FileLoaderTest.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/DependencyInjection/Tests/Loader/YamlFileLoaderTest.php
Outdated
Show resolved
Hide resolved
ded957c
to
55ac82e
Compare
703118b
to
8a4b6ca
Compare
278edc5
to
e01cb8e
Compare
c47d5fd
to
072f096
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work! Let's get rid of XML and ext-xml.
I am not really sure we need to go thus far. At least for the Validator component I can tell that there has been nearly zero maintenance work during the last years for the |
Let's split Validator and Serializer in a separate PR? |
Sylius uses XML files for its validator config, as one visible OSS example. |
072f096
to
b374b96
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still 👍
87c2578
to
c225359
Compare
I'm with @xabbuh on this one. I understand that we don't want to require ext-dom anymore and that's great. But if someone likes using XML config files, why shouldn't we allow it optionally if the maintenance overhead is barely measurable? |
ade2761
to
6527895
Compare
Validator and Serializer have been split out, so this PR is now only about routing and DI. From them, deprecating makes sense to me: maintenance is still difficult because of XSD, but the most important is the overhead on the doc and for bundles. When there was no PHP format, XML was OK. But now that we have it, there's no point for writing XML for that anymore. |
6527895
to
aca2986
Compare
( Serializer and Validator can be removed from this PR.) |
I have a hard time following this initiative to be honest. Given the impact on the ecosystem can be quite significant, I think it's a good idea to make a clear high level plan grounded by clear issues with the current system. #60200 has a very clear problem statement: maintaining the FrameworkBundle's XSD file has proofed to be a burden, often forgotten when adding new config options. I very much agree with @stof's comment that this is different for maintaining bundle configuration than maintaining XML loaders for routing, validation, serializer, etc. These formats have proofed to be quite static over the time, requiring less changes to their XSD files and thus less burden from this problem. Further on in the discussion, other problems have been mentioned like XML being a documentation burden and XML special treatment like Last but not least, the alternatives are quite the moving target at the moment. Work on the JSON schema for Yaml has been stalled for some time (#59620). And the PHP config objects might be replaced with array shapes (#58771) some time in the future. I'm not sure it's great to force the ecosystem (mostly talking about the open source bundles and projects like drupal and sulu) to such new and moving formats, instead of relying on the very mature and stable XML format. All in all, I think the timing is a bit off and we must first guarantee the new formats are stable and remove some of the troublesome points before deprecating older formats. |
From #60560 (comment)