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

Necessary refactorings for Property hooks #11659

Open
wants to merge 33 commits into
base: 3.4.x
Choose a base branch
from

Conversation

beberlei
Copy link
Member

@beberlei beberlei commented Oct 10, 2024

This prepares the internals of ClassMetadata to go away from $reflFields as an array of ReflectionProperty and sub-classes. This is problematic in PHP 8.4, because we need to use setRawValueWithoutLazyInitialization and getRawValue instead. 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 PropertyAccessor interface has just two methods:

interface PropertyAccessor
{
    public function setValue(object $object, mixed $value): void;
    public function getValue(object $object): mixed;
}

For backwards compatibility ClassMetadata::$reflFields is converted from array to a new LegacyReflectionFields class that maps from property accessors to persistence-based reflection property instances. Accessing this structure emits a deprecation.

Tasks

  • Add tests for all property accessors
  • Static Analysis
  • Think how the 2-3 left over reflFields uses in core can be handled in the future.

The actual support for property hooks should be done after this is merged in a seprate PR.

@greg0ire
Copy link
Member

This PR inlines the Persistence Reflection namespace into a new PropertyAccessors namespace

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?

@beberlei
Copy link
Member Author

@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.

@greg0ire
Copy link
Member

Should all the PropertyAccessor types be marked as internal so that moving them doesn't become a breaking change?

@beberlei beberlei changed the base branch from 3.3.x to 3.4.x November 3, 2024 20:02
@beberlei beberlei marked this pull request as ready for review February 14, 2025 23:03
@beberlei beberlei requested a review from greg0ire February 15, 2025 21:26
greg0ire
greg0ire previously approved these changes Feb 16, 2025
Comment on lines +859 to 870
if ($mapping->enumType !== null) {
$accessor = new EnumPropertyAccessor(
$accessor,
$mapping->enumType,
);
}

$this->reflFields[$field] = new ReflectionEmbeddedProperty(
$parentReflFields[$mapping->declaredField],
$childProperty,
$this->propertyAccessors[$field] = new EmbeddablePropertyAccessor(
$parentAccessors[$mapping->declaredField],
$accessor,
$mapping->originalClass,
);
Copy link
Member

Choose a reason for hiding this comment

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

Not necessarily in scope of this PR, but I'd expect that a factory would also handle the creation of EnumPropertyAccessor and EmbeddablePropertyAccessor.

Copy link
Member Author

Choose a reason for hiding this comment

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

There is a reason I did it this way:

The code in the factory is what was copy adapted from doctrine/persistence, and the enum and embeddable accessors are originally from the ORM code as well. Mixing them in a factory, when we potentially move part of that code out back into persistece library at some point could prove problematic.

@@ -470,7 +470,7 @@ public static function provideCardClasses(): iterable
public function testItAllowsReadingAttributes(): void
{
$metadata = $this->_em->getClassMetadata(Card::class);
$property = $metadata->getReflectionProperty('suit');
$property = $metadata->reflFields['suit'];
Copy link
Member

Choose a reason for hiding this comment

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

Don't we expect/assert here a deprecation trigger?

Copy link
Member Author

Choose a reason for hiding this comment

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

I didnt pay much attention to this change, but developers downstream (like this test simulates looking at the referenced PR) might want to still access the original reflector.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants