-
Notifications
You must be signed in to change notification settings - Fork 2.5k
Necessary refactorings for Property hooks #11659
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
Conversation
What are the consequences for the persistence reflection namespace, and for the ODM? I suppose if there is code in persistence, it is because it is shared with the ODM? |
|
@greg0ire for now its just inline because this way its easier to test the changes fully without multi component changes. We can move the PropertyAccessors namespace into persistence if we think that will help for MongoDB. |
|
Should all the PropertyAccessor types be marked as internal so that moving them doesn't become a breaking change? |
…ternal reflection properties. no tests.
…ccessors at runtime.
…ort for lazy objects.
ee39bd0 to
238fb74
Compare
tests/Tests/ORM/Mapping/PropertyAccessors/ReadOnlyAccessorTest.php
Outdated
Show resolved
Hide resolved
Co-authored-by: Claudio Zizza <[email protected]>
….php Co-authored-by: Claudio Zizza <[email protected]>
| assert(isset($class->reflFields[$fieldName])); | ||
| $propertyType = $class->reflFields[$fieldName]->getType(); | ||
| $fieldName = $fieldMapping->fieldName; | ||
| $propertyType = (new ReflectionProperty($fieldMapping->declared ?: $class->name, $fieldName))->getType(); |
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.
This may fail with Embedded mapping:
#[ORM\Embedded(class: 'EmbeddableDummy')]
#[Groups(['friends'])]
public ?EmbeddableDummy $embeddedDummy = null;
The fieldName will be: embeddedDummy.dummyName which leads to a RuntimeException:
Property ApiPlatform\Tests\Fixtures\TestBundle\Entity\RelatedDummy::$embeddedDummy.dummyName does not exist
I'm unsure whereas the FieldMapping is wrong or if it should get fixed here. Stack trace attached:
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.
should I open a new issue for that?
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.
I have the same issue with Embedded.
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.
@soyuka @hlecorche can you please test if #11873 fixes the problem?
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.
@beberlei Yes, it works. Thanks
Bugfix: Missed a spot using getUnderlyingReflector
We should use the newly introduced ClassMetadata::$propertyAccessors instead. See doctrine#11659
It is replaced with property accessors since doctrine#11659
It is replaced with property accessors since doctrine#11659
It is replaced with property accessors since doctrine#11659

This prepares the internals of
ClassMetadatato go away from$reflFieldsas an array of ReflectionProperty and sub-classes. This is problematic in PHP 8.4, because we need to usesetRawValueWithoutLazyInitializationandgetRawValueinstead. In the existing inheritance heirachy this is going to be messy.This PR inlines the Persistence Reflection namespace into a new PropertyAccessors namespace and reworks the API to be wrappers of ReflectionProperty (aggregation) instead of inheritance. It also cleans up the code to avoid the references to old Doctrine proxy instances. You can access the new API via
ClassMetadata::$propertyAccessors.The
PropertyAccessorinterface has just two methods:For backwards compatibility
ClassMetadata::$reflFieldsis converted from array to a newLegacyReflectionFieldsclass that maps from property accessors to persistence-based reflection property instances. Accessing this structure emits a deprecation.Tasks
The actual support for property hooks should be done after this is merged in a seprate PR.