Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[DoctrineBridge] Inject the entity manager instead of the class metadata factory in DoctrineExtractor#27829
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
[DoctrineBridge] Inject the entity manager instead of the class metadata factory in DoctrineExtractor#27829
Uh oh!
There was an error while loading.Please reload this page.
Conversation
06c61bb toce4511bCompare| 4.2.0 | ||
| ----- | ||
| * deprecated injection`ClassMetadataFactory` in`DoctrineExtractor`, |
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.
injecting
| ----- | ||
| * deprecated injection`ClassMetadataFactory` in`DoctrineExtractor`, | ||
| and instance of`EntityManagerInterface` must be injected instead |
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.
s/and/an
s/must/should (friendlier)
| publicfunction__construct(ClassMetadataFactory$classMetadataFactory) | ||
| /** | ||
| * @param EntityManagerInterface|ClassMetadataFactory $entityManager |
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 document only the not-deprecated type
| if ($entityManagerinstanceof EntityManagerInterface) { | ||
| $this->entityManager =$entityManager; | ||
| }else { | ||
| @trigger_error(sprintf('Injecting an instance of "%s" in "%s" is deprecated since version 4.2 and will not be possible anymore in 5.0. Inject an instance of "%s" instead.', ClassMetadataFactory::class,__CLASS__, EntityManagerInterface::class),E_USER_DEPRECATED); |
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.
Injecting an instance of "%s" in "%s" is deprecated since Symfony 4.2, inject an instance of "%s" instead.
ce4511b to968d75aCompare| $this->classMetadataFactory =$classMetadataFactory; | ||
| if ($entityManagerinstanceof EntityManagerInterface) { | ||
| $this->entityManager =$entityManager; | ||
| }else { |
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.
elseif ($entityManager instanceof ClassMetadataFactory) and then we should add anelse block that throws anInvalidArgumentException for everything else
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.
We don't check for invalid types when documented in the DockBlock, do we?
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.
For other places where we deprecated arguments and removed the type hint we added them.
xabbuh commentedJul 6, 2018
How will this behave when you have more than one entity manager configured? Right now the entity manager just forwards the call so there is no difference. But is this assumption future proof? |
dunglas commentedJul 6, 2018
There is a different metadata factory for every entity manager, so I don't think it can be an issue. |
| { | ||
| try { | ||
| $metadata =$this->classMetadataFactory->getMetadataFor($class); | ||
| $metadata =$this->entityManager ?$this->entityManager->getClassMetadata($class) :$this->classMetadataFactory->getMetadataFor($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.
what about extracting private method for that?
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.
It doesn't worth it: it will introduce a performance penalty and anyway it will be dropped in Symfony 5.
chalasr commentedJul 9, 2018
UPGRADE files need to be updated |
fabpot left a comment
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.
Can you a note in the UPGRADE file?
968d75a to9e05506Compare| class DoctrineExtractorimplements PropertyListExtractorInterface, PropertyTypeExtractorInterface | ||
| { | ||
| private$entityManager; | ||
| private$classMetadataFactory; |
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 this one have a@deprecated tag?
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 don't think so, it's a private property anyway
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.
It might help at time to clean up deprecated features from the next major branch.
With some well placed@deprecated flags for each deprecated feature, cleaning an entire component can be way easier (justgreping) and free of leftovers (each one leading to "drop dead code" PRs, you know).
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.
that, and the fact it hints to not use the property anymore
…ata factory in DoctrineExtractor
262906b to3aab4a1Comparefabpot commentedJul 13, 2018
Thank you@dunglas. |
…the class metadata factory in DoctrineExtractor (dunglas)This PR was squashed before being merged into the 4.2-dev branch (closes#27829).Discussion----------[DoctrineBridge] Inject the entity manager instead of the class metadata factory in DoctrineExtractor| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? |no <!-- don't forget to update src/**/CHANGELOG.md files -->| BC breaks? | no <!-- seehttps://symfony.com/bc -->| Deprecations? | yes <!-- don't forget to update UPGRADE-*.md and src/**/CHANGELOG.md files -->| Tests pass? | yes <!-- please add some, will be required by reviewers -->| Fixed tickets | n/a <!-- #-prefixed issue number(s), if any -->| License | MIT| Doc PR | n/aAs explained by@stof in#27735 (comment), injecting the `ClassMetadataFactory` directly can lead to issues when resetting the EntityManager.This PR deprecates this usage and encourages to inject the entity manager directly.Commits-------3aab4a1 [DoctrineBridge] Inject the entity manager instead of the class metadata factory in DoctrineExtractor
…class metadata factory (dunglas, javiereguiluz)This PR was merged into the master branch.Discussion----------[PropertyInfo] Inject the entity manager instead of the class metadata factorysymfony/symfony#27829Commits-------44df1a7 Added the missing versionadded directive1f87e0c [PropertyInfo] Inject the entity manager instead of the class metadata factory
Uh oh!
There was an error while loading.Please reload this page.
As explained by@stof in#27735 (comment), injecting the
ClassMetadataFactorydirectly can lead to issues when resetting the EntityManager.This PR deprecates this usage and encourages to inject the entity manager directly.