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

CRM-20532 Fix issue where cannot get information on a single extensio… #10322

Merged
merged 3 commits into from
May 9, 2017

Conversation

seamuslee001
Copy link
Contributor

…n with extension.get api and ensure that Fields specified in return key are supported

…n with extension.get api and ensure that Fields specified in return key are supported
@seamuslee001
Copy link
Contributor Author

ping @eileenmcnaughton @twomice @ineffyble I think this fixes it in an elegant way whilst still supporting that effort to have all information for Extensions returned

/**
* Test retunging a single extension
*/
public function testSingleExtesnionGet() {
Copy link
Contributor

Choose a reason for hiding this comment

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

lysdexia rules

@@ -332,6 +332,7 @@ function _civicrm_api3_extension_refresh_spec(&$fields) {
* API result
*/
function civicrm_api3_extension_get($params) {
$keys = (array) $params['key'];
Copy link
Contributor

Choose a reason for hiding this comment

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

could this cause an enotice?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think you can cast a null as an array

Copy link
Contributor

Choose a reason for hiding this comment

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

would it never not be set?

}
return _civicrm_api3_basic_array_get('Extension', $params, $result, 'id', array());
$returnFields = !empty($params['return']) ? (array) $params['return'] : array();
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you should use $options = _civicrm_api3_get_options_from_params() to extract return - we do support 'return' as an array or comma separated string & should be consistent in our inconsistency

@@ -346,9 +347,17 @@ function civicrm_api3_extension_get($params) {
}
$info = CRM_Extension_System::createExtendedInfo($obj);
$info['id'] = $id++; // backward compatibility with indexing scheme
$result[] = $info;
if (!empty($params['key'])) {
Copy link
Contributor

Choose a reason for hiding this comment

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

what about moving the filtering to the start of the loop - I have a feeling some of those calls are expensive

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried that and didn't seem to get anywhere

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also doing it further down keeps the indexing the same no matter what the get is like

Copy link
Contributor

Choose a reason for hiding this comment

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

ok - is this not slow? $info = CRM_Extension_System::createExtendedInfo($obj);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Doesn't feel that slow to me

Copy link
Contributor

Choose a reason for hiding this comment

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

ok

@eileenmcnaughton
Copy link
Contributor

Makes sense - if the tests can't find anything wrong then I can't either

…arams and adding an isset test to the ['key']
@eileenmcnaughton eileenmcnaughton merged commit 5542006 into civicrm:master May 9, 2017
@colemanw
Copy link
Member

@eileenmcnaughton ought the jira ticket https://issues.civicrm.org/jira/browse/CRM-20532 be closed as well?

@seamuslee001
Copy link
Contributor Author

@colemanw I'm dealing with JIRA and I think I'll put a note on the API change log

@seamuslee001 seamuslee001 deleted the CRM-20432 branch May 22, 2017 21:44
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.

3 participants