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] Create an util class to determine services implementing a FQCN#20940
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
| $services =$this->typeHelper->getOfType($type); | ||
| if (1 ===count($services)) { | ||
| return$services[0]; | ||
| }elseif (1 <count($services)) { |
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.
justif here
| try { | ||
| $name = (new \ReflectionClass($class))->name; | ||
| }catch (\ReflectionException$e) { |
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 works fine only if you have a fallback autoloader throwing a ReflectionException as a last resort to avoid the fatal error. As you extracted the logic, you need to handle this requirement in your new service
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 is needed in theAutowirePass because it fetches parameters type hint but this helper doesn't use anything else than the class name, is it still needed?
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 class may extend a non-existent parent class (if the service relies on an optional dependency which is not installed)
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.
You're right, it's updated in98481b9.
| */ | ||
| finalclass ServiceTypeHelper | ||
| { | ||
| privatestatic$classesName; |
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.
classNames would be a better name
| * | ||
| * @param Definition $definition | ||
| * | ||
| * @return string|false |
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 would usestring|null (a nullable string type) instead of this mixed type
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 changed it but I'm not sure that's worth it (I used?: to not change theisset to aarray_key_exists).
stof commentedDec 15, 2016
Does it work fine when the compiler pass registers new autowired services ? The compiler pass currently updates it type map for them, but you removed lots of the logic. |
GuilhemN commentedDec 15, 2016
| $name =false; | ||
| } | ||
| returnself::$classNames[$class] =$name ?:null; |
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 will assign null, not false.
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 I put brackets around it, usefalse or usearray_key_exists?
nicolas-grekas commentedFeb 14, 2017
Generally speaking, types should not be used for wiring things. We added some features recently to make it possible (ie#21530 ) but there is a big difference: there, the type is used on a very local scope only (the config file that uses it). Here, it opens the door to global type-related side effects. |
GuilhemN commentedFeb 14, 2017
@nicolas-grekas this proposal was mostly intended for the DunglasActionBundle but now that you implemented |
Uh oh!
There was an error while loading.Please reload this page.
Reopening of#18300.
This PR creates a new util class: the
ServiceTypeHelper. It extracts some of theAutowirePasslogic usable like this (only during the container compilation):This could be useful in theDunglasActionBundle, to detect when a class is already registered in the container.
Edit: one new advantage of using
autowireTypes: the types map is no longer built if they are sufficient!