Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[DependencyInjection] Prevent service:method factory notation in PHP config#24732
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
vudaltsov commentedOct 29, 2017
Should I move the string splitting logic to the |
ro0NL commentedOct 29, 2017
See Im not sure why we dont do this directly in Otherwise perhaps go with |
nicolas-grekas commentedNov 1, 2017
I know I suggested this one, but I've mixed feelings about it :) |
ogizanagi commentedNov 1, 2017
I'd also prefer the exception suggested by@nicolas-grekas instead of this. |
vudaltsov commentedNov 1, 2017
You know, I would also agree :) |
nicolas-grekas commentedNov 12, 2017
@vudaltsov let's throw here then instead? |
c83ae1f to81ead82Comparevudaltsov commentedNov 16, 2017
@nicolas-grekas, done |
| finalpublicfunctionfactory($factory) | ||
| { | ||
| if (is_string($factory) &&1 ===substr_count($factory,':')) { | ||
| thrownewInvalidArgumentException('The short factory notation is not supported by the PHP-based DI configuration.'); |
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.
In addition, it should also suggest to use[ref('service'), 'method'] 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.
I am also considering to replaceshort factory notation withservice:method notation. WDYT
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.
So the final exception will look likeThe service:method factory notation is not supported by the PHP-based DI configuration. Use [ref('service'), 'method'] 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.
LGTM, with double quotes around, and resolved/actual id+method, eg:
Invalid factory "%s:%s": the `service:method` notation is not available when using PHP-based DI configuration. Use "[ref('%s'), '%s']" 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.
Static is still allowed usingfactory('class::method')? Or should that notation also be explicit; thusfactory('class', 'method').
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, done
nicolas-grekas 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.
(could you please squash, and update the commit message meanwhile please)
be6fa70 to49fc677Comparevudaltsov commentedNov 17, 2017
@nicolas-grekas, squashed, reworded and rebased |
nicolas-grekas commentedNov 17, 2017
Thank you@vudaltsov. |
…ion in PHP config (vudaltsov)This PR was merged into the 3.4 branch.Discussion----------[DependencyInjection] Prevent service:method factory notation in PHP config| Q | A| ------------- | ---| Branch? | 3.4| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#24704| License | MIT| Doc PR |Started working on fixing#24704.@nicolas-grekas, am I on the right way? If yes I will look at the tests and try to add this case.Commits-------49fc677 Throw on service:method factory notation in PHP-based DI configuration
Started working on fixing#24704.
@nicolas-grekas, am I on the right way? If yes I will look at the tests and try to add this case.