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

Fix attribute ManyToMany mapping #10671

Merged

Conversation

BoShurik
Copy link
Contributor

@BoShurik BoShurik commented May 3, 2023

Fixes #9902

@greg0ire greg0ire changed the base branch from 2.14.x to 2.15.x May 4, 2023 10:18
@greg0ire
Copy link
Member

greg0ire commented May 4, 2023

Can you please write a test for this, based on one of the examples mentioned in the issue you fix?

@BoShurik BoShurik force-pushed the fix-attribute-many-to-many-mapping branch from 61be776 to 4c320a7 Compare May 4, 2023 11:01
@BoShurik
Copy link
Contributor Author

BoShurik commented May 4, 2023

@greg0ire done

@BoShurik BoShurik force-pushed the fix-attribute-many-to-many-mapping branch from 4c320a7 to 2f95c4d Compare May 4, 2023 11:20
Copy link
Member

@greg0ire greg0ire left a comment

Choose a reason for hiding this comment

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

I've tested locally, and the both assertions fail without your changes 👍

Please improve your commit message according to our contributing guide

Right now, one has to read the code and tests to realize what's broken and how you fix it.

@BoShurik BoShurik force-pushed the fix-attribute-many-to-many-mapping branch from 2f95c4d to 436ce8b Compare May 4, 2023 12:12
@greg0ire greg0ire added the Bug label May 4, 2023

foreach ($joinTableAttribute->inverseJoinColumns as $joinColumn) {
$joinTable['inverseJoinColumns'][] = $this->joinColumnToArray($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've looked into the issue, and it looks like there is a piece of code that is just missing from this new implementation. I don't know why it was done like that, but your commit message could look like this:

Take into account join columns specifications.

Currently, the AttributeDriver ignores any join column attribute
specified on a many to many relationship.
Let's copy code from the AnnotationDriver to fix that.

Fixes #9902

Copy link
Member

@greg0ire greg0ire May 4, 2023

Choose a reason for hiding this comment

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

It looks like some other stuff was forgotten in #8266 in this code block: d7d6b9d

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 was missing because php 8.0 does not support nested attributes

Copy link
Member

Choose a reason for hiding this comment

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

Ah great! That explains it fully!

BoShurik added 2 commits May 4, 2023 15:35
Currently, the AttributeDriver ignores any join column attribute specified on a many to many relationship.
Let's copy code from the AnnotationDriver to fix that.

Fixes doctrine#9902
@BoShurik BoShurik force-pushed the fix-attribute-many-to-many-mapping branch from 436ce8b to 2af6e4d Compare May 4, 2023 12:50
@greg0ire greg0ire merged commit 8b0d6a1 into doctrine:2.15.x May 4, 2023
@greg0ire
Copy link
Member

greg0ire commented May 4, 2023

Thanks @BoShurik !

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

Successfully merging this pull request may close these issues.

Wrong mapping for ManyToMany JoinTable fields when use attributes
3 participants