8000 [FrameworkBundle] Validator cache fails with annotations by mpajunen · Pull Request #21102 · symfony/symfony · GitHub
[go: up one dir, main page]

Skip to content

[FrameworkBundle] Validator cache fails with annotations #21102

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 2 commits into from

Conversation

mpajunen
Copy link
Contributor
@mpajunen mpajunen commented Dec 30, 2016
Q A
Branch? 3.2
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? no
Fixed tickets -
License MIT
Doc PR -

Validation cache warmup can fail if the validated classes contain annotations that use constants.

The cause is the autoloader introduced in #20830. Using another autoloader in addition to the default composer one changes the behavior of the annotation reader: DocParser may check for constants in undefined classes. With the new autoloader that check causes a \ReflectionException and caching that class fails.

This can also cause an error in LazyLoadingMetadataFactory: If loading a class fails, value false may remain in the loadedClasses property. If that class is a parent of another validated class, merging the parent constraints will now fail.

This PR currently includes a fix for the second problem: Invalid state in LazyLoadingMetadataFactory is no longer possible. This gets rid of the error, but the cache is still not built correctly as demonstrated by the failing test case. I'm not quite sure how to fix this correctly.

Ping @nicolas-grekas

There are still problems to solve:

@xabbuh
Copy link
Member
xabbuh commented Dec 30, 2016

@mpajunen Does this solve the same (or even more) as #20793?

@mpajunen
Copy link
Contributor Author

(Whoops, didn't think to look for an open PR that far back since our problem only happens with 3.2.1.)

Just by looking at the changes in #20793 it will highly likely solve the error situation since it includes basically the same fix as this PR.

The main problem most likely remains however. The 3.2.1 autoloader will still throw \LogicExceptions for classes with annotations and those classes will still be missing from the cache. I'll try to verify later.

@mpajunen
Copy link
Contributor Author

Created a branch to check #20793 with the new test case from this PR: https://github.com/mpajunen/symfony/tree/validator-warmup-with-20793

Test still fails as expected.

And no, my smaller fix probably does nothing for the earlier issue that #20793 is meant to fix.

@nicolas-grekas
Copy link
Member

Continued in #21243
Thank you @mpajunen!

fabpot added a commit that referenced this pull request Jan 12, 2017
…pter-related cache warmers (nicolas-grekas, mpajunen)

This PR was merged into the 3.2 branch.

Discussion
----------

[FrameworkBundle] Fix class_exists() checks in PhpArrayAdapter-related cache warmers

| Q             | A
| ------------- | ---
| Branch?       | 3.2
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

Follow up of #21102

Commits
-------

e09dccc [FrameworkBundle] Add annotated validator cache test case
c60009e [FrameworkBundle] Fix class_exists() checks in PhpArrayAdapter-related cache warmers
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.

4 participants
0