Avoid lazy object initialization when initializing read-only property #12335
Conversation
greg0ire
force-pushed
the
gh-12166
branch
2 times, most recently
from
47e9350 to
7f075bc
Compare
| public function testProxyWithReadonlyIdIsNotInitializedImmediately(): void | ||
| { | ||
| if (! $this->_em->getConfiguration()->isNativeLazyObjectsEnabled()) { | ||
| $this->markTestSkipped('Property hooks require native lazy objects to be enabled.'); |
There was a problem hiding this comment.
This skipping looks weird to me. This test does not rely on property hooks
There was a problem hiding this comment.
Ah yes, sorry, I copied it from somewhere and failed to adapt the message.
| $id = $proxy->getId(); | ||
| self::assertSame(123, $id); | ||
| self::assertTrue( | ||
| $reflClass->isUninitializedLazyObject($proxy), |
There was a problem hiding this comment.
Using the dedicated method of the base test case should allow making this test compatible with old proxy systems
| use Doctrine\Tests\OrmFunctionalTestCase; | ||
| use PHPUnit\Framework\Attributes\RequiresPhp; | ||
|
|
||
| #[RequiresPhp('>= 8.4.0')] |
There was a problem hiding this comment.
This should not be necessary anymore if we execute the test with all proxy implementations
SenseException
left a comment
SenseException
left a comment
There was a problem hiding this comment.
This is okay for handling the lazy-loading, but if I remove your newly added code, the test fails with a message like "no such table: gh12166_lazy_entity", because it tries to access a table that doesn't exist (and doesn't need to since the test uses a Reference). This can be confusing if a regression happens during future development.
greg0ire
force-pushed
the
gh-12166
branch
2 times, most recently
from
10489bc to
459bac6
Compare
greg0ire
force-pushed
the
gh-12166
branch
2 times, most recently
from
fa3832c to
a4dc09d
Compare
Good point, addressed. |
|
Maybe this should use the |
Initializing e.g. a readonly ID does not require loading any data from the database. However, calling isInitialized() on the reflection of a readonly property triggers the native lazy object initialization. If we have a lazy property at hand, then the property cannot be initialized already, so it is safe to skip the call.
|
@stof I did not know about that method, thanks for pointing it out! It makes the code smaller, so I've combined both I've also been able to remove the word "assume" from the commit message and PR description, which feels good. |
I just discovered now actually. |
3 participants
Initializing e.g. a readonly ID does not require loading any data from
the database. However, calling isInitialized() on the reflection of a
readonly property triggers the native lazy object initialization.
If we have a lazy property at hand, then the property cannot be initialized
already, so it is safe to skip the call.
Fixes #12166