Extract mappings to their own DTOs #10405

greg0ire · 2023-01-14T18:50:27Z

An experiment, not sure if I will manage to do it, but at least it will allow us to port some changes to 2.x:

for instance, everywhere FieldMapping the DTO is used, FieldMapping the array-shape should be used
we can add missing fields to FieldMapping (for instance, version, or default)

Todo

mpdude · 2023-01-15T10:41:12Z

What is the supposed migration path for this?

In 2.x, the ClassMetadataInfo::$fieldMappings is a public property, marked as READ-ONLY, but not internal. To my understanding, that means it is allowed to read and work with them.

I don't know about the deprecation/transition rules for this project. But changing this on the 3.0 branch means things will break during the upgrade, and there is no "smooth" transition (e. g. by getting deprecation warnings in 2.x, which can be fixed, then the 3.0 upgrade can be made).

I generally support the idea and think it's a good thing design-wise.

Can we make that more user-friendly, by

adding the DTO in 2.x already,
implementing ArrayAccess, make things look like before
trigger a deprecation when the array access is actually used
drop the array access in 3.0?

greg0ire · 2023-01-15T14:17:54Z

I'd actually add the array access on 3.0,much like what I did recently for the Token class in doctrine/lexer, that way the type does not change from array to DTO in a minor. What we might do to ease the transition could be to introduce a factory that would produce arrays with nullable properties instead of optional properties, to make the array closer to the final dto

greg0ire · 2023-01-17T20:54:37Z

lib/Doctrine/ORM/Mapping/FieldMapping.php

+    /** @var int|null The database length of the column. Optional. Default value taken from the type. */
+    public int|null $length = null;
+    /**
+     * @var bool|null Marks the field as the primary key of the entity. Multiple


While trying to fix the build, I reverted to bool|null here, to be closer to the previous situation. Now that the build is green, I can do a separate commit to make boolean field not-nullable, and default to false.

greg0ire · 2023-01-17T20:55:13Z

lib/Doctrine/ORM/Mapping/ClassMetadataFactory.php

@@ -328,15 +328,16 @@ private function getShortName(string $className): string
    private function addInheritedFields(ClassMetadata $subClass, ClassMetadata $parentClass): void
    {
        foreach ($parentClass->fieldMappings as $mapping) {
-            if (! isset($mapping['inherited']) && ! $parentClass->isMappedSuperclass) {
-                $mapping['inherited'] = $parentClass->name;
+            $subClassMapping = clone $mapping;


Lost a huge amount of time on this, but yeah, we have to clone here now.

#10397 changed that area, did you include/merge that already?

why can’t you just update the value?

I need to update the child value without updating the parent value. When using an array, a copy is made. When using objects, we're altering the parent objects.

We do need to do a merge up.

Would it be better to do the clone up in doLoadMetadata or so, in a more central location?

Merge up done in #10421

About doLoadMetadata, I'm not sure… I mean there's one clone per mapping to do, so it's not like you could pass clones to addInheritedFields or something 🤔

greg0ire · 2023-01-18T07:56:47Z

Did a new push, comes with static analysis issues that I should be able to solve once I have converted every argument to addMappingInheritanceInformation to a DTO.

greg0ire · 2023-01-21T19:51:17Z

Pushed a new version with an association mapping DTO, hopefully that was the hardest part of the job.

greg0ire · 2023-01-28T09:59:16Z

Just found out that I'm doing the opposite of what's been done 13 years ago in 8d3e0e6 😅
Why oh why?

mpdude · 2023-01-28T13:53:42Z

Maybe performance? Arrays might be faster to access than methods to call?

greg0ire · 2023-01-28T14:25:27Z

Discussed it internally, and you are right… well, you were. It was true in PHP 5.3, but no longer is. I think I can proceed with this.

mpdude · 2023-01-28T14:38:09Z

Before you put too much work into it, are you sure there is a smooth migration path?

greg0ire · 2023-01-28T14:45:06Z

I think the ArrayAccess implementation is enough. All the arrays I'm changing are object-like arrays, so they are very unlikely to be used with anything else than the [] notation. Also, I experimented with this on doctrine/lexer and got no complaints.

Instead of synchronizing isCascade* with cascade, we dynamically compute these methods.

Now that we deal with objects, everything is simpler.

When switching to an object API, it becomes obvious that it should be mandatory. You have to assert it is not null everywhere.

SenseException

I haven't done the full review yet. The amount of changes slows my progress, so maybe I'll do it in parts.

SenseException · 2023-03-29T22:10:32Z

lib/Doctrine/ORM/Cache/DefaultCache.php

@@ -157,7 +158,7 @@ public function evictCollectionRegions(): void

        foreach ($metadatas as $metadata) {
            foreach ($metadata->associationMappings as $association) {
-                if (! $association['type'] & ClassMetadata::TO_MANY) {
+                if (! $association instanceof ToManyAssociationMapping) {


If those are all AssociationMapping, wouldn't the use of a method be an option instead of an instanceof check? You've used methods at other places too.

You're right, I will add such a method.

greg0ire · 2023-03-31T07:19:30Z

Closing in favor of smaller PRs, the first of which will be #10607

greg0ire marked this pull request as draft January 14, 2023 18:50

greg0ire mentioned this pull request Jan 15, 2023

Field mapping improvements #10408

Merged

greg0ire force-pushed the dto branch from 8967aab to bb61b9a Compare January 17, 2023 20:39

greg0ire commented Jan 17, 2023

View reviewed changes

greg0ire force-pushed the dto branch from bb61b9a to e6dca99 Compare January 18, 2023 07:43

greg0ire force-pushed the dto branch from e6dca99 to 3239ca0 Compare January 21, 2023 19:32

greg0ire changed the title ~~Extract FieldMapping in its own DTO~~ Extract FieldMapping and AssociationMapping to their own DTO Jan 21, 2023

greg0ire mentioned this pull request Jan 21, 2023

Use the right property #10441

Merged

greg0ire force-pushed the dto branch from 32446ed to 62ce4df Compare January 21, 2023 19:47

greg0ire force-pushed the dto branch 2 times, most recently from 5ab67bc to c92ad09 Compare January 27, 2023 19:05

greg0ire mentioned this pull request Jan 28, 2023

Move psalm types to ClassMetadata #10478

Merged

greg0ire changed the title ~~Extract FieldMapping and AssociationMapping to their own DTO~~ Extract mappings to their own DTOs Feb 5, 2023

greg0ire force-pushed the dto branch from c92ad09 to 41ab47c Compare February 5, 2023 13:42

greg0ire mentioned this pull request Feb 5, 2023

Use recognized array key #10498

Merged

greg0ire force-pushed the dto branch 4 times, most recently from 8296190 to 6974d9c Compare February 8, 2023 22:51

greg0ire mentioned this pull request Feb 9, 2023

Use array shapes where appropriate #10513

Merged

greg0ire added 22 commits March 28, 2023 10:24

Remove redundant AssociationMapping::$type

49ad40f

Make abstract classes that should be

ef8d196

Move joinColumns to where they belong

1290b20

Use psalm-param to fix cs issue

b5bf1cb

Use more precise/accurate docblocks

4795e65

Move validation logic to dtos

895ce27

Extract array access implementation into a trait

aeed003

switch to static

1fadc99

Remove unused baseline entries

1c869f2

Baseline remaining issues

3c6835c

Make readonly properties that can be

720aa4b

Finalize methods that can be finalized

f8942cd

Rename JoinColumnData to JoinColumnMapping

bdd1647

Remove unused constructor

7ba408b

Use null coalesce assignment operator

e66961d

Document properties as nullable

b5d2ff2

Default orphan removal to false

f667d13

Avoid array access in new files

a883fbe

Avoid array access trait when overridden

3ba09ea

Remove nullability for join columns

1684548

Serialize only what's necessary

35d7b4b

Stop synchronizing properties

abc5625

Instead of synchronizing isCascade* with cascade, we dynamically compute these methods.

greg0ire force-pushed the dto branch from 5b48c7f to abc5625 Compare March 28, 2023 08:25

greg0ire added 2 commits March 28, 2023 11:04

Narrow type down

40d46df

Remove reference access

fb38580

Now that we deal with objects, everything is simpler.

greg0ire requested a review from beberlei March 28, 2023 11:32

Make referencedColumnName mandatory

2924f75

When switching to an object API, it becomes obvious that it should be mandatory. You have to assert it is not null everywhere.

SenseException reviewed Mar 30, 2023

View reviewed changes

greg0ire closed this Mar 31, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extract mappings to their own DTOs #10405

Extract mappings to their own DTOs #10405

greg0ire commented Jan 14, 2023 •

edited

Loading

mpdude commented Jan 15, 2023

greg0ire commented Jan 15, 2023

greg0ire Jan 17, 2023

greg0ire Jan 17, 2023

mpdude Jan 17, 2023

greg0ire Jan 17, 2023

mpdude Jan 17, 2023

greg0ire Jan 17, 2023

greg0ire commented Jan 18, 2023 •

edited

Loading

greg0ire commented Jan 21, 2023

greg0ire commented Jan 28, 2023 •

edited

Loading

mpdude commented Jan 28, 2023

greg0ire commented Jan 28, 2023 •

edited

Loading

mpdude commented Jan 28, 2023

greg0ire commented Jan 28, 2023

SenseException left a comment

SenseException Mar 29, 2023

greg0ire Mar 31, 2023

greg0ire commented Mar 31, 2023

Extract mappings to their own DTOs #10405

Extract mappings to their own DTOs #10405

Conversation

greg0ire commented Jan 14, 2023 • edited Loading

Todo

mpdude commented Jan 15, 2023

greg0ire commented Jan 15, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greg0ire commented Jan 18, 2023 • edited Loading

greg0ire commented Jan 21, 2023

greg0ire commented Jan 28, 2023 • edited Loading

mpdude commented Jan 28, 2023

greg0ire commented Jan 28, 2023 • edited Loading

mpdude commented Jan 28, 2023

greg0ire commented Jan 28, 2023

SenseException left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greg0ire commented Mar 31, 2023

greg0ire commented Jan 14, 2023 •

edited

Loading

greg0ire commented Jan 18, 2023 •

edited

Loading

greg0ire commented Jan 28, 2023 •

edited

Loading

greg0ire commented Jan 28, 2023 •

edited

Loading