Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.6k
[DI] Deprecate XML services without ID#22903
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
nicolas-grekas left a comment• edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
👍 makes sense to me
| ++$num; | ||
| } | ||
| if ($num) { | ||
| @trigger_error(sprintf('Anonymous services (%d) in file %s are deprecated since 3.4 and wont be supported in 4.0', $num, $file), E_USER_DEPRECATED); |
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.
imho, no need for $num. To be a bit more explicit, I propose:
Anonymous services are deprecated since Symfony 3.4, the "id" attribute will be required in version 4.0 in %s.
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.
it will allow for missing deprecations more easy that way, but that's more or less related to loose comparison of those.
Updated :)
failures unrelated. |
| @@ -412,6 +412,10 @@ private function processAnonymousServices(\DOMDocument $xml, $file, $defaults) | |||
| // anonymous services "in the wild" | |||
| if (false !== $nodes = $xpath->query('//container:services/container:service[not(@id)]')) { | |||
| if ($nodes->length) { | |||
| @trigger_error(sprintf('Anonymous services are deprecated since Symfony 3.4, the "id" attribute will be required in version 4.0 in %s.', $file), E_USER_DEPRECATED); | |||
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.
@nicolas-grekas worth mentioning this only applies to outlined anon. services (top level, non-inlined... how should we call 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.
Top-level anonymous services... then?
UPGRADE-3.4.md Outdated
| @@ -11,5 +16,5 @@ Finder | |||
| Validator | |||
| --------- | |||
| *not setting the `strict` option of the `Choice` constraint to `true` is | |||
| *Not setting the `strict` option of the `Choice` constraint to `true` is | |||
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.
should IMO be reverted as the sentence does not end with a dot
Do we still have a test which ensures that deprecations are not triggered for nested anonymous definitions? |
ro0NL commentedMay 25, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Yep :)https://github.com/symfony/symfony/pull/22903/files#diff-25d0b562ad1c177bac8f4f9d0247b2acR7 it's tricky though due loose comparison of deprecations; if it triggered we wouldnt catch it (before it did by including the no. of occurences) |
@ro0NL But this one is only tested in a |
| @@ -230,6 +230,17 @@ public function testLoadAnonymousServices() | |||
| $this->assertSame($fooArgs[0], $barArgs[0]); | |||
| } | |||
| /** | |||
| * @group legacy | |||
| * @expectedDeprecation Top-level anonymous services are deprecated since Symfony 3.4, the "id" attribute will be required in version 4.0 in %sservices_without_id.xml. | |||
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.
the%s seems to be a left-over from thesprintf pattern
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.
it includes an absolute path, so this deals with phpunit'sassertStringMatchesFormat
| @@ -412,6 +412,10 @@ private function processAnonymousServices(\DOMDocument $xml, $file, $defaults) | |||
| // anonymous services "in the wild" | |||
| if (false !== $nodes = $xpath->query('//container:services/container:service[not(@id)]')) { | |||
| if ($nodes->length) { | |||
| @trigger_error(sprintf('Top-level anonymous services are deprecated since Symfony 3.4, the "id" attribute will be required in version 4.0 in %s.', $file), E_USER_DEPRECATED); | |||
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.
maybe we should include the class in the deprecation message to make it easier to find the place
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.
currently it triggers one deprecation per file, kinda like that; it goes away after you solved everything. Also class can be empty... not helping :)
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.
updated thanks tohttps://secure.php.net/manual/en/domnode.getlineno.php
@xabbuh extra test added |
Thank you@ro0NL. |
This PR was merged into the 3.4 branch.Discussion----------[DI] Deprecate XML services without ID| Q | A| ------------- | ---| Branch? | 3.4| Bug fix? | no, confusing though| New feature? | yes| BC breaks? | no| Deprecations? | yes| Tests pass? | yes| Fixed tickets | #... <!-- #-prefixed issue number(s), if any -->| License | MIT| Doc PR | symfony/symfony-docs#... <!--highly recommended for new features-->On slack someone had a issue with class named services;> So, probably should have done this sooner, I stepped through with a debugger and it looks like \Symfony\Component\DependencyInjection\Loader\XmlFileLoader::processAnonymousServices assigns a sha256 to services that don't have any IDs> When my manually wired service is registered, it has an ID that looks like 1_344b468f6069ffe8c32092409d99c59abc218f41071ce4c4230c198876129bc0, so it doesn't override the auto-loaded one> I swear I read that IDs default to the class name now...The fix was easy; doing `<service/>` instead of `<service/>`. However the thing is... i made the exact same mistake trying to reproduce 😅I think given the recent developments (dropping type based autowiring and class named services) it makes sense to force XML service to specify an ID attribute (the top level ones). This would be consistent with YAML and PHP as well.Fixing deprecations is also easy, just change `class` attribute to `id` like i've done for the frameworkbundle in this PR.Any thoughts?Commits-------b8c68da [DI] Deprecate XML services without ID
Is it possible to allow unnamed toplevel services, which will be implicitly named after the class name? |
On slack someone had a issue with class named services;
The fix was easy; doing
<service/>instead of<service/>. However the thing is... i made the exact same mistake trying to reproduce 😅I think given the recent developments (dropping type based autowiring and class named services) it makes sense to force XML service to specify an ID attribute (the top level ones). This would be consistent with YAML and PHP as well.
Fixing deprecations is also easy, just change
classattribute toidlike i've done for the frameworkbundle in this PR.Any thoughts?