Skip to content

Fixed join condition replacement with new aliases.#2221

Closed
CvekCoding wants to merge 1 commit into
api-platform:masterfrom
CvekCoding:master
Closed

Fixed join condition replacement with new aliases.#2221
CvekCoding wants to merge 1 commit into
api-platform:masterfrom
CvekCoding:master

Conversation

@CvekCoding

Copy link
Copy Markdown
Contributor
Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets
License MIT

FilterEagerLoadingExtension didn't work properly with join conditions. As a result, the reference to the old alias remained in the condition.

@soyuka

soyuka commented Oct 1, 2018

Copy link
Copy Markdown
Member

Would it be possible to add a regression test to illustrate the issue?

@CvekCoding

Copy link
Copy Markdown
Contributor Author

@soyuka , I'll try to do this on weekend. Unfortunately I don't have time for this on weekdays.

}

$associationAlias = $queryNameGenerator->generateJoinAlias($association);
$associationAlias = $newAlias ?: $queryNameGenerator->generateJoinAlias($association);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

$newAlias ?? $queryNameGenerator->generateJoinAlias($association) ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

$newAlias ?? $queryNameGenerator->generateJoinAlias($association) ?

Yes, you are right. It's more accurate.

@soyuka

soyuka commented Feb 15, 2019

Copy link
Copy Markdown
Member

I think that this has been fixed by b0dbe4a#diff-84e5c2df62a8cf1ce1e759018f78674c lmk if it's not the case

@soyuka soyuka closed this Feb 15, 2019
@CvekCoding

Copy link
Copy Markdown
Contributor Author

@soyuka , no, I checked 2.4.2 branch and my issue is not fixed still.
In my project I cotinue use Api-platform fork.

@soyuka soyuka reopened this May 3, 2019
@soyuka

soyuka commented May 3, 2019

Copy link
Copy Markdown
Member

Thanks @Siregacvek for getting back to me, would it be possible to add a regression test?

@soyuka

soyuka commented May 15, 2019

Copy link
Copy Markdown
Member

superseeded by #2793, thanks for your work on this @Siregacvek, your commit has been used in that PR. You're welcome to check the regression test there to see how we did it :).

@soyuka soyuka closed this May 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants