Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[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

Closed

Conversation

@hason
Copy link
Contributor

QA
Branch?master
Bug fix?yes
New feature?maybe
BC breaks?no
Deprecations?no
Tests pass?yes
Fixed tickets#18188,#18214
LicenseMIT
Doc PR

@nicolas-grekas
Copy link
Member

nicolas-grekas commentedApr 19, 2016
edited
Loading

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
ContributorAuthor

@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
ContributorAuthor

Yes, see#18188

@dunglas
Copy link
Member

dunglas commentedApr 19, 2016
edited
Loading

👎, 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
ContributorAuthor

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

@dunglas
Copy link
Member

Ok I missed that it was inrequire-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
ContributorAuthor

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 commentedApr 19, 2016
edited
Loading

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.
Seehttps://3v4l.org/v0T0n

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

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 commentedApr 19, 2016
edited
Loading

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)?

private$ambiguousServiceTypes =array();
private$reflectionHelper;

publicfunction__construct(ReflectionHelper$reflectionHelper =null)

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
ContributorAuthor

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 commentedApr 20, 2016
edited
Loading

@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
Copy link
ContributorAuthor

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

@nicolas-grekas
Copy link
Member

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
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
ContributorAuthor

@nicolas-grekas@lisachenko This feature is optional and has to by enabled bySymfony\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
ContributorAuthor

Closed in favour of#18600.

@hasonhason closed thisApr 20, 2016
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@hason@nicolas-grekas@dunglas@lisachenko@javiereguiluz@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp