[DoctrineBridge] Inject the entity manager instead of the class metadata factory in DoctrineExtractor #27829
Conversation
06c61bb to
ce4511b
Compare
| 4.2.0 | ||
| ----- | ||
|
|
||
| * deprecated injection `ClassMetadataFactory` in `DoctrineExtractor`, |
| ----- | ||
|
|
||
| * deprecated injection `ClassMetadataFactory` in `DoctrineExtractor`, | ||
| and instance of `EntityManagerInterface` must be injected instead |
There was a problem hiding this comment.
s/and/an
s/must/should (friendlier)
| private $classMetadataFactory; | ||
|
|
||
| public function __construct(ClassMetadataFactory $classMetadataFactory) | ||
| /** | ||
| * @param EntityManagerInterface|ClassMetadataFactory $entityManager |
There was a problem hiding this comment.
should document only the not-deprecated type
| if ($entityManager instanceof 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.
Injecting an instance of "%s" in "%s" is deprecated since Symfony 4.2, inject an instance of "%s" instead.
ce4511b to
968d75a
Compare
| $this->classMetadataFactory = $classMetadataFactory; | ||
| if ($entityManager instanceof EntityManagerInterface) { | ||
| $this->entityManager = $entityManager; | ||
| } else { |
There was a problem hiding this comment.
elseif ($entityManager instanceof ClassMetadataFactory) and then we should add an else block that throws an InvalidArgumentException for everything else
There was a problem hiding this comment.
We don't check for invalid types when documented in the DockBlock, do we?
There was a problem hiding this comment.
For other places where we deprecated arguments and removed the type hint we added them.
|
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? |
|
There is a different metadata factory for every entity manager, so I don't think it can be an issue. |
| @@ -40,7 +50,7 @@ public function __construct(ClassMetadataFactory $classMetadataFactory) | |||
| public function getProperties($class, array $context = array()) | |||
| { | |||
| try { | |||
| $metadata = $this->classMetadataFactory->getMetadataFor($class); | |||
| $metadata = $this->entityManager ? $this->entityManager->getClassMetadata($class) : $this->classMetadataFactory->getMetadataFor($class); | |||
There was a problem hiding this comment.
what about extracting private method for that?
There was a problem hiding this comment.
It doesn't worth it: it will introduce a performance penalty and anyway it will be dropped in Symfony 5.
|
UPGRADE files need to be updated |
968d75a to
9e05506
Compare
| @@ -27,11 +28,22 @@ | |||
| */ | |||
| class DoctrineExtractor implements PropertyListExtractorInterface, PropertyTypeExtractorInterface | |||
| { | |||
| private $entityManager; | |||
| private $classMetadataFactory; | |||
There was a problem hiding this comment.
should this one have a @deprecated tag?
There was a problem hiding this comment.
I don't think so, it's a private property anyway
There was a problem hiding this comment.
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 (just greping) and free of leftovers (each one leading to "drop dead code" PRs, you know).
There was a problem hiding this comment.
that, and the fact it hints to not use the property anymore
…ata factory in DoctrineExtractor
262906b to
3aab4a1
Compare
|
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 <!-- see https://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/a As 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 factory symfony/symfony#27829 Commits ------- 44df1a7 Added the missing versionadded directive 1f87e0c [PropertyInfo] Inject the entity manager instead of the class metadata factory
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.