Fix typed properties for default metadata (#7939) by Lustmored · Pull Request #8589 · doctrine/orm

Lustmored · 2021-04-04T12:30:05Z

This is continuation of work from #7939. Tests for real life scenarios were missing therefore inheritance from typed properties wasn't running with metadata drivers.

This PR introduces such tests, adds metadata to test object for all drivers and makes small changes in drivers to adapt. Most needed changes were changing default value to null or removing required from not required XML attributes.

beberlei

This is great work, thank you very much! I have only a few comments.

beberlei · 2021-04-04T17:16:57Z

lib/Doctrine/ORM/Mapping/ClassMetadataInfo.php

        if ($this->isTypedProperty($mapping['fieldName'])) {
            $mapping = $this->validateAndCompleteTypedAssociationMapping($mapping);
+        } elseif (isset($mapping['joinColumns'])) {
+            foreach ($mapping['joinColumns'] as &$joinColumn) {


I believe this will not work here, because _validateAndCompleteAssociationMapping is called before the code for setting default joinColumns is called in _validateAndCompleteOneToOneMapping.

I believe, you need to put this code into _validateAndCompleteOneToOneMapping, it is also used for ManyToOne, and those two assocations are the only two that we are interested in completing anyways.

Otherwise it will not work for the case where you only specify @ManyToOne but not a corresponding @JoinColumn.

You are right and adding tests for @ManyToOne without joinColumn uncovered it. I have fixed it exactly as suggested.

beberlei · 2021-04-04T17:17:36Z

lib/Doctrine/ORM/Mapping/Driver/AnnotationDriver.php

            $columnAnnot = $this->reader->getPropertyAnnotation($property, Mapping\Column::class);
            if ($columnAnnot) {
-                if ($columnAnnot->type === null) {
-                    throw MappingException::propertyTypeIsRequired($className, $property->getName());


is this method still used anywhere else? otherwise we can @deprecated it in MappingException.

It seems it is not. I have added @deprecated

beberlei · 2021-04-04T17:22:07Z

tests/Doctrine/Tests/Models/TypedProperties/UserTyped.php

     * @OneToOne(cascade={"persist"}, orphanRemoval=true)
     * @JoinColumn
     */
+    #[ORM\OneToOne(cascade: ["persist"], orphanRemoval: true), ORM\JoinColumn]


Please add another ManyToOne association to another email that does not define a join column.

Great suggestion - it have shown a case not covered before and failing due to ManyToOne constructor dependency and wrong place of nullable checks. Fixed :)

Lustmored · 2021-04-05T10:48:51Z

I have rebased and fix mentioned problems. Some tests now fail (despite showing All checks have passed here - take a look in checks tab) but it's withing cascade operation tests so I assume it's out of the scope of this PR and some change in 2.9.x branch probably causes them.

beberlei · 2021-04-11T17:55:03Z

@Lustmored that is a serious heisenbug, can reproduce locally unless I poke Xdebug at it, then it succeeds suddenly. Will investigate. It fails on 2.9.x as well.

beberlei · 2021-04-18T07:07:27Z

For some unclear reason, sometimes the CommitOrderCalculator changes the order of Entity0 and EntityG in the CascadeRemoveOrderTest. This leads to the foreign key failure during commit. The problem is two relationships in both directions, so each entity has a foreign key to the other. The CommitOrderCalculactor does not seem to prefer the order with the join column that has onDelete=SET NULL. This looks like a bug in the CommitOrderCalculator, however why this suddenly starts happening only in this branch is confusing.

beberlei · 2021-04-18T07:09:00Z

Ah of course it happens because of this change: https://github.com/doctrine/orm/pull/8589/files#diff-20cad4158379b4e6d3f30045b627fd986d0b238212c8b0318bbfca13d09f0d3dR44

…ble by default

…rderTest to circumvent new CommitOrderCalculator bug.

…or BC

…8678) * [GH-8589] A new approach to non-nullable typed associations for BC * Address review comment about keeping the target entity type detection.

fixes lowest tests see doctrine/orm#8589 nullable default value changes and breaks our GraphQl schema

Lustmored mentioned this pull request Apr 4, 2021

Use typed properties for default metadata for #7939 #8439

Merged

beberlei requested changes Apr 4, 2021

View reviewed changes

Lustmored force-pushed the fix-typed-2.9.x branch from 86a114f to d14d0a2 Compare April 5, 2021 10:32

Lustmored requested a review from beberlei April 5, 2021 10:49

beberlei added this to the 2.9.0 milestone Apr 11, 2021

Lustmored and others added 11 commits April 18, 2021 09:33

Make Column::$type, Column::$nullable and JoinColumn::$nullable nulla…

f3e63ff

…ble by default

Add tests for mapped typed properties (type and nullable)

0246ac2

Fix Yaml driver tests and remove driver exceptions thrown too early

7d6bad6

Fix PHP driver tests

9060e28

Fix static PHP driver tests

104ba83

Fix XML driver tests

dbc88aa

Coding Standards

0b7ca9d

Deprecate unused MappingException method

4980b9c

Add manyToOne test and check nullable at the right place

ce734ae

Coding Standards

25270cb

Merge branch '2.9.x' into fix-typed-2.9.x

ddcc61c

beberlei force-pushed the fix-typed-2.9.x branch from 330ce2a to ddcc61c Compare April 18, 2021 07:34

beberlei added 2 commits April 18, 2021 14:12

Bugfix: Temporarily change association join columns in CascadeRemoveO…

a7592dc

…rderTest to circumvent new CommitOrderCalculator bug.

phpcs

e0f99a8

beberlei approved these changes Apr 18, 2021

View reviewed changes

beberlei merged commit a223048 into doctrine:2.9.x Apr 18, 2021

beberlei mentioned this pull request Apr 18, 2021

Check CommitOrderCalculator inconsistencies before 2.9.0 #8638

Closed

beberlei added a commit to beberlei/doctrine2 that referenced this pull request May 9, 2021

[doctrineGh-8589] A new approach to non-nullable typed associations f…

1add3d8

…or BC

beberlei mentioned this pull request May 9, 2021

[Gh-8589] A new approach to non-nullable typed associations for BC #8678

Merged

jmsche mentioned this pull request May 25, 2021

Upgrade from 2.8.x to 2.9.1: possible BC break for nullable behaviour #8723

Closed

Warxcell mentioned this pull request May 25, 2021

Add automatic type detection for Embedded. #8724

Merged

franmomu mentioned this pull request Feb 20, 2022

Use typed properties for default metadata doctrine/mongodb-odm#2411

Merged

soyuka added a commit to soyuka/core that referenced this pull request Jan 20, 2023

chore: doctrine/orm > 2.9.4

f238da4

fixes lowest tests see doctrine/orm#8589 nullable default value changes and breaks our GraphQl schema

soyuka added a commit to soyuka/core that referenced this pull request Jan 20, 2023

chore: doctrine/orm > 2.9.2

4b6d273

fixes lowest tests see doctrine/orm#8589 nullable default value changes and breaks our GraphQl schema

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix typed properties for default metadata (#7939)#8589

Fix typed properties for default metadata (#7939)#8589
beberlei merged 13 commits intodoctrine:2.9.xfrom
Lustmored:fix-typed-2.9.x

Lustmored commented Apr 4, 2021

Uh oh!

beberlei left a comment

Uh oh!

beberlei Apr 4, 2021

Uh oh!

Lustmored Apr 5, 2021

Uh oh!

beberlei Apr 4, 2021

Uh oh!

Lustmored Apr 5, 2021

Uh oh!

beberlei Apr 4, 2021

Uh oh!

Lustmored Apr 5, 2021

Uh oh!

Lustmored commented Apr 5, 2021

Uh oh!

beberlei commented Apr 11, 2021

Uh oh!

beberlei commented Apr 18, 2021

Uh oh!

beberlei commented Apr 18, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

Lustmored commented Apr 4, 2021

Uh oh!

beberlei left a comment

Choose a reason for hiding this comment

Uh oh!

beberlei Apr 4, 2021

Choose a reason for hiding this comment

Uh oh!

Lustmored Apr 5, 2021

Choose a reason for hiding this comment

Uh oh!

beberlei Apr 4, 2021

Choose a reason for hiding this comment

Uh oh!

Lustmored Apr 5, 2021

Choose a reason for hiding this comment

Uh oh!

beberlei Apr 4, 2021

Choose a reason for hiding this comment

Uh oh!

Lustmored Apr 5, 2021

Choose a reason for hiding this comment

Uh oh!

Lustmored commented Apr 5, 2021

Uh oh!

beberlei commented Apr 11, 2021

Uh oh!

beberlei commented Apr 18, 2021

Uh oh!

beberlei commented Apr 18, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants