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

dev/core#2568 Avoid PHP warnings #20178

Closed
wants to merge 1 commit into from
Closed

Conversation

bjendres
Copy link
Contributor

Overview

Just trying to get rid of PHP warnings, see https://lab.civicrm.org/dev/core/-/issues/2568

@civibot
Copy link

civibot bot commented Apr 28, 2021

(Standard links)

@civibot civibot bot added the master label Apr 28, 2021
@@ -5297,8 +5297,9 @@ public static function getContributionTokenValues($id, $messageToken) {
$result = civicrm_api3('Contribution', 'get', ['id' => $id]);
// lab.c.o mail#46 - show labels, not values, for custom fields with option values.
if (!empty($messageToken)) {
$messageTokenContribution = CRM_Utils_Array::value('contribution', $messageToken);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
$messageTokenContribution = CRM_Utils_Array::value('contribution', $messageToken);
$messageTokenContribution = CRM_Utils_Array::value('contribution', $messageToken, []);

given we are using an array function we probably should return an array by default rather than NULL

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, that's a very good point. Seems like a was a little too quick to respond :)

Copy link
Contributor

Choose a reason for hiding this comment

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

@bjendres Also we should use the coalesce operator instead of the CRM_Utils_Array::value() function.

Eg.

$messageTokenContribution = $messageToken['contribution'] ?? [];

eileenmcnaughton added a commit to eileenmcnaughton/civicrm-core that referenced this pull request Apr 29, 2021
Same as civicrm#20178 but with a test
(and a couple of changes to support that)
@eileenmcnaughton
Copy link
Contributor

I added a test for this here #20188 - it includes a slightly different version of this fix but it might be nicer to merge this & only merge that for the test

@bjendres
Copy link
Contributor Author

Closed in favour of (WIP) #20188

@bjendres bjendres closed this Apr 29, 2021
eileenmcnaughton added a commit to eileenmcnaughton/civicrm-core that referenced this pull request Apr 29, 2021
Same as civicrm#20178 but with a test
(and a couple of changes to support that)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants