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

APIv3 - Improve array-based apis to support sorting and operators #19690

Merged
merged 1 commit into from
Mar 1, 2021

Conversation

colemanw
Copy link
Member

@colemanw colemanw commented Feb 28, 2021

Overview

This backports some APIv4 code to v3, for the purpose of supporting entityRef widgets for Afform.

PR broken off from #19687 for easier review.

Before

Array-based apis such as Afform would have very limited support for searching and sorting.

After

Full range of search operators supported. Sorting supported.

Comments

Adds some corresponding unit test coverage.

@civibot
Copy link

civibot bot commented Feb 28, 2021

(Standard links)

@civibot civibot bot added the master label Feb 28, 2021
api/v3/utils.php Outdated
if (is_array($searchValue) && count($searchValue) === 1 && in_array(array_keys($searchValue)[0], CRM_Core_DAO::acceptedSQLOperators())) {
$operator = array_keys($searchValue)[0];
$searchValue = array_values($searchValue)[0];
return \Civi\Api4\Generic\Traits\ArrayQueryActionTrait::filterCompare($record, [$key, $operator, $searchValue]);
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't love converting this to static - could we instantiate an instance & call it with $this-> ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not really, no. It's a trait. And traits can't be instantiated, nor can they be used by classless APIv3 files.

Copy link
Member Author

Choose a reason for hiding this comment

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

One alternative would be copy & paste, if you're worried about future APIv4 refactoring affecting v3. But IMO we can cross that bridge later & this is ok for now.

Copy link
Contributor

Choose a reason for hiding this comment

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

@colemanw honestly I'd prefer copy & paste - it's kinda contaminating the structure of what we are building for apiv4 with the needs of our legacy api

@colemanw
Copy link
Member Author

colemanw commented Mar 1, 2021

@eileenmcnaughton ok, copy-paste it is then.
I've updated the code & beefed up the v3 unit test a bit more.

@eileenmcnaughton
Copy link
Contributor

It would be an understatement to say I don't love backporting this stuff to apiv3. However, we have enough test cover specifically on this and the api in general I think the risk factor is OK and I appreciate that we just aren't quite there to run every widget on apiv4 yet

…ators

This backports some APIv4 code to v3, for the purpose of supporting
entityRef widgets for Afform.
@colemanw colemanw merged commit 33e989a into civicrm:master Mar 1, 2021
@colemanw colemanw deleted the api3Operators branch March 1, 2021 02:27
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.

2 participants