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

Skip non-mapped typed properties with default values when using Proxies #2399

Open
wants to merge 2 commits into
base: 2.10.x
Choose a base branch
from

Conversation

franmomu
Copy link
Contributor

@franmomu franmomu commented Dec 26, 2021

Q A
Type bug
BC Break yes
Fixed issues #2349

Summary

Taking a look at #2349 seems like the problem is that the proxy is not initialized when it's managed.

Looking at the code, if the document is an instance of GhostObjectInterface and it not initialized, it calls to $document->setProxyInitializer(null); which removes the initialization.

To be honest I'm not sure if it's the right fix, maybe that code was there for some reason.

The test is from https://github.com/andythorne/doctrine-odm-example-error

Update: I was taking a look again to this issue, apparently one way to solve it is to skip those properties when using a proxy.

@@ -2839,7 +2839,7 @@ public function getOrCreateDocument(string $className, array $data, array &$hint
$document = $this->identityMap[$class->name][$serializedId];
$oid = spl_object_hash($document);
if ($document instanceof GhostObjectInterface && ! $document->isProxyInitialized()) {
$document->setProxyInitializer(null);
$document->initializeProxy();
Copy link
Member

Choose a reason for hiding this comment

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

This seems to do the trick, but we shouldn't initialize the object at this point as this will lead to a db query while we already have data waiting for hydration. The correct fix would be to somehow initialize properties not managed by the ODM

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @malarzm! that makes sense, I'll try to take a look how the ORM does this if I can. In the meantime I leave the test here just in case someone wants to give it a try

@franmomu franmomu force-pushed the fix_proxy_initializer branch 2 times, most recently from 559e747 to 4a1a004 Compare December 26, 2021 19:10
@malarzm malarzm modified the milestones: 2.3.1, 2.3.2 Dec 28, 2021
@franmomu franmomu force-pushed the fix_proxy_initializer branch 4 times, most recently from 47540d6 to b13ead6 Compare March 3, 2022 10:56
@franmomu
Copy link
Contributor Author

franmomu commented Mar 3, 2022

I was taking a look again at this issue and one possible solution would be to skip these properties.

I've skipped the typed non-mapped properties that have a default value. What about this? maybe is there something else to take into account? 🤔

@franmomu franmomu requested a review from malarzm March 3, 2022 11:23
@franmomu franmomu changed the title Initialize proxy when document is not initialized Skip non-mapped typed properties with default values when using Proxies Mar 4, 2022
@malarzm malarzm modified the milestones: 2.3.2, 2.3.3 Mar 8, 2022
@malarzm malarzm modified the milestones: 2.3.3, 2.3.4 Apr 20, 2022
@malarzm malarzm modified the milestones: 2.4.1, 2.4.2, 2.4.3 May 10, 2022
@malarzm malarzm modified the milestones: 2.4.3, 2.4.4 Dec 20, 2022
@carlos-granados
Copy link

@malarzm any chance that this might be merged? We are experiencing this issue with Doctrine 2.3 and were hoping to see it solved when we upgrade to 2.4

@alcaeus
Copy link
Member

alcaeus commented Mar 17, 2023

2.4 is already released. I'll take a look at this after the weekend for inclusion in a 2.5 patch release.

@alcaeus alcaeus changed the base branch from 2.3.x to 2.5.x March 22, 2023 12:06
@alcaeus alcaeus modified the milestones: 2.4.4, 2.5.1 Mar 22, 2023
@alcaeus
Copy link
Member

alcaeus commented Mar 22, 2023

I made some adjustments to the tests to no longer rely on the absence of an error (and instead running an assertion), as well as applying the logic to all unmapped properties, not only those with default values.

$customers = $this->dm->getRepository(GH2349Customer::class)->findAll();

foreach ($customers as $customer) {
self::assertSame([], $customer->getEvents()); // This would trigger an error if unmapped properties are unset
Copy link
Member

Choose a reason for hiding this comment

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

This test seems fragile:

  • we don't check whether $customer is indeed a proxy
  • and is it still uninitialized (the test would still pass)

Maybe a db query count would be also good to assert so we never get into situation like the one found in original PR?

Copy link
Member

Choose a reason for hiding this comment

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

Also i believe this would be better placed in a generic proxy test, where we don't even need database operations. Just creating a proxy and asserting whether unmapped stuff can be used without initialization - this should be easier to maintain in future

@@ -130,6 +132,19 @@ private function createInitializer(
* @return array<int, string>
*/
private function skippedFieldsFqns(ClassMetadata $metadata): array
{
$skippedIdFieldsFqns = $this->getIdFieldsFqns($metadata);
$skippedNonMappedFieldsFqns = $this->getUnmappedPropertyFqns($metadata);
Copy link
Member

Choose a reason for hiding this comment

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

This looks more expensive than the original identifier exclusion and is called each and every time we create a proxy object, pretty much a hot path. We should memoize the result of skippedFieldsFqns per $metadata

Copy link
Member

Choose a reason for hiding this comment

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

Or put into metadata itself?

foreach ($reflectionClass->getProperties() as $property) {
$propertyName = $property->getName();

if ($metadata->hasField($propertyName)) {
Copy link
Member

Choose a reason for hiding this comment

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

Just a note as field with different PHP and DB field always confused me: metadata is operating on PHP's object properties, and DB field name is used only in hydrators/persisters, right? If yes then we're A-OK here

use Documents74\GH2349Customer;
use Documents74\GH2349Order;

/** @requires PHP 7.4 */
Copy link
Member

@malarzm malarzm Mar 22, 2023

Choose a reason for hiding this comment

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

If I'm seeing correctly, 7.4 was already minimal version for ODM 2.5.x :) No harm done given we haven't cleaned this up yet, worth doing before 2.6.x :)

@malarzm malarzm modified the milestones: 2.5.1, 2.5.2 Apr 5, 2023
@malarzm malarzm modified the milestones: 2.5.2, 2.5.3 Apr 13, 2023
@malarzm malarzm modified the milestones: 2.5.3, 2.5.4 Oct 23, 2023
@malarzm malarzm modified the milestones: 2.5.4, 2.5.5 Nov 3, 2023
@alcaeus alcaeus removed this from the 2.5.5 milestone Nov 24, 2023
@alcaeus alcaeus changed the base branch from 2.5.x to 2.10.x September 20, 2024 12:42
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.

Cannot access uninitialized non-nullable property when accessing a non-db field property of a document
4 participants