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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 13 commits into from
Apr 18, 2021
Merged

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

merged 13 commits into from
Apr 18, 2021

Conversation

Lustmored
Copy link
Contributor

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.

@Lustmored Lustmored mentioned this pull request Apr 4, 2021
Merged
beberlei
beberlei requested changes Apr 4, 2021
View reviewed changes
Copy link
Member

@beberlei beberlei left a comment

Choose a reason for hiding this comment

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

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

lib/Doctrine/ORM/Mapping/ClassMetadataInfo.php Outdated
@@ -1642,6 +1642,14 @@ protected function _validateAndCompleteAssociationMapping(array $mapping)

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

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

lib/Doctrine/ORM/Mapping/Driver/AnnotationDriver.php
@@ -327,10 +327,6 @@ public function loadMetadataForClass($className, ClassMetadata $metadata)
// @Column, @OneToOne, @OneToMany, @ManyToOne, @ManyToMany
$columnAnnot = $this->reader->getPropertyAnnotation($property, Mapping\Column::class);
if ($columnAnnot) {
if ($columnAnnot->type === null) {
throw MappingException::propertyTypeIsRequired($className, $property->getName());
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems it is not. I have added @deprecated

tests/Doctrine/Tests/Models/TypedProperties/UserTyped.php Outdated
public float $float;

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

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.

@Lustmored Lustmored requested a review from beberlei April 5, 2021 10:49
@beberlei beberlei added this to the 2.9.0 milestone Apr 11, 2021
@beberlei
Copy link
Member

@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
Copy link
Member

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
Copy link
Member

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

beberlei added 2 commits April 18, 2021 14:12
@beberlei
Bugfix: Temporarily change association join columns in CascadeRemoveO…
a7592dc 
…rderTest to circumvent new CommitOrderCalculator bug.
@beberlei
phpcs
e0f99a8
@beberlei beberlei merged commit a223048 into doctrine:2.9.x Apr 18, 2021
@beberlei beberlei mentioned this pull request Apr 18, 2021
Closed
beberlei added a commit to beberlei/doctrine2 that referenced this pull request May 9, 2021
@beberlei
[doctrineGh-8589] A new approach to non-nullable typed associations f…
1add3d8 
…or BC
@beberlei beberlei mentioned this pull request May 9, 2021
Merged
beberlei added a commit that referenced this pull request May 10, 2021
@beberlei
[GH-8589] A new approach to non-nullable typed associations for BC (#…
e24b0f0 
…8678)

* [GH-8589] A new approach to non-nullable typed associations for BC

* Address review comment about keeping the target entity type detection.
@jmsche jmsche mentioned this pull request May 25, 2021
Closed
@Warxcell Warxcell mentioned this pull request May 25, 2021
Merged
@franmomu franmomu mentioned this pull request Feb 20, 2022
Merged
soyuka added a commit to soyuka/core that referenced this pull request Jan 20, 2023
@soyuka
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
@soyuka
chore: doctrine/orm > 2.9.2
4b6d273 
fixes lowest tests see doctrine/orm#8589

nullable default value changes and breaks our GraphQl schema
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@beberlei beberlei beberlei approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
2.9.0
Development

Successfully merging this pull request may close these issues.
2 participants
@Lustmored @beberlei