Skip to content

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
@greg0ire greg0ire added this to the 3.4.0 milestone Feb 27, 2025
@greg0ire greg0ire merged commit d540f73 into doctrine:3.4.x Feb 27, 2025
84 checks passed
assert(isset($class->reflFields[$fieldName]));
$propertyType = $class->reflFields[$fieldName]->getType();
$fieldName = $fieldMapping->fieldName;
$propertyType = (new ReflectionProperty($fieldMapping->declared ?: $class->name, $fieldName))->getType();
Copy link

@soyuka soyuka Feb 28, 2025

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:

20250228_11h05m35s_grim

Copy link

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?

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.

Copy link
Member Author

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?

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

greg0ire added a commit that referenced this pull request Mar 16, 2025
Bugfix: Missed a spot using getUnderlyingReflector
greg0ire added a commit to greg0ire/doctrine-orm that referenced this pull request Mar 18, 2025
We should use the newly introduced ClassMetadata::$propertyAccessors instead.
See doctrine#11659
greg0ire added a commit to greg0ire/doctrine-orm that referenced this pull request Jul 24, 2025
It is replaced with property accessors since
doctrine#11659
greg0ire added a commit to greg0ire/doctrine-orm that referenced this pull request Jul 24, 2025
It is replaced with property accessors since
doctrine#11659
greg0ire added a commit to greg0ire/doctrine-orm that referenced this pull request Jul 24, 2025
It is replaced with property accessors since
doctrine#11659
@doctrine doctrine deleted a comment from michaeltyra19-collab Aug 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants