Swap CRM_Utils_Array::value for empty() in conditionals #15005

colemanw · 2019-08-09T12:41:17Z

Overview

When the output of CRM_Utils_Array::value is immediately cast to boolean by a conditional, then it's better to use empty() as it's faster & more readable.
If the output is being compared to NULL in the conditional then it's better to use isset().

Technical Details

The before/after statements are identical in every scenario I can think of.

Comments

I did a mass regex replace of these 5 years ago with no ill-effects, this is just catching some stragglers that didn't get converted with that first pass.

civibot · 2019-08-09T12:41:19Z

(Standard links)

If this is your first pull-request for CiviCRM, please browse CONTRIBUTING.md for information about the development and testing processes.
If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

colemanw · 2019-08-09T12:42:06Z

CRM/Badge/BAO/Badge.php

-      if ($startOffset == NULL && CRM_Utils_Array::value('height_participant_image', $formattedRow)) {
-        $startOffset = CRM_Utils_Array::value('height_participant_image', $formattedRow);
+      if ($startOffset == NULL && !empty($formattedRow['height_participant_image'])) {
+        $startOffset = $formattedRow['height_participant_image'];


This was silly as the conditional already established it was not empty

Do you mean earlier in the formatLabel function? Anyway this seems ok but makes spidey-sense tingle for me for a different reason - out of scope here but for reference I don't see how $startOffset could be NULL, and even if it could be then adding it to $y in line 227 is iffy even if it works.

colemanw · 2019-08-09T12:44:46Z

@eileenmcnaughton @demeritcowboy this is the simplest & safest chunk of #15003

eileenmcnaughton · 2019-08-09T23:07:25Z

test this please

demeritcowboy · 2019-08-11T15:32:22Z

CRM/Financial/Form/FinancialAccount.php

@@ -143,7 +143,7 @@ public static function formRule($values, $files, $self) {
      if ($values['financial_account_type_id'] != $financialAccountTypeId) {
        $errorMsg['financial_account_type_id'] = ts('Taxable accounts should have Financial Account Type set to Liability.');
      }
-      if (CRM_Utils_Array::value('tax_rate', $values) == NULL) {
+      if (!isset($values['tax_rate'])) {
        $errorMsg['tax_rate'] = ts('Please enter value for tax rate');


I started at the bottom and have looked upward to about here. Strictly speaking these two lines aren't identical, it just happens to work out because of the context, and actually the original is probably flawed. If $values['tax_rate'] is integer 0 instead of string 0 then the original is true but the replacement is false. However because it's a form value it's a string, and you do want it to evaluate to false here because 0 is an allowed value. So both statements work in context here but similar ones elsewhere might need to take into account whether it could be integer 0 or string 0.

eileenmcnaughton · 2019-08-11T20:49:15Z

api/v3/Payment.php

@@ -132,7 +132,7 @@ function civicrm_api3_payment_cancel($params) {
 */
 function civicrm_api3_payment_create($params) {
  // Check if it is an update
-  if (CRM_Utils_Array::value('id', $params)) {
+  if (!empty($params['id'])) {


eileenmcnaughton · 2019-08-11T20:50:40Z

@demeritcowboy how many of these have you checked? There are 75 in this PR I think - I have checked 1 :-)

demeritcowboy · 2019-08-11T20:53:49Z

I started at the bottom and went up to the null one in financial. I also started to look at the pledge_installment null one. So maybe if you go downwards and I go upwards?

eileenmcnaughton · 2019-08-11T20:54:52Z

OK - I hate having so many in a PR - 10 would be about my limit - I'll do another 5 now & then come back to it later

eileenmcnaughton · 2019-08-11T20:56:22Z

CRM/Activity/BAO/Activity.php

@@ -312,9 +312,9 @@ public static function create(&$params) {
    }

    // Set priority to Normal for Auto-populated activities (for Cases)
-    if (CRM_Utils_Array::value('priority_id', $params) === NULL &&
+    if (!isset($params['priority_id']) &&


OK - it's not the same but I guess priority_id can't be 0....

How is this not the same @eileenmcnaughton? The default return value of CRM_Utils_Array::value is NULL if the variable isn't set, and isset($foo = NULL) returns FALSE.

Right - it could be set & equal to NULL or not set - I think here it doesn't matter in that set & equal to null would still be empty

eileenmcnaughton · 2019-08-11T20:56:31Z

CRM/Activity/BAO/Activity.php

      // if not set and not 0
-      !CRM_Utils_Array::value('id', $params)
+      empty($params['id'])


eileenmcnaughton · 2019-08-11T20:56:39Z

CRM/Admin/Form/Extensions.php

@@ -205,7 +205,7 @@ public function postProcess() {
        'version' => 3,
        'key' => $this->_key,
      ]);
-      if (!CRM_Utils_Array::value('is_error', $result, FALSE)) {
+      if (empty($result['is_error'])) {


eileenmcnaughton · 2019-08-11T20:56:48Z

CRM/Admin/Form/ScheduleReminders.php

@@ -523,7 +523,7 @@ public function parseActionSchedule($values) {
      'end_date',
    ];

-    if (!CRM_Utils_Array::value('absolute_date', $params)) {
+    if (empty($params['absolute_date'])) {


eileenmcnaughton · 2019-08-11T20:57:26Z

CRM/Admin/Form/ScheduleReminders.php

@@ -577,7 +577,7 @@ public function parseActionSchedule($values) {

    $params['is_active'] = CRM_Utils_Array::value('is_active', $values, 0);

-    if (CRM_Utils_Array::value('is_repeat', $values) == 0) {
+    if (empty($values['is_repeat'])) {


ok - I guess is_repeat is a boolean - although that sort of thing always feels like a big assumption with our code

Well since this statement uses loose == and not strict ===, in this case the two statements are equivalent.

demeritcowboy · 2019-08-12T00:08:10Z

CRM/Core/Page/AJAX/RecurringEntity.php

@@ -14,7 +14,7 @@ class CRM_Core_Page_AJAX_RecurringEntity {

  public static function updateMode() {
    $finalResult = [];
-    if (CRM_Utils_Array::value('mode', $_REQUEST) && CRM_Utils_Array::value('entityId', $_REQUEST) && CRM_Utils_Array::value('entityTable', $_REQUEST)) {
+    if (CRM_Utils_Array::value('mode', $_REQUEST) && CRM_Utils_Array::value('entityId', $_REQUEST) && !empty($_REQUEST['entityTable'])) {
      $mode = CRM_Utils_Type::escape($_REQUEST['mode'], 'Integer');


Maybe it's just because of the regex used to find these, but is there a reason only the last instance on the line was converted?

Applied a slightly different regex to fix this and a couple other spots like it.

demeritcowboy · 2019-08-12T00:11:30Z

CRM/Core/BAO/RecurringEntity.php

@@ -917,7 +917,7 @@ public function mapFormValuesToDB($formParams = []) {
        }
      }
      if ($formParams['repeats_by'] == 2) {
-        if (CRM_Utils_Array::value('entity_status_1', $formParams) && CRM_Utils_Array::value('entity_status_2', $formParams)) {
+        if (CRM_Utils_Array::value('entity_status_1', $formParams) && !empty($formParams['entity_status_2'])) {
          $dbParams['entity_status'] = $formParams['entity_status_1'] . " " . $formParams['entity_status_2'];


Another instance where only the last one on the line was converted.

demeritcowboy · 2019-08-12T00:22:11Z

CRM/Contribute/Form/Contribution/Main.php

@@ -915,13 +915,13 @@ public static function formRule($fields, $files, $self) {
          $errors['pledge_installments'] = ts('Please enter a valid number of pledge installments.');
        }
        else {
-          if (CRM_Utils_Array::value('pledge_installments', $fields) == NULL) {
+          if (!isset($fields['pledge_installments'])) {
            $errors['pledge_installments'] = ts('Pledge Installments is required field.');


This is ok. I r-run'd to check. Note that CRM_Utils_Rule::positiveInteger() above lets 0 thru (both int and string), and then as a string it gets thru here both originally and with the replacement, and then it gets caught down below by both the original and replacement on line 924.

demeritcowboy · 2019-08-12T00:28:40Z

Status: I'm up to CRM/Contribute/Form/Contribution/Main.php coming from the bottom.

demeritcowboy · 2019-08-12T19:01:26Z

I think we've looked at them all now unless the recent push added some new lines that weren't there before? If not then I don't have anything more to add.

eileenmcnaughton · 2019-08-12T20:38:25Z

Ok - merging based on @demeritcowboy having gone through them (& me having looked at a smaller number)

colemanw · 2019-08-12T23:36:04Z

Thanks for the merge/review @demeritcowboy & @eileenmcnaughton.
I'm not sure how to go further given our review capacity because my other regexes all make thousands of replacements.

eileenmcnaughton · 2019-08-12T23:39:17Z

@colemanw we've split other high-risk cleanups like this over multiple releases - ie. we removed a handful of the LOWER & dao->free() instances each release for half a dozen or more - which gave us time to see if any of them caused regressions.

I'm less of a fan of these changes as I don't personally have an issue with CRM_Utils_Array::value & am still adding it in some places :-)

civibot bot added the master label Aug 9, 2019

colemanw commented Aug 9, 2019

View reviewed changes

colemanw force-pushed the empty branch from ba482a5 to eb17a31 Compare August 9, 2019 13:58

demeritcowboy reviewed Aug 11, 2019

View reviewed changes

eileenmcnaughton reviewed Aug 11, 2019

View reviewed changes

CRM/Activity/BAO/Activity.php

// if not set and not 0

!CRM_Utils_Array::value('id', $params)

empty($params['id'])

Copy link

Contributor

eileenmcnaughton Aug 11, 2019

Choose a reason for hiding this comment

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

ok

eileenmcnaughton reviewed Aug 11, 2019

View reviewed changes

demeritcowboy reviewed Aug 12, 2019

View reviewed changes

Swap CRM_Utils_Array::value for empty() or isset() in conditionals

de6c59c

colemanw force-pushed the empty branch from eb17a31 to de6c59c Compare August 12, 2019 12:28

eileenmcnaughton merged commit e60c6c5 into civicrm:master Aug 12, 2019

colemanw deleted the empty branch August 12, 2019 21:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Swap CRM_Utils_Array::value for empty() in conditionals #15005

Swap CRM_Utils_Array::value for empty() in conditionals #15005

colemanw commented Aug 9, 2019 •

edited

Loading

civibot bot commented Aug 9, 2019

colemanw Aug 9, 2019

demeritcowboy Aug 12, 2019

colemanw commented Aug 9, 2019

eileenmcnaughton commented Aug 9, 2019

demeritcowboy Aug 11, 2019

eileenmcnaughton Aug 11, 2019

eileenmcnaughton commented Aug 11, 2019

demeritcowboy commented Aug 11, 2019

eileenmcnaughton commented Aug 11, 2019

eileenmcnaughton Aug 11, 2019

colemanw Aug 12, 2019

eileenmcnaughton Aug 12, 2019

eileenmcnaughton Aug 11, 2019

eileenmcnaughton Aug 11, 2019

eileenmcnaughton Aug 11, 2019

eileenmcnaughton Aug 11, 2019

colemanw Aug 12, 2019

demeritcowboy Aug 12, 2019

colemanw Aug 12, 2019

demeritcowboy Aug 12, 2019

colemanw Aug 12, 2019

demeritcowboy Aug 12, 2019

demeritcowboy commented Aug 12, 2019

demeritcowboy commented Aug 12, 2019

eileenmcnaughton commented Aug 12, 2019

colemanw commented Aug 12, 2019

eileenmcnaughton commented Aug 12, 2019

Swap CRM_Utils_Array::value for empty() in conditionals #15005

Swap CRM_Utils_Array::value for empty() in conditionals #15005

Conversation

colemanw commented Aug 9, 2019 • edited Loading

Overview

Technical Details

Comments

civibot bot commented Aug 9, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colemanw commented Aug 9, 2019

eileenmcnaughton commented Aug 9, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eileenmcnaughton commented Aug 11, 2019

demeritcowboy commented Aug 11, 2019

eileenmcnaughton commented Aug 11, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

demeritcowboy commented Aug 12, 2019

demeritcowboy commented Aug 12, 2019

eileenmcnaughton commented Aug 12, 2019

colemanw commented Aug 12, 2019

eileenmcnaughton commented Aug 12, 2019

colemanw commented Aug 9, 2019 •

edited

Loading