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.

Sign up for GitHub

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

Improve conditional in api3 ChainSubscriber #16718

Merged
merged 1 commit into from
Mar 10, 2020
Merged

Improve conditional in api3 ChainSubscriber #16718

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Civi/API/Subscriber/ChainSubscriber.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ public function onApiRespond(\Civi\API\Event\RespondEvent $event) {
$apiRequest = $event->getApiRequest();
if ($apiRequest['version'] < 4) {
$result = $event->getResponse();
if (!is_array($result) || ($result['is_error'] ?? 0) == 0) {
if (is_array($result) && empty($result['is_error'])) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@colemanw I'm pretty sure from my testing that strings & ints (eg. results of getvalue) are expected to pass onto the chaining code. It's a bit odd I guess since I'm not quite sure what gets passed on but we'd need to be sure that's never a good thing before changing

Copy link
Member Author

Choose a reason for hiding this comment

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

@eileenmcnaughton I see no way for that to work, nor can I get it to work in the api explorer.

Copy link
Contributor

@eileenmcnaughton eileenmcnaughton Mar 10, 2020 •
edited by colemanw
Loading

Choose a reason for hiding this comment

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

OK - I guess we don't have test cover so it's not in the contract & you would not be able to pass in anything

Some part of me things I've seen something like this before though

civicrm_api3('DummyEntity', 'dummyAction', [
  'api.Thing1.do => 1,
  'api.Thing2.do => 2,
  'api.Thing13do => 2,
];

As a way of calling multiple actions from one js api call

I can't find it but we'd be breaking it for nothing if my vague feeling is right

Copy link
Member Author

Choose a reason for hiding this comment

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

Again, no I don't think we'd be breaking it because that scenario is already non-functional. Chaining works by looping through $result['values']. If $result is not an array, then there's nothing to loop through, just some PHP errors and no chain.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK

$this->callNestedApi($event->getApiKernel(), $apiRequest['params'], $result, $apiRequest['action'], $apiRequest['entity'], $apiRequest['version']);
$event->setResponse($result);
}
Expand Down