[REF][PHP8.1] Batch 4 of fixing issues found in unit tests where pass… #24018
Conversation
|
(Standard links)
|
seamuslee001
force-pushed
the
php8_1_null_batch_4
branch
from
c2583c0
to
d40e797
Compare
CRM/Core/BAO/OptionGroup.php
Outdated
| @@ -67,7 +67,7 @@ public static function add(&$params, $ids = []) { | |||
| if (empty($params['name']) && empty($params['id'])) { | |||
| $params['name'] = CRM_Utils_String::titleToVar(strtolower($params['title'])); | |||
| } | |||
| elseif (!empty($params['name']) && strpos($params['name'], ' ')) { | |||
| elseif (!empty($params['name']) && strpos(($params['name'] ?? ''), ' ')) { | |||
There was a problem hiding this comment.
This one's odd. If $params['name'] is null it wouldn't even reach that clause. Is it actually null here or is it something like $params['name'] is an object, or a nested array?
There was a problem hiding this comment.
I can't find which test would be doing this. If it's not empty it can't be null.
There was a problem hiding this comment.
ok have updated I think your right but not 100% sure now
CRM/Core/BAO/Address.php
Outdated
| @@ -774,7 +774,7 @@ public static function parseStreetAddress($streetAddress, $locale = NULL) { | |||
| // the DB to fatal | |||
| $fields = CRM_Core_BAO_Address::fields(); | |||
| foreach ($fields as $fieldname => $field) { | |||
| if (!empty($field['maxlength']) && strlen(CRM_Utils_Array::value($fieldname, $parseFields)) > $field['maxlength']) { | |||
| if (!empty($field['maxlength']) && strlen(CRM_Utils_Array::value($fieldname, $parseFields, '')) > $field['maxlength']) { | |||
There was a problem hiding this comment.
We're trying to get rid of this function. Can it just be $parseFields[$fieldname] ?? ''. We know $parseFields is an array so don't have to think about that goofiness.
There was a problem hiding this comment.
There were a couple others lower down - will attack them later.
CRM/Utils/File.php
Outdated
| @@ -1138,7 +1138,7 @@ public static function isDir(?string $dir) { | |||
| return FALSE; | |||
| }); | |||
| try { | |||
| $is_dir = is_dir($dir); | |||
| $is_dir = is_dir($dir ?? ''); | |||
There was a problem hiding this comment.
For this one I'd like to check what they did upstream in php itself with their unit tests. For security reasons this function should behave identical to is_dir with some small exceptions.
There was a problem hiding this comment.
So it looks like php itself does not have a test for null on this. My worry is that there is some edge-case situation where the empty string would return true even though you passed in null, bypassing security. I'm thinking the answer is to update the test to expect something different based on php version, since that is what is actually the case.
I'll take a stab at that.
There was a problem hiding this comment.
This is tricky because it's a deprecation which is hard to get at. How about for now just before the try on line 1140:
if ($dir === NULL) {
return FALSE;
}instead of the ?? ''
There was a problem hiding this comment.
Or no wait it would have to be before the set_error_handler - the very first thing in the function.
seamuslee001
force-pushed
the
php8_1_null_batch_4
branch
2 times, most recently
from
deb4b42
to
5278e0e
Compare
…ing NULL values triggers deprecations Apply fixes as per Demeritcowboy
seamuslee001
force-pushed
the
php8_1_null_batch_4
branch
from
5278e0e
to
7a702ae
Compare
…ing NULL values triggers deprecations
Overview
This is the 4th batch of fixes for when we are passing NULL values to functions that don't like having NULL values passed to them in php8.1
Before
Deprecations triggered
After
Less Deprecations triggered
ping @eileenmcnaughton @totten @demeritcowboy @colemanw