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.

Sign up for GitHub

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

Jump to bottom

Leverage LazyGhostTrait when possible #5

Closed
wants to merge 1 commit into from
Closed

Leverage LazyGhostTrait when possible #5

wants to merge 1 commit into from

Conversation

nicolas-grekas
Copy link
Owner

@nicolas-grekas nicolas-grekas commented Sep 20, 2022 •
edited
Loading

This PR replaces Doctrine's proxies by VarExporter's LazyGhostTrait.

It needs doctrine/persistence#307 (merged)

Requires running composer require symfony/var-exporter:^6.2@dev doctrine/persistence:^3.1@dev to enable the new proxies for now.

nicolas-grekas
nicolas-grekas commented Sep 20, 2022
View reviewed changes
lib/Doctrine/ORM/UnitOfWork.php
@@ -2870,23 +2864,25 @@ public function createEntity($className, array $data, &$hints = [])
break;

default:
$normalizedAssociatedId = $this->normalizeIdentifier($targetClass, $associatedId);
Copy link
Owner Author

Choose a reason for hiding this comment

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

Without normalization, proxies are initialized with type-incompatible values for the identifier fields!?!

Copy link

Choose a reason for hiding this comment

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

this looks like a bug in the existing Doctrine implementation too. I suggest submitting this fix against 2.13.x

stof
stof reviewed Sep 20, 2022
View reviewed changes
@stof
Copy link

stof commented Sep 20, 2022

As hinted in the description of the PR, Doctrine relies on the fact that setting a property would initialize a proxy. This forces a round-trip to the DB to fetch data that could be updated directly instead. It be great to figure out if this requirement really makes sense, and ditch it of not.

This requirement is a must-have. Without it, change tracking would be broken as Doctrine would not have the old value initialized to compare it on flush.

@stof
Copy link

stof commented Sep 20, 2022

or at least, it is a must-have with the current implementation of the UnitOfWork.

@nicolas-grekas nicolas-grekas force-pushed the proxy-common-less branch 3 times, most recently from aa1af65 to 6ec5ab9 Compare September 22, 2022 07:56
@nicolas-grekas nicolas-grekas deleted the branch 2.14.x September 26, 2022 12:23
@nicolas-grekas nicolas-grekas changed the base branch from proxy-common-less to 2.14.x September 26, 2022 12:25
@nicolas-grekas nicolas-grekas force-pushed the ve-proxy-2 branch 2 times, most recently from 4feaba3 to 6a8d04a Compare September 27, 2022 16:59
@nicolas-grekas
Copy link
Owner Author

PR updated. Now that LazyGhostTrait initializes also when setting a property, doctrine/persistence#307 is needed, and more importantly it's way easier to integrate with the ORM.

This PR is green locally. Ready for more extensive review.

@beberlei before I open this on the official repo of doctrine/orm, it'd be happy to know if that work is of any interest to Doctrine. I spent many weeks on it so I hope yes, but you're the one that's going to decide in the end :)

/cc @greg0ire, @derrabus and @malarzm also if you can have a look and maybe share your opinion on the PR?

@nicolas-grekas nicolas-grekas force-pushed the ve-proxy-2 branch 3 times, most recently from d76ee6d to 5b5dbc3 Compare October 3, 2022 14:49
@greg0ire
Copy link

greg0ire commented Oct 3, 2022 •
edited
Loading

share your opinion on the PR

The changes in the docs directory are the ones I can understand, and it appears you are lifting a good number of restrictions. Besides, I'm really looking forward to getting rid of doctrine/common, so big 👍 from me 🙂 Regarding the technical side of things, I'll defer to Benjamin and Alexander, as I'm really unfamiliar with the proxy stuff.

stof
stof reviewed Oct 4, 2022
View reviewed changes
lib/Doctrine/ORM/Internal/Hydration/SimpleObjectHydrator.php
@@ -148,10 +148,6 @@ protected function hydrateRowData(array $row, array &$result)
}
}

if (isset($this->_hints[Query::HINT_REFRESH_ENTITY])) {
$this->registerManaged($this->class, $this->_hints[Query::HINT_REFRESH_ENTITY], $data);
Copy link

Choose a reason for hiding this comment

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

why removing this ?

Copy link
Owner Author

Choose a reason for hiding this comment

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

I borrowed this from https://github.com/doctrine/orm/pull/6719/files#diff-c9c1faa5750ed5a13ec97a922d181d5b26a9f74fd79345cef5264725054cef2b and it makes tests pass without breaking any when using Common\Proxy but I must admit I didn't understand this completely yet. Anyone does?

@nicolas-grekas nicolas-grekas force-pushed the ve-proxy-2 branch 2 times, most recently from 86ba407 to 926d8a6 Compare October 19, 2022 16:53
@nicolas-grekas
Copy link
Owner Author

Only one failing test remains: ENABLE_SECOND_LEVEL_CACHE=1 ./vendor/bin/phpunit --filter testCloneProxy

The reason of the failure is that when the cloned proxy is initialized, it registers itself in the manager. The previous proxies were initialized specially when cloning. I'm not sure how to adjust the code for the new proxies.

@nicolas-grekas nicolas-grekas force-pushed the ve-proxy-2 branch 3 times, most recently from cb8fec6 to 760e1c9 Compare October 26, 2022 16:14
@nicolas-grekas nicolas-grekas force-pushed the ve-proxy-2 branch 16 times, most recently from 7c9d7ea to 74d407f Compare October 27, 2022 20:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stof stof stof left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@nicolas-grekas @stof @greg0ire