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 - Fix same-table joins and remove unused code #20715

Merged
merged 1 commit into from
Jun 30, 2021

Conversation

colemanw
Copy link
Member

Overview

This fixes implicit joins between a table and itself, e.g. the Contact.employer_id which links to the Contact table
Also removes alpha code no longer used by APIv4.

Comments

Adds a test to ensure the join works across multiple entities.

@civibot
Copy link

civibot bot commented Jun 26, 2021

(Standard links)

@civibot civibot bot added the master label Jun 26, 2021
This fixes implicit joins between a table and itself, e.g. the Contact.employer_id which links to the Contact table
Also removes alpha code no longer used by APIv4.
}

$canBeShorter = empty($path) || count($currentPath) + 1 < count($path);
$tooFar = $depth > self::MAX_JOIN_DEPTH;
Copy link
Contributor

Choose a reason for hiding this comment

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

@colemanw this seems to be the only thing I wonder about with this, This seems to be something about trying to contort with something in MySQL. Is that replicated somewhere else?

Copy link
Member Author

Choose a reason for hiding this comment

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

The long complicated code here was all made to handle a type of join which no longer exists in APIv4.
During alpha development, there was work done to support a pseudo-join where the api would run the main query, collect the ids and do a second query behind-the-scenes to gather e.g. contact.activities. This functionality has been dropped in favor of explicit joins.

@monishdeb
Copy link
Member

Tested on my local, works fine on depth > 1 joins. Added unit test captures the behavior on implicit joins accurately. Reviewed code, and ensured that the changing findPaths() definition won't cause any breaksge. Based on my these assessments merging the PR.

@monishdeb monishdeb merged commit 962e1ff into civicrm:master Jun 30, 2021
@colemanw colemanw deleted the fixFkJoins branch June 30, 2021 14:30
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