The Wayback Machine - https://web.archive.org/web/20201109205649/https://github.com/symfony/symfony/pull/38628
Skip to content
Sign up
Sign in Sign up

/ symfony

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

Jump to bottom

[DoctrineBridge] indexBy could reference to association columns #38628

Merged
merged 2 commits into from Nov 9, 2020
Merged

[DoctrineBridge] indexBy could reference to association columns #38628

merged 2 commits into from Nov 9, 2020
+69 −2

Conversation

@juanmiguelbesada
Copy link
Contributor

@juanmiguelbesada juanmiguelbesada commented Oct 19, 2020

Q A
Branch? 3.4
Bug fix? yes
New feature? no
Deprecations? no
Tickets Fix #37982
License MIT
Doc PR -

This is my approach to solve #37982. It partials reverts @xabbuh PR #38604

This is my first Symfony contribution, so please, tell me if I need to do something more or something is wrong.

Also, this bug affects 4.x and 5.x versions. I think merging in this branches is done automatically. If not, please tell me.

Thanks you
@juanmiguelbesada juanmiguelbesada requested a review from dunglas as a code owner Oct 19, 2020
@juanmiguelbesada juanmiguelbesada mentioned this pull request Oct 19, 2020
Closed
@nicolas-grekas nicolas-grekas added this to the 3.4 milestone Oct 19, 2020
@xabbuh
xabbuh reviewed Oct 27, 2020
View changes
src/Symfony/Bridge/Doctrine/PropertyInfo/DoctrineExtractor.php Outdated
$associationMapping = $subMetadata->getAssociationMapping($fieldName);

/** @var ClassMetadataInfo $subMetadata */
$indexProperty = $subMetadata->getSingleAssociationReferencedJoinColumnName($fieldName);

This comment has been minimized.

Sign in to view
@xabbuh

xabbuh Oct 27, 2020
Member

Shouldn't we add some kind of check that the column name used here is in fact the join column?
@nicolas-grekas nicolas-grekas mentioned this pull request Oct 28, 2020
Closed
@nicolas-grekas
Copy link
Member

@nicolas-grekas nicolas-grekas commented Oct 28, 2020

Please check #38861 also.
@juanmiguelbesada
Copy link
Contributor Author

@juanmiguelbesada juanmiguelbesada commented Oct 28, 2020

I will take a look
@jderusse jderusse mentioned this pull request Oct 29, 2020
Closed
@jderusse
jderusse reviewed Oct 30, 2020
View changes
Copy link
Member

@jderusse jderusse left a comment

People at Doctrine says it could/shoud be a property (doctrine/orm#8018 (comment))
Which leads to issue in Symfony: #38861

Given this is not clear, I suggest to supports both cases:

  • IndexBy point to a property
  • IndexBy point to a column_name
@bwach
Copy link

@bwach bwach commented Oct 30, 2020 •
edited

People at Doctrine says it could/shoud be a property (doctrine/orm#8018 (comment))
Which leads to issue in Symfony: #38861

Given this is not clear, I suggest to supports both cases:

* IndexBy point to a property

* IndexBy point to a column_name

This is also reflected in description for ClassMetadataInfo which we use here (depending on how you understand the whole paragraph)

https://github.com/doctrine/orm/blob/c1943624ab1260c629316bab104dc5130c060154/lib/Doctrine/ORM/Mapping/ClassMetadataInfo.php#L516

describing "indexBy" as:

Specification of a field on target-entity that is used to index the collection by.

So it should be one of the fields on the entity, not a column.
I guess the problem in the long run is that doctrine doesn't validate this, so you can type anything into the "indexBy" annotation and it will still work.
@xabbuh
Copy link
Member

@xabbuh xabbuh commented Nov 2, 2020

How does Doctrine behave for one or the other? Does it ignore the attribute? Does it try to map the column to a property or vice versa?
@juanmiguelbesada juanmiguelbesada force-pushed the juanmiguelbesada:issue-37982 branch 2 times, most recently from 6b58fa4 to a0b6778 Nov 5, 2020
@juanmiguelbesada
Copy link
Contributor Author

@juanmiguelbesada juanmiguelbesada commented Nov 5, 2020

PR updated fixing #38861 (I hope) and adding tests
@juanmiguelbesada juanmiguelbesada requested review from xabbuh and jderusse Nov 5, 2020
@xabbuh
xabbuh approved these changes Nov 9, 2020
View changes
Copy link
Member

@xabbuh xabbuh left a comment

LGTM
@carsonbot carsonbot changed the title [DoctrineBridge] indexBy could reference to association columns [DoctrineBridge] indexBy could reference to association columns Nov 9, 2020
juanmiguelbesada and others added 2 commits Oct 19, 2020
@juanmiguelbesada @nicolas-grekas
[DoctrineBridge] indexBy could reference to association columns
4c36145
Bart Wach @nicolas-grekas
failing test for issue 38861
Loading status checks…
f9a0e00
@nicolas-grekas nicolas-grekas force-pushed the juanmiguelbesada:issue-37982 branch from cbd7c6f to f9a0e00 Nov 9, 2020
@nicolas-grekas
Copy link
Member

@nicolas-grekas nicolas-grekas commented Nov 9, 2020

Thank you @juanmiguelbesada.
@nicolas-grekas nicolas-grekas merged commit 6724ca7 into symfony:3.4 Nov 9, 2020
1 of 3 checks passed
1 of 3 checks passed
Travis CI - Pull Request Build Errored
Details
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
fabbot.io Your code looks good.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xabbuh xabbuh

@dunglas dunglas

@jderusse jderusse

Assignees
No one assigned
Labels
Bug DoctrineBridge Status: Reviewed
Projects
None yet
Milestone
3.4
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
@juanmiguelbesada @nicolas-grekas @bwach @xabbuh @jderusse @carsonbot
You can’t perform that action at this time.
Morty Proxy This is a proxified and sanitized view of the page, visit original site.