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

franmomu · 2021-12-26T12:03:31Z

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.

malarzm · 2021-12-26T17:06:29Z

lib/Doctrine/ODM/MongoDB/UnitOfWork.php

@@ -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();


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

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 · 2022-03-03T11:02:26Z

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? 🤔

carlos-granados · 2023-03-17T13:12:14Z

@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 · 2023-03-17T17:39:05Z

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

alcaeus · 2023-03-22T12:08:04Z

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.

malarzm · 2023-03-22T18:06:56Z

tests/Doctrine/ODM/MongoDB/Tests/Functional/Ticket/GH2349Test.php

+        $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


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?

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

malarzm · 2023-03-22T18:10:00Z

lib/Doctrine/ODM/MongoDB/Proxy/Factory/StaticProxyFactory.php

@@ -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);


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

Or put into metadata itself?

malarzm · 2023-03-22T18:15:01Z

lib/Doctrine/ODM/MongoDB/Proxy/Factory/StaticProxyFactory.php

+        foreach ($reflectionClass->getProperties() as $property) {
+            $propertyName = $property->getName();
+
+            if ($metadata->hasField($propertyName)) {


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

malarzm · 2023-03-22T18:18:06Z

tests/Doctrine/ODM/MongoDB/Tests/Functional/Ticket/GH2349Test.php

+use Documents74\GH2349Customer;
+use Documents74\GH2349Order;
+
+/** @requires PHP 7.4 */


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 :)

franmomu added the Bug label Dec 26, 2021

franmomu added this to the 2.3.1 milestone Dec 26, 2021

franmomu linked an issue Dec 26, 2021 that may be closed by this pull request

Cannot access uninitialized non-nullable property when accessing a non-db field property of a document #2349

Open

franmomu force-pushed the fix_proxy_initializer branch from adbcac6 to c35fee8 Compare December 26, 2021 12:14

malarzm requested changes Dec 26, 2021

View reviewed changes

franmomu force-pushed the fix_proxy_initializer branch 2 times, most recently from 559e747 to 4a1a004 Compare December 26, 2021 19:10

malarzm modified the milestones: 2.3.1, 2.3.2 Dec 28, 2021

franmomu force-pushed the fix_proxy_initializer branch 4 times, most recently from 47540d6 to b13ead6 Compare March 3, 2022 10:56

franmomu added this to 2.3 (bug fixes only) in Roadmap Mar 3, 2022

franmomu requested a review from malarzm March 3, 2022 11:23

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 modified the milestones: 2.3.2, 2.3.3 Mar 8, 2022

malarzm modified the milestones: 2.3.3, 2.3.4 Apr 20, 2022

malarzm modified the milestones: 2.4.1, 2.4.2, 2.4.3 May 10, 2022

malarzm modified the milestones: 2.4.3, 2.4.4 Dec 20, 2022

alcaeus changed the base branch from 2.3.x to 2.5.x March 22, 2023 12:06

alcaeus modified the milestones: 2.4.4, 2.5.1 Mar 22, 2023

alcaeus force-pushed the fix_proxy_initializer branch from b13ead6 to b4b57e2 Compare March 22, 2023 12:07

franmomu added 2 commits March 22, 2023 13:09

Add a test to check proxy initialization

a1cfe9a

Skip unmapped fields when generating proxies

f6cc0c1

alcaeus force-pushed the fix_proxy_initializer branch from b4b57e2 to f6cc0c1 Compare March 22, 2023 12:09

malarzm reviewed Mar 22, 2023

View reviewed changes

malarzm modified the milestones: 2.5.1, 2.5.2 Apr 5, 2023

malarzm modified the milestones: 2.5.2, 2.5.3 Apr 13, 2023

malarzm modified the milestones: 2.5.3, 2.5.4 Oct 23, 2023

malarzm modified the milestones: 2.5.4, 2.5.5 Nov 3, 2023

alcaeus removed this from the 2.5.5 milestone Nov 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

franmomu commented Dec 26, 2021 •

edited

malarzm Dec 26, 2021

franmomu Dec 26, 2021

franmomu commented Mar 3, 2022

carlos-granados commented Mar 17, 2023

alcaeus commented Mar 17, 2023

alcaeus commented Mar 22, 2023

malarzm Mar 22, 2023

malarzm Mar 22, 2023

malarzm Mar 22, 2023

malarzm Mar 23, 2023

malarzm Mar 22, 2023

malarzm Mar 22, 2023 •

edited

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

Are you sure you want to change the base?

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

Conversation

franmomu commented Dec 26, 2021 • edited

Summary

malarzm Dec 26, 2021

Choose a reason for hiding this comment

franmomu Dec 26, 2021

Choose a reason for hiding this comment

franmomu commented Mar 3, 2022

carlos-granados commented Mar 17, 2023

alcaeus commented Mar 17, 2023

alcaeus commented Mar 22, 2023

malarzm Mar 22, 2023

Choose a reason for hiding this comment

malarzm Mar 22, 2023

Choose a reason for hiding this comment

malarzm Mar 22, 2023

Choose a reason for hiding this comment

malarzm Mar 23, 2023

Choose a reason for hiding this comment

malarzm Mar 22, 2023

Choose a reason for hiding this comment

malarzm Mar 22, 2023 • edited

Choose a reason for hiding this comment

franmomu commented Dec 26, 2021 •

edited

malarzm Mar 22, 2023 •

edited