Skip to content
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

Compatibility with ORM 3 #1722

Merged
merged 1 commit into from
Nov 12, 2023
Merged

Compatibility with ORM 3 #1722

merged 1 commit into from
Nov 12, 2023

Conversation

derrabus
Copy link
Member

@derrabus derrabus commented Nov 5, 2023

This PR adds compatibility with ORM 3. I'm not very proud of the code that I had to write and I'm open to ideas. 🙈

As a side effect, I'm deprecating not enabling the lazy ghost implementation, just as the ORM did. If the app did not specify which proxy implementation should be used, this is what the bundle decides:

  • ORM 2 on PHP < 8.1: Legacy implementation
  • ORM 2 on PHP >= 8.1: Legacy implementation + runtime deprecation
  • ORM 3: Lazy Ghosts

If the app explicitly opts out of lazy ghosts despite having ORM 3 installed, the container won't compile.

@derrabus derrabus added this to the 2.11.0 milestone Nov 5, 2023
use function sprintf;
use function trait_exists;

if (trait_exists(LazyGhostTrait::class)) {
if (trait_exists(LazyGhostTrait::class) && property_exists(EntityRepository::class, '_entityName')) {
Copy link
Member Author

Choose a reason for hiding this comment

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

This basically opts us out of the new LazyServiceEntityRepository for ORM 3. I need a little help to get this to work, @nicolas-grekas.

Copy link
Member

@nicolas-grekas nicolas-grekas Nov 6, 2023

Choose a reason for hiding this comment

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

Ideally, we would get rid of LazyServiceEntityRepository: I introduced it to keep BC with existing protected properties on EntityRepository. But since those are private in 3.0, we should now be able to deal with lazyness by decorating public/protected methods. As a reminder, the purpose of the lazy proxy is to move the $manager = $registry->getManagerForClass($entityClass); call out of the construction step.

Copy link
Member

Choose a reason for hiding this comment

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

This PR still support ORM 2.x though.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've pushed a version with a proxy for EntityRepository.

@derrabus derrabus force-pushed the feature/orm-3 branch 6 times, most recently from 07f448a to 08aee56 Compare November 11, 2023 11:52
@derrabus
Copy link
Member Author

Ugh, Psalm 4 is a mess. :-(

@derrabus derrabus marked this pull request as ready for review November 11, 2023 13:22
$configuration = new Configuration(true);
$configuration->getConfigTreeBuilder();

$this->assertFalse(class_exists('Doctrine\Common\Proxy\AbstractProxyFactory', false));
Copy link
Member Author

Choose a reason for hiding this comment

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

This is a very old test and I'm not sure if we still need it. I cannot make it pass anymore because I need to feature-check if we can allow not enabling the lazy ghost implementation.

@ostrolucky
Copy link
Member

Can we move some of those psalm suppressions inline? We put it to config file only when that doesn't work.

@derrabus
Copy link
Member Author

Can we move some of those psalm suppressions inline? We put it to config file only when that doesn't work.

Done for the MethodSignatureMismatch suppression.

@derrabus derrabus force-pushed the feature/orm-3 branch 5 times, most recently from 6fbd181 to 3e8ca45 Compare November 11, 2023 14:53
Repository/ContainerRepositoryFactory.php Outdated Show resolved Hide resolved
Repository/RepositoryFactoryCompatibility.php Show resolved Hide resolved
*/
class ServiceEntityRepository extends LazyServiceEntityRepository
{
if (property_exists(EntityRepository::class, '_entityName')) {
Copy link
Member

Choose a reason for hiding this comment

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

Oh boy, people were already complaining about previous logic being too difficult for SA tools to understand (#1677), now it's going to get worse. But I guess we can't do anything about it

Copy link
Member Author

Choose a reason for hiding this comment

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

Not without breaking stuff, I'm afraid. It might be worth doing a 3.0 soon'ish.

Repository/ServiceEntityRepositoryProxy.php Outdated Show resolved Hide resolved
@derrabus derrabus merged commit 7fd7671 into doctrine:2.11.x Nov 12, 2023
15 checks passed
@derrabus derrabus deleted the feature/orm-3 branch November 12, 2023 17:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants