Skip to content

[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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from

Conversation

hason
Copy link
Contributor

@hason hason commented Apr 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
Copy link
Member

nicolas-grekas commented Apr 19, 2016

A reflection parser is not required here, and we already know how to get the type of arguments that use non-existing classes:
https://github.com/symfony/symfony/blob/master/src/Symfony/Component/VarDumper/Caster/ReflectionCaster.php#L218-L234

@hason
Copy link
Contributor Author

hason commented Apr 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
Copy link
Member

Do we really need to get information about such classes?

@hason
Copy link
Contributor Author

hason commented Apr 19, 2016

Yes, see #18188

@dunglas
Copy link
Member

dunglas commented Apr 19, 2016

👎, using a static parser will be a disaster from a perf point of view for an edge case (when it is called).
I prefer an error here than using such heavy logic. But there is maybe another way to fix the bug (using an error handler or something like that).

@hason
Copy link
Contributor Author

hason commented Apr 19, 2016

@dunglas The static parser is an optional as same as ocramius/proxy-manager for lazy services. Would you rather let the Symfony throwing fatal errors?

@dunglas
Copy link
Member

dunglas commented Apr 19, 2016

Ok I missed that it was in require-dev...

I've some concerns anyway:

  1. are you sure that there is no lightweight solution (an error handler for instance)?
  2. is it really justified to add (and maintain) this code for an edge case like this one?

@hason
Copy link
Contributor Author

hason commented Apr 19, 2016

I don't know how to catch fatal errors in PHP < 7, register_shutdown_function is not a solution.

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Apr 19, 2016

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 \Error.
See https://3v4l.org/v0T0n

*
* @return \ReflectionClass
*/
public function getReflection($class)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getReflectionClass?

@dunglas
Copy link
Member

dunglas commented Apr 19, 2016

Ok +0 as there is no other solution.

@nicolas-grekas
Copy link
Member

Shouldn't this be applied on 2.8 (I did not check)?

@@ -29,6 +30,12 @@ class AutowirePass implements CompilerPassInterface
private $definedTypes = array();
private $types;
private $ambiguousServiceTypes = array();
private $reflectionHelper;

public function __construct(ReflectionHelper $reflectionHelper = null)
Copy link
Member

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?

Copy link
Contributor Author

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
Copy link
Member

nicolas-grekas commented Apr 20, 2016

@hason see patch proposal here: master...nicolas-grekas:no-parent
But the class loader of GoAOP is not compatible with Symfony's DebugClassLoader, which means this is currently useless in the dev env, where it should be most valuable.

@hason hason force-pushed the static-reflection branch from cd78938 to b106b0c Compare April 20, 2016 08:02
@hason
Copy link
Contributor Author

hason commented Apr 20, 2016

@dunglas @nicolas-grekas What do you mean about a method that toggle static reflection?

@nicolas-grekas
Copy link
Member

Given this covers really an edge case, I'd personally prefer not adding any new public interface for it. That's why and how I proposed the previous suggestion.

@lisachenko
Copy link

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.
In personal, I don't like adding this static reflection library for symfony as dependency, because it's slow and will require a lot of time to process the code for big instances, however, cache warming process can solve this issue.

@hason
Copy link
Contributor Author

hason commented Apr 20, 2016

@nicolas-grekas @lisachenko This feature is optional and has to by enabled by Symfony\Component\DependencyInjection\Util\ReflectionHelper::preferStaticReflection(true);.

@nicolas-grekas
Copy link
Member

Looks like we are allowed to throw in the autoloader, thus work around this fatal error with a throwing autoloader. See #18600

@hason
Copy link
Contributor Author

hason commented Apr 20, 2016

Closed in favour of #18600.

@hason hason closed this Apr 20, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants