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

[REF] Replace CRM_Utils_Array::value with ?? in variable assignments #16768

Merged
merged 1 commit into from
Mar 17, 2020

Conversation

colemanw
Copy link
Member

@colemanw colemanw commented Mar 13, 2020

Overview

This is part of a series of PRs to update an old code pattern. CRM_Utils_Array::value() can in many cases be swapped for the new PHP7 ?? operator.

Technical Details

This PR was done entirely by regex find/replace. It targets variable assignments that use a single CRM_Utils_Array::value() call where the 3rd param is either NULL or omitted.

No manual tweaking was done, only regex. The two patterns used were:

  • = CRM_Utils_Array::value\(([^,]+), ([^(),]+)\);$
  • = CRM_Utils_Array::value\(([^,]+), ([^(),]+), NULL\);$

For additional safety, this was done with file filter set to .php, case-sensitive enabled, and comments and string literals excluded.

Screenshot from 2020-03-13 09-32-54

Comments

With 1,698 lines affected, IMO there's only one way to do this: treat it like a mass code autoformat like the one @twomice did a few years ago. Trust the regex, trust the unit tests, do a little spot-checking for sanity's sake, but do not attempt manual review.

@civibot
Copy link

civibot bot commented Mar 13, 2020

(Standard links)

@twomice
Copy link
Contributor

twomice commented Mar 13, 2020

  1. Any thoughts on how this might impact any return-by-reference usage? Is that even used in the codebase? (FWIW, I did a quick manual review of the PR and didn't see the return keyword on any of the changed lines, so this may not be a concern here.)

The statement return $foo ?? $bar; in a return-by-reference function will therefore not work and a warning is issued.

-- From https://www.php.net/manual/en/language.operators.comparison.php

  1. Is this meant to be a step toward deprecating CRM_Utils_Array::value()? That would be nice.

@colemanw
Copy link
Member Author

colemanw commented Mar 13, 2020

@twomice #16719 already tackled return statements so this PR should just be variable assignments.

I don't know when/if CRM_Utils_Array::value() will be officially deprecated. IMO it's not needed and should be phased out.

@twomice
Copy link
Contributor

twomice commented Mar 13, 2020

👍 @colemanw

@colemanw
Copy link
Member Author

I think the big safety consideration here is what happens when the $collection being passed to CRM_Utils_Array::value() is not an array. Doing a little testing, this is what I've found: (note this matrix would get more complex if the $default being passed to CRM_Utils_Array::value()'s 3rd param was something other than null but this PR only deals with that simpler scenario).

$collection type CRM_Utils_Array::value($key, $collection, NULL) $collection[$key] ?? NULL Behaviors match?
array value or null value or null ✔️
null null null ✔️
number null null ✔️
boolean null null ✔️
string null string offset or null sometimes
object null fatal error 🚫

IMO the last two items are ok. Any code passing a string or an object to a function that expects an array is already broken so it's better to have the errors more prominent.

@colemanw
Copy link
Member Author

Test failures should be fixed by merging #16778

@colemanw colemanw changed the title Replace CRM_Utils_Array::value with ?? in variable assignments [REF] Replace CRM_Utils_Array::value with ?? in variable assignments Mar 16, 2020
@colemanw
Copy link
Member Author

This is rebased and tests are passing. It will go stale soon due to its size.

eileenmcnaughton added a commit to eileenmcnaughton/civicrm-core that referenced this pull request Mar 17, 2020
This is a partial of civicrm#16768 & consists of the lines I have reviewed from it
eileenmcnaughton added a commit to eileenmcnaughton/civicrm-core that referenced this pull request Mar 17, 2020
Partial of  civicrm#16768 - only contains test lines which I don't think need individual review
@mlutfy
Copy link
Member

mlutfy commented Mar 17, 2020

👍 I think this really improves the code readability.

(alas it requires rebase)

@colemanw
Copy link
Member Author

@mlutfy - rebased

@mlutfy
Copy link
Member

mlutfy commented Mar 17, 2020

OK, so this PR is really scary (huge number of changes), but based on other similar cleanups Coleman as done (which have gone well), I think we should bite the bullet. Last call? :D

@mlutfy mlutfy merged commit 96d9784 into civicrm:master Mar 17, 2020
@mlutfy
Copy link
Member

mlutfy commented Mar 17, 2020

Linting passed, so I merged (after other test runs had passed previously; this was just a 2 line rebase).

@colemanw colemanw deleted the arrayValue5 branch March 17, 2020 17:38
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