CRM-21110 Get Total relationships without need for extra table #11009
Conversation
|
Jenkins test this please |
|
Code looks good to me, but I haven't grokked that query so I'm just going on looks. @eileenmcnaughton @monishdeb any comments? |
There was a problem hiding this comment.
In general, I see no problem here, and agree that all else being equal it's preferable to avoid temp tables. (BTW, if we have to use temp tables, it would be good to name them something that looks obviously temporary, like 'TEMP_civicrm_contact_relationships', since 'civicrm_contact_relationships' looks like it could be a real table and thus be confusing to new devs examining the code.)
My brief testing (with a contact having 6K relationships, and a contact having just 1 relationship), shows no measurable difference in performance on the Relationships tab, comparing with vs. without this PR.
I believe this PR does no harm in performance and improves the code. I recommend merging (after the minor capitalization fix mentioned in my comment).
CRM/Contact/BAO/Relationship.php
Outdated
| return $relationshipCount; | ||
| } | ||
| else { | ||
|
|
||
| if ($includeTotalCount) { | ||
| $values['total_relationships'] = CRM_Core_DAO::singleValueQuery("SELECT count(*) FROM civicrm_contact_relationships"); | ||
| $values['total_relationships'] = CRM_Core_DAO::singleValueQuery("SELECT count(*) FROM ({$queryString}) as r"); |
There was a problem hiding this comment.
Minor point, but capitalize SQL keyword AS.
There was a problem hiding this comment.
Thanks @twomice
|
To be more specific:
|
seamuslee001
force-pushed
the
CRM-21110-2
branch
from
118e1df to
e729799
Compare
|
@eileenmcnaughton @monishdeb i think this should be mergable now following @twomice's review |
|
Yup it make sense. Merging based on @twomice feedback and my code review. |
Overview
This improves on the patch for CRM-21110 from #10907 by keeping the total_relationships array key but getting that value without the need for a separate table
@eileenmcnaughton @monishdeb I have deployed this version of the patch to AUG prod at the moment and @ineffyble does report that the relationships tab feels snappier and i think this is better because it means no temporary table