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] Fix CRM_Utils_Array calls that return potentially incorrect value types #26899

Merged
merged 9 commits into from
Jul 22, 2023

Conversation

colemanw
Copy link
Member

@colemanw colemanw commented Jul 21, 2023

Overview

Resolves a lot of current & potential-future php deprecation notices by removing CRM_Utils_Array calls that were passing out an incorrect data type.

Before

Lots of cases where it would have been trying to concatenate strings with NULL, or pass NULL into a function that only accepts a string, like trim().

After

Fixed

Technical Details

The way it was interacting with CRM_Utils_String::munge was especially problematic because in a scenario like

CRM_Utils_String::munge(CRM_Utils_Array::value('name', $thing))

The default return value of NULL could get passed into munge which would then pass it into trim() and preg_replace() causing two warnings, and then would fail to recognize that it was empty because it wasn't equal to $char. I've cleaned that up too.

@civibot
Copy link

civibot bot commented Jul 21, 2023

Thank you for contributing to CiviCRM! ❤️ We will need to test and review the PR. 👷

Introduction for new contributors
  • If this is your first PR, an admin will greenlight automated testing with the command ok to test or add to whitelist.
  • A series of tests will automatically run. You can see the results at the bottom of this page (if there are any problems, it will include a link to see what went wrong).
  • A demo site will be built where anyone can try out a version of CiviCRM that includes your changes.
  • If this process needs to be repeated, an admin will issue the command test this please to rerun tests and build a new demo site.
  • Before this PR can be merged, it needs to be reviewed. Please keep in mind that reviewers are volunteers, and their response time can vary from a few hours to a few weeks depending on their availability and their knowledge of this particular part of CiviCRM.
  • A great way to speed up this process is to "trade reviews" with someone - find an open PR that you feel able to review, and leave a comment like "I'm reviewing this now, could you please review mine?" (include a link to yours). You don't have to wait for a response to get started (and you don't have to stop at one!) the more you review, the faster this process goes for everyone 😄
  • To ensure that you are credited properly in the final release notes, please add yourself to contributor-key.yml
  • For more information about contributing, see CONTRIBUTING.md.
Quick links for reviewers

@civibot civibot bot added the master label Jul 21, 2023
@colemanw colemanw force-pushed the utilsArrayFixes branch 4 times, most recently from 816e315 to dfe00e2 Compare July 21, 2023 21:40
@colemanw colemanw changed the title Replace CRM_Utils_Array with potentially incorrect value types [REF] Fix CRM_Utils_Array calls that return potentially incorrect value types Jul 21, 2023
@mlutfy mlutfy added the merge ready PR will be merged after a few days if there are no objections label Jul 21, 2023
@mlutfy
Copy link
Member

mlutfy commented Jul 21, 2023

Looks good to me, based on code review.

@totten
Copy link
Member

totten commented Jul 22, 2023

The ?? is definitely nicer style than CRM_Utils_Array::value(), and it's a good point that some of those were using $default=NULL without much attention to what works best in the context. So generally: 🎉

Nitpick: As posted, it's one commit with many changes (100+ items over 50+ files) and with several subtle variations in detail (e.g. some items retain NULL default; some are switched to '' or 0). I worry that test-coverage doesn't really address breadth+subtlety of changes. What I might do is rebase/split to highlight the different types of changes (just so that I can see/agree with each as bucket)

@totten totten force-pushed the utilsArrayFixes branch 2 times, most recently from d0e7742 to ab268a1 Compare July 22, 2023 03:33
@colemanw
Copy link
Member Author

Ok thanks @totten those splits make sense.

@colemanw colemanw merged commit 1f0a905 into civicrm:master Jul 22, 2023
@colemanw colemanw deleted the utilsArrayFixes branch July 22, 2023 06:10
@totten
Copy link
Member

totten commented Jul 22, 2023

Yeah, it's a big one, so it's good for it to merge quickly.

FWIW, I read through a bunch of these, and they seemed fine:

  • (REF) Replace CRM_Utils_Array with equivalent ?? NULL
  • (REF) Replace CRM_Utils_Array in context of is_numeric()
  • (REF) Replace CRM_Utils_Array having default ''
  • (REF) Replace CRM_Utils_Array in context of conditional expressions
  • (REF) Replace CRM_Utils_Array in context of string-building (HTML/SQL…

I didn't look closely at the others:

  • (REF) Replace CRM_Utils_Array in context of munge()
  • (REF) Replace CRM_Utils_Array in context of date-time function calls
  • CRM_Utils_String::munge() - Change test
  • (REF) Replace CRM_Utils_Array in more complex cases

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
master merge ready PR will be merged after a few days if there are no objections
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants