[DependencyInjection] Prevent service:method factory notation in PHP config #24732
Conversation
|
Should I move the string splitting logic to the |
|
See Im not sure why we dont do this directly in Otherwise perhaps go with |
|
I know I suggested this one, but I've mixed feelings about it :) |
|
I'd also prefer the exception suggested by @nicolas-grekas instead of this. |
|
You know, I would also agree :) |
|
@vudaltsov let's throw here then instead? |
c83ae1f to
81ead82
Compare
|
@nicolas-grekas, done |
| @@ -22,6 +24,10 @@ | |||
| */ | |||
| final public function factory($factory) | |||
| { | |||
| if (is_string($factory) && 1 === substr_count($factory, ':')) { | |||
| throw new InvalidArgumentException('The short factory notation is not supported by the PHP-based DI configuration.'); | |||
There was a problem hiding this comment.
In addition, it should also suggest to use [ref('service'), 'method'] instead.
There was a problem hiding this comment.
I am also considering to replace short factory notation with service:method notation. WDYT
There was a problem hiding this comment.
So the final exception will look like The 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.
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.
Static is still allowed using factory('class::method')? Or should that notation also be explicit; thus factory('class', 'method').
There was a problem hiding this comment.
@nicolas-grekas, done
be6fa70 to
49fc677
Compare
|
@nicolas-grekas, squashed, reworded and rebased |
|
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.