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] Add ReflectionHelper for native or static reflection of class#18578
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
hason commentedApr 18, 2016
| Q | A |
|---|---|
| Branch? | master |
| Bug fix? | yes |
| New feature? | maybe |
| BC breaks? | no |
| Deprecations? | no |
| Tests pass? | yes |
| Fixed tickets | #18188,#18214 |
| License | MIT |
| Doc PR |
nicolas-grekas commentedApr 19, 2016 • 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.
A reflection parser is not required here, and we already know how to get the type of arguments that use non-existing classes: |
hason commentedApr 19, 2016
@nicolas-grekas Really? This PR solves the problem with existing classes that depend on non-existing classes. That's a big difference. |
nicolas-grekas commentedApr 19, 2016
Do we really need to get information about such classes? |
hason commentedApr 19, 2016
Yes, see#18188 |
dunglas commentedApr 19, 2016 • 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.
👎, using a static parser will be a disaster from a perf point of view for an edge case (when it is called). |
hason commentedApr 19, 2016
@dunglas The static parser is an optional as same as |
dunglas commentedApr 19, 2016
Ok I missed that it was in I've some concerns anyway:
|
hason commentedApr 19, 2016
I don't know how to catch fatal errors in PHP < 7, |
nicolas-grekas commentedApr 19, 2016 • 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.
So, I played a bit with this corner case, looks like there is no other way around, even in PHP 7 where missing parent classes are hard fatal errors, not |
| * | ||
| * @return \ReflectionClass | ||
| */ | ||
| public function getReflection($class) |
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.
getReflectionClass?
dunglas commentedApr 19, 2016 • 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.
Ok +0 as there is no other solution. |
nicolas-grekas commentedApr 19, 2016
Shouldn't this be applied on 2.8 (I did not check)? |
| private$ambiguousServiceTypes =array(); | ||
| private$reflectionHelper; | ||
| publicfunction__construct(ReflectionHelper$reflectionHelper =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.
Instead of injecting an object, I suggest a string:$reflectionClassClassname = \ReflectionClass::class
Could it be made to work this way?
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.
No, because is neccessary to call__toString method for GoAOP.
nicolas-grekas commentedApr 20, 2016 • 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.
@hason see patch proposal here:master...nicolas-grekas:no-parent |
hason commentedApr 20, 2016
@dunglas@nicolas-grekas What do you mean about amethod that toggle static reflection? |
nicolas-grekas commentedApr 20, 2016
Given this covers really an edge case, I'd personally prefernot adding any new public interface for it. That's why and how I proposed the previous suggestion. |
lisachenko commentedApr 20, 2016
Hello, guys! I noticed that you faced this ugly issue with unavailable parent classes. It's really bad one and sadly can be solved only with static reflection. |
hason commentedApr 20, 2016
@nicolas-grekas@lisachenko This feature is optional and has to by enabled by |
nicolas-grekas commentedApr 20, 2016
Looks like we are allowed to throw in the autoloader, thus work around this fatal error with a throwing autoloader. See#18600 |
hason commentedApr 20, 2016
Closed in favour of#18600. |