CRM-21792 Extension.get API - Fix regression (4.7.13) in result filtering #11709
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Overview -------- When one calls the `Extension.get` API, it should support filtering. For example, these commands would return only installed extensions or only uninstalled extensions (respectively): ``` cv api Extension.get status=installed cv api Extension.get status=uninstalled ``` Before ------ The implementation of `Extension.get` passes a list of all extensions down to `_civicrm_api3_basic_array_get()` which is supposed to interpret any APIv3 options (such as filter-values/offsets/limits). However, it confuses the `return` list (i.e. the names of any columns *that the user wants to see*) with the *filterable* list (i.e. the names of any columns *for which filtering is permitted*). After ----- The implementation of `Extension.get` passes a list of all extensions down to `_civicrm_api3_basic_array_get()`. It also passes a fixed list of columns which one might reasonably want to filter. It might be nice to support filtering on all fields, but (a) several fields wouldn't be sensible (because they're nested arrays) and (b) this provides a smaller surface-area to maintain. Comments -------- The doc-block for `_civicrm_api3_basic_array_get()` describes the purpose of this field: https://github.com/civicrm/civicrm-core/blob/4.7.0/api/v3/utils.php#L2390-L2391 Since ~v4.7.13, this line of code appears to have gone through several revisions, e.g. * The regression was originally introduced by 517dfba * Subsequent partial-fixes were d4c44c7, 9776f41, 34239e8, 525ccb6 CC @lucianov88 @twomice @tschuettler @seamuslee001 for review
Filtering extensions by key is a good idea -- so good we have three ways to do it! ;) All of these fields accept a `string|array<string>` input, which is different from the typical filtering supported by `_civicrm_api3_basic_array_get()` (which only matches on values)... If you happen to choose a basic filter that passes both filters (i.e. a simple string-match), then it works. But if you choose a filter where they behave differnetly (i.e. an array), then you get weird results.
The test has a magic number which seems to be based on the number of extensions in the `drupal-clean` build profile -- which is basically the smallest number anyone might have. However, it gives false-negatives if you have any other extensions around. Make it a little less flaky.
c649b04 to
0847651
Compare
|
Updated to (a) include unit-test for filtering by status and (b) fix double-filtering for |
|
(CiviCRM Review Template WORD-1.0)
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Overview
When one calls the
Extension.getAPI, it should support filtering. Forexample, these commands would return only installed extensions or only
uninstalled extensions (respectively):
Before
The implementation of
Extension.getpasses a list of all extensions down to_civicrm_api3_basic_array_get()which is supposed to interpret any APIv3options (such as filter-values/offsets/limits). However, it confuses the
returnlist (i.e. the names of any columns that the user wants to see) with thefilterable list (i.e. the names of any columns for which filtering is
permitted).
After
The implementation of
Extension.getpasses a list of all extensions down to_civicrm_api3_basic_array_get(). It also passes a fixed list of columnswhich one might reasonably want to filter.
It might be nice to support filtering on all fields, but (a) several fields
wouldn't be sensible (because they're nested arrays) and (b) this provides a
smaller surface-area to maintain.
Comments
The doc-block for
_civicrm_api3_basic_array_get()describes the purpose of this field:https://github.com/civicrm/civicrm-core/blob/4.7.0/api/v3/utils.php#L2390-L2391
Since ~v4.7.13, this line of code appears to have gone through several revisions, e.g.
CC @lucianov88 @twomice @tschuettler @seamuslee001 for review