From 1d470b758666648ede1c2d4cffdbeb1252ab3d18 Mon Sep 17 00:00:00 2001 From: Tim Otten Date: Thu, 22 Feb 2018 08:38:41 -0800 Subject: [PATCH 1/3] Extension.get API - Fix regression (4.7.13) in result filtering 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 517dfba8b * Subsequent partial-fixes were d4c44c700, 9776f417, 34239e81, 525ccb68 CC @lucianov88 @twomice @tschuettler @seamuslee001 for review --- api/v3/Extension.php | 8 ++------ tests/phpunit/api/v3/ExtensionTest.php | 20 ++++++++++++++++++++ 2 files changed, 22 insertions(+), 6 deletions(-) diff --git a/api/v3/Extension.php b/api/v3/Extension.php index 3b9378e6c8e9..441a7f5a88eb 100644 --- a/api/v3/Extension.php +++ b/api/v3/Extension.php @@ -368,12 +368,8 @@ function civicrm_api3_extension_get($params) { $result[] = $info; } } - $options = _civicrm_api3_get_options_from_params($params); - $returnFields = !empty($options['return']) ? $options['return'] : array(); - if (!in_array('id', $returnFields)) { - $returnFields = array_merge($returnFields, array('id')); - } - return _civicrm_api3_basic_array_get('Extension', $params, $result, 'id', $returnFields); + $filterableFields = array('id', 'key', 'type', 'status', 'path'); + return _civicrm_api3_basic_array_get('Extension', $params, $result, 'id', $filterableFields); } /** diff --git a/tests/phpunit/api/v3/ExtensionTest.php b/tests/phpunit/api/v3/ExtensionTest.php index bb04b2a7175b..f9cdd0c99240 100644 --- a/tests/phpunit/api/v3/ExtensionTest.php +++ b/tests/phpunit/api/v3/ExtensionTest.php @@ -92,6 +92,26 @@ public function testExtensionGet() { $this->assertEquals(6, $result['count']); } + /** + * Filtering by status=installed or status=uninstalled should produce different results. + */ + public function testExtensionGetByStatus() { + $installed = $this->callAPISuccess('extension', 'get', array('status' => 'installed')); + $uninstalled = $this->callAPISuccess('extension', 'get', array('status' => 'uninstalled')); + + // If the filter works, then results should be strictly independent. + $this->assertEquals( + array(), + array_intersect( + CRM_Utils_Array::collect('key', $installed['values']), + CRM_Utils_Array::collect('key', $uninstalled['values']) + ) + ); + + $all = $this->callAPISuccess('extension', 'get', array()); + $this->assertEquals($all['count'], $installed['count'] + $uninstalled['count']); + } + public function testGetMultipleExtensions() { $result = $this->callAPISuccess('extension', 'get', array('key' => array('test.extension.manager.paymenttest', 'test.extension.manager.moduletest'))); $this->assertEquals(2, $result['count']); From c3733bf533daf1aef3bb6f72074c4318c917cb7f Mon Sep 17 00:00:00 2001 From: Tim Otten Date: Thu, 22 Feb 2018 12:15:24 -0800 Subject: [PATCH 2/3] Extension.get API - Only apply special filters once (key/keys/full_name) 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` 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. --- api/v3/Extension.php | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/api/v3/Extension.php b/api/v3/Extension.php index 441a7f5a88eb..6926aa8bfbb8 100644 --- a/api/v3/Extension.php +++ b/api/v3/Extension.php @@ -368,7 +368,13 @@ function civicrm_api3_extension_get($params) { $result[] = $info; } } - $filterableFields = array('id', 'key', 'type', 'status', 'path'); + + // These fields have been filtered already, and they have special semantics. + unset($params['key']); + unset($params['keys']); + unset($params['full_name']); + + $filterableFields = array('id', 'type', 'status', 'path'); return _civicrm_api3_basic_array_get('Extension', $params, $result, 'id', $filterableFields); } From 084765118628226fe0504947b7e1c5d282c0b8c7 Mon Sep 17 00:00:00 2001 From: Tim Otten Date: Thu, 22 Feb 2018 12:26:12 -0800 Subject: [PATCH 3/3] api_v3_ExtensionTest::testExtensionGet - Be a little less flaky 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. --- tests/phpunit/api/v3/ExtensionTest.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/phpunit/api/v3/ExtensionTest.php b/tests/phpunit/api/v3/ExtensionTest.php index f9cdd0c99240..0dc76726d8d5 100644 --- a/tests/phpunit/api/v3/ExtensionTest.php +++ b/tests/phpunit/api/v3/ExtensionTest.php @@ -89,7 +89,7 @@ public function testExtensionGet() { $result = $this->callAPISuccess('extension', 'get', array()); $testExtensionResult = $this->callAPISuccess('extension', 'get', array('key' => 'test.extension.manager.paymenttest')); $this->assertNotNull($result['values'][$testExtensionResult['id']]['typeInfo']); - $this->assertEquals(6, $result['count']); + $this->assertTrue($result['count'] >= 6); } /**