Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[DI] Enhance DX by throwing instead of triggering a deprecation notice#22185
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
fabpot commentedMar 27, 2017
👍 |
ro0NL commentedMar 27, 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.
Given the linked issue.. what about adding edit: while reading adding deprecated may sound weird.. but from a per-file perspective it could make sense to deprecate a bunch of services (old namespace?) at once. |
curry684 commentedMar 27, 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.
I'm obviously in favor of the BC break here, as the case is just about too hypothetical to consider at all. It doesn't close#22143 though, just its symptoms. The real issue there is the
I'm surprised to discover thereis a whitelist. I was expecting even |
nicolas-grekas commentedMar 27, 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.
@curry684 yes, there is a whitelist. Many attributes make no sense as defaults. See#21071 for the discussion about it. You missed learning about that because you did not get the exception you were supposed to - and that would have told about it (fixed here.) Allowing factory/whatever in "defaults" is another discussion. So to me, this closes your issue, and you should open another issue as a feat request (yes, I know your issue was about that also, but let's split things now that we learned there are two things, isn't it? :) ) |
stof left a comment
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.
👍
…precation notice (nicolas-grekas)This PR was merged into the 3.3-dev branch.Discussion----------[DI] Enhance DX by throwing instead of triggering a deprecation notice| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | no| BC breaks? | yes - at the config file level, for edge cases| Deprecations? | no| Tests pass? | yes| Fixed tickets |#22143| License | MIT| Doc PR | -Looking at the linked issue, I'm reconsidering our decision to trigger a deprecation notice when one uses `_instanceof` or `_defaults` as a service name. While on the BC side, this is strict - on the DX side, it looks like this opens a trap where people fill fall into.The same occurs to me with named args: instead of silently accepting invalid args as was the case before, let's throw to help DX when people do mistakes.Last change in this PR: the complex logic required to force strings to be given as `$id` args into `Reference` or `Alias` makes no sense to me, especially considering that a `string` type hint on PHP7 will *do* a string cast.Commits-------b07da3d [DI] Enhance DX by throwing instead of triggering a deprecation notice
| continue; | ||
| } | ||
| if ('' ===$key ||'$' !==$key[0]) { | ||
| thrownewInvalidArgumentException(sprintf('Invalid key "%s" found in arguments of method "%s" for service "%s": only integer or $named arguments are allowed.',$key,$method,$this->currentId)); |
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.
Looks like I am running into this now: What do you understand under a named arg? What exact configuration is forbidden by this change?
Is it possible that this disallows something like this:
<argumentkey="string"type="collection"> <argumentkey="other_string">value</argument></argument>
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 issue iskey=string on your top-level argument. Adding a key on an argument only makes sense inside a collection-argument (as it is then the key inside the array) or using a supported way of defining named arguments.
Previously, defining keys for top-level arguments was not detected early, and would be causing weird bugs for some features
Uh oh!
There was an error while loading.Please reload this page.
Looking at the linked issue, I'm reconsidering our decision to trigger a deprecation notice when one uses
_instanceofor_defaultsas a service name. While on the BC side, this is strict - on the DX side, it looks like this opens a trap where people fill fall into.The same occurs to me with named args: instead of silently accepting invalid args as was the case before, let's throw to help DX when people do mistakes.
Last change in this PR: the complex logic required to force strings to be given as
$idargs intoReferenceorAliasmakes no sense to me, especially considering that astringtype hint on PHP7 willdo a string cast.