-
-
Notifications
You must be signed in to change notification settings - Fork 825
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
[NFC] add comments & extract function in contribution search #9716
Conversation
c702f2c
to
8f165fa
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good and my search still reports sensible things
Merging per review from Seamus. |
* | ||
* @param $apiEntity | ||
* The api entity being called. | ||
* This sort-of duplicates $mode in a confusing way. Probably not by design. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks nicer.
A suggestion on docblock style going-forward (not meant as a complaint about this): @param $apiEntity
looks like a typical case where I'd try to include an example as part of the doc, as in:
* @param string $apiEntity
* The api entity being called. Ex: 'Contact' or 'ContributionPage'.
As readers who are tuned-in to the nuances of Civi's data modelling (SQL/DAO/BAO/API), you and I probably interpret apiEntity
as "an entity name in the camel-case notation of APIv3" (ContributionPage
), but less experienced contributors might interpret "entity" more ambiguously. (ContributionPage
, contribution_page
, api/v3/ContributionPage.php
, civicrm_api3_contribution_page_*
, CRM_Contribute_DAO_ContributionPage
, and civicrm_contribution_page
are all ways to identify the same entity... and all those ways would actually show up if you were tracing an API call.)
An example value is a really simple/clear.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree - although when I add stuff to pre-existing comments I don't feel the need to be that thorough
This is the NFC part of #9631 - I think the main part can wait until after the cut off but the comment additions & database extraction allow simplification of that PR for review.
Note the only non-comment & non-test change is to extract the setting on soft credit options into it's own function