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

APIv4 - Support multiple implicit joins to the same table #21071

Merged
merged 3 commits into from
Aug 17, 2021

Conversation

colemanw
Copy link
Member

@colemanw colemanw commented Aug 9, 2021

Overview

Supports multiple implicit joins to the same table in APIv4 & SearchKit.

Fixes https://lab.civicrm.org/dev/report/-/issues/73

Before

APIv4 always used the name of the table with no sql alias when adding an implicit join, meaning it could only be added once.

After

Creates a unique alias for each instance of an implicit join, allowing the same table to be joined multiple times.

@civibot
Copy link

civibot bot commented Aug 9, 2021

(Standard links)

@civibot civibot bot added the master label Aug 9, 2021
@colemanw colemanw force-pushed the api4JoinFixes branch 2 times, most recently from bb2678e to af57f65 Compare August 9, 2021 15:57
@colemanw colemanw marked this pull request as ready for review August 9, 2021 22:09
@colemanw colemanw changed the title [WIP] Api4 join fixes APIv4 - Support multiple implicit joins to the same table Aug 9, 2021
@colemanw colemanw force-pushed the api4JoinFixes branch 2 times, most recently from fcbd12a to 1d748e1 Compare August 10, 2021 13:11
@MegaphoneJon
Copy link
Contributor

It'll be a couple days before I can do a proper review, but I reviewed the test and can confirm that it's correct.

Before: APIv4 always used the name of the table when adding an implicit join, meaning it could only be added once.

After: Creates a unique alias for each instance of an implicit join, allowing the same table to be joined multiple times.

Fixes dev/report#73
@MegaphoneJon
Copy link
Contributor

My manual testing shows that this fixes the original issue without any side effects I can see (which would get picked up in tests anyway I guess).

@seamuslee001
Copy link
Contributor

Merging based on @MegaphoneJon 's review

@seamuslee001 seamuslee001 merged commit e4218da into civicrm:master Aug 17, 2021
@seamuslee001 seamuslee001 deleted the api4JoinFixes branch August 17, 2021 23:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants