-
-
Notifications
You must be signed in to change notification settings - Fork 515
Skip native lazy object initialization for unmapped properties #2922
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
base: 2.15.x
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,8 +12,10 @@ | |
| use Doctrine\Persistence\NotifyPropertyChanged; | ||
| use LogicException; | ||
| use ReflectionClass; | ||
| use ReflectionProperty; | ||
| use WeakMap; | ||
|
|
||
| use function array_key_exists; | ||
| use function count; | ||
|
|
||
| use const PHP_VERSION_ID; | ||
|
|
@@ -27,6 +29,9 @@ class NativeLazyObjectFactory implements ProxyFactory | |
| private readonly UnitOfWork $unitOfWork; | ||
| private readonly LifecycleEventManager $lifecycleEventManager; | ||
|
|
||
| /** @var array<class-string, ReflectionProperty[]> */ | ||
| private array $skippedProperties = []; | ||
|
|
||
| public function __construct( | ||
| DocumentManager $documentManager, | ||
| ) { | ||
|
|
@@ -68,13 +73,40 @@ public function getProxy(ClassMetadata $metadata, $identifier): object | |
|
|
||
| $metadata->propertyAccessors[$metadata->identifier]->setValue($proxy, $identifier); | ||
|
|
||
| foreach ($this->getSkippedProperties($metadata) as $property) { | ||
| $property->skipLazyInitialization($proxy); | ||
| } | ||
|
|
||
| if (isset(self::$lazyObjects)) { | ||
| self::$lazyObjects[$proxy] = true; | ||
| } | ||
|
|
||
| return $proxy; | ||
| } | ||
|
|
||
| /** @return ReflectionProperty[] */ | ||
| private function getSkippedProperties(ClassMetadata $metadata): array | ||
| { | ||
| if (isset($this->skippedProperties[$metadata->name])) { | ||
| return $this->skippedProperties[$metadata->name]; | ||
| } | ||
|
|
||
| $skippedProperties = []; | ||
| foreach ($metadata->reflClass->getProperties() as $property) { | ||
| if (array_key_exists($property->name, $metadata->propertyAccessors)) { | ||
| continue; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just to confirm, every mapped field should have its own property accessor according to So this Will this logic be useful in any other contexts? If so, it may be worth capturing this logic in a ClassMetadata method.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It might be an optimization to store the list of unmapped properties in the ClassMetadata, and leverage the cache instead of iterating over all the class properties. Also, there might be an issue if the ClassMetadata is modified after the first lazy object is created, as this list is not updated. I'd rather keep this simple design for now. |
||
| } | ||
|
|
||
| if ($property->isVirtual()) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm pleasantly surprised that the ReflectionProperty docs explain what virtual properties are. |
||
| continue; | ||
| } | ||
|
|
||
| $skippedProperties[] = $property; | ||
| } | ||
|
|
||
| return $this->skippedProperties[$metadata->name] = $skippedProperties; | ||
| } | ||
|
|
||
| /** @internal Only for tests */ | ||
| public static function enableTracking(bool $enabled = true): void | ||
| { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,6 +15,7 @@ | |
| use Doctrine\ODM\MongoDB\Tests\BaseTestCase; | ||
| use Documents\Account; | ||
| use Documents\Address; | ||
| use Documents\DocumentWithUnmappedProperties; | ||
| use Documents\Group; | ||
| use Documents\Phonenumber; | ||
| use Documents\Profile; | ||
|
|
@@ -28,6 +29,15 @@ | |
|
|
||
| class ReferencesTest extends BaseTestCase | ||
| { | ||
| public function testSkipInitializationForUnmappedProperties(): void | ||
| { | ||
| $loadedDocument = $this->dm->getReference(DocumentWithUnmappedProperties::class, '123'); | ||
| $this->assertInstanceOf(DocumentWithUnmappedProperties::class, $loadedDocument); | ||
|
|
||
| // Accessing unmapped property should not initialize the document | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we add a check using |
||
| self::assertSame('bar', $loadedDocument->foo); | ||
| } | ||
|
|
||
| public function testManyDeleteReference(): void | ||
| { | ||
| $user = new User(); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Memoization!