Used Api to get financial account for an entity defined in civicrm_en… #10130
Conversation
There was a problem hiding this comment.
I agree in principle with these changes. I think it's a tidy up. I made a couple of suggestions. In addition it still needs a careful code-readthrough just looking for errors at the line level (spelling, right params etc.) This probably a 10-15 minute job and I'm happy for it to be done internally within JMA since it's just a fresh-pair of eyes thing
CRM/Contribute/PseudoConstant.php
Outdated
| NULL, | ||
| 'account_relationship' | ||
| ); | ||
| public static function getRelationalFinancialAccount($entityId, $accountRelationType, $entityTable = 'civicrm_financial_type') { |
There was a problem hiding this comment.
I can't see any instance where $entityTable is passed in - should we remove this var?
Also - we should probably cache the values to Civi::statics - I think there are examples in CRM_Core_Pseudoconsant
monishdeb
force-pushed
the
codeCleanup
branch
2 times, most recently
from
d470004
to
d59649c
Compare
…tity_financial_account table
| 'account_relationship.name' => $accountRelationType, | ||
| 'entity_table' => 'civicrm_financial_type', | ||
| 'entity_id' => $entityId, | ||
| )); |
There was a problem hiding this comment.
@eileenmcnaughton I have removed the 3rd parameter that was representing entity_table. However, using Civi::$statics won't be applicable here because of dynamic factor $entityId currently looking at the examples at CRM_Core_Pseudoconstant most of them support level 2 combination list. But in this case, it need to support level 3 combination e.g. Civi::$statics[__CLASS__][$entityID][$accountRelationType][$financialAccountID] instead of 2 level combination Civi::$statics[__CLASS__][$accountRelationType][$financialAccountID]. So that will be huge 3 level combination to store in cache
There was a problem hiding this comment.
ok - that's fine - the statics can hold an array of arrays but the parameter was bothering me more - people are motivated to fix performance issues if they matter but bad code signatures get copied & pasted....
Happy for this to be merged
There was a problem hiding this comment.
Sorry, my earlier comment wasn't finished. Just updated it.
|
@eileenmcnaughton the test build passed. Can you merge it? |
…tity_financial_account table