ENH PHP 8.1 compatibility #1294

emteknetnz · 2022-03-07T04:14:42Z

Issue silverstripe/silverstripe-framework#10250

GuySartorelli

Found a lot of situations where null coalescing just isn't necessary - and I stopped because there were just too many of them. We've discussed this a bit in person but for the sake of having this recorded somewhere can you please respond with your thinking on this? It's definitely overkill the way this is being done so far and it'll be good to have it recorded that this is a conscious decision.

GuySartorelli · 2022-04-21T00:37:03Z

code/AdminRootController.php

@@ -73,7 +73,7 @@ public static function rules()

            // Map over the array calling add_rule_for_controller on each
            $classes = CMSMenu::get_cms_classes(null, true, CMSMenu::URL_PRIORITY);
-            array_map(array(__CLASS__, 'add_rule_for_controller'), $classes);
+            array_map(array(__CLASS__, 'add_rule_for_controller'), $classes ?? []);


CMSMenu::get_cms_classes() always outputs an array, so null coalescing isn't necessary here.

GuySartorelli · 2022-04-21T00:42:14Z

code/AdminRootController.php

@@ -92,7 +92,7 @@ protected static function add_rule_for_controller($controllerClass)
        if ($urlSegment) {
            // Make director rule
            if ($urlRule[0] == '/') {
-                $urlRule = substr($urlRule, 1);
+                $urlRule = substr($urlRule ?? '', 1);


To get to this bit of code, we must already have tried to get index 0 of $urlRule which would have thrown an error if that is null - so null coalescing does nothing here.

GuySartorelli · 2022-04-21T00:45:33Z

code/CMSBatchAction.php

@@ -154,7 +154,7 @@ public function applicablePagesHelper($ids, $methodName, $checkStagePages = true
        $draftPages = DataObject::get($managedClass)->byIDs($ids);

        // Filter out the live-only ids
-        $onlyOnLive = array_fill_keys($ids, true);
+        $onlyOnLive = array_fill_keys($ids ?? [], true);


We're already checking that $ids is an array, so no need for null coalescing here.

silverstripe-admin/code/CMSBatchAction.php

Lines 144 to 146 in d3ae28b

if (!is_array($ids)) {

user_error("Bad \$ids passed to applicablePagesHelper()", E_USER_WARNING);

}

GuySartorelli · 2022-04-21T00:46:26Z

code/CMSBatchAction.php

@@ -163,7 +163,7 @@ public function applicablePagesHelper($ids, $methodName, $checkStagePages = true
                }
            }
        }
-        $onlyOnLive = array_keys($onlyOnLive);
+        $onlyOnLive = array_keys($onlyOnLive ?? []);


This is definitely an array here - it's assigned using array_fill_keys().

GuySartorelli · 2022-04-21T00:47:47Z

code/CMSBatchActionHandler.php

        foreach ($ids as $k => $id) {
            $ids[$k] = (int)$id;
        }
-        return array_filter($ids);
+        return array_filter($ids ?? []);


$ids can only be array or false - never null (it's assigned using preg_split().

Suggested change

return array_filter($ids ?? []);

return array_filter($ids ?: []);

Correct fix here would be some defensive programming like adding a new line about this such as $ids = is_array($ids) ? $ids : [];

However since this hasn't come up in any unit test failures, I think we should just leave it as is. (There will be thousands of other examples similar to this)

GuySartorelli · 2022-04-21T00:54:04Z

code/CMSMenu.php

        foreach ($subClasses as $key => $className) {
            // Remove abstract classes and LeftAndMain
-            if (in_array($className, $abstractClasses) || ClassInfo::classImplements($className, TestOnly::class)) {
+            if (in_array($className, $abstractClasses ?? []) || ClassInfo::classImplements($className, TestOnly::class)) {


$abstractClasses is explicitly declared as an array. No need for null coalescing here.

GuySartorelli · 2022-04-21T00:55:15Z

code/CMSMenuItem.php

@@ -96,7 +96,7 @@ public function getAttributesHTML($attrs = null)

        // Remove empty or excluded values
        foreach ($attrs as $key => $value) {
-            if (($excludeKeys && in_array($key, $excludeKeys))
+            if (($excludeKeys && in_array($key, $excludeKeys ?? []))


The first part of this condition checks for truthiness of $excludeKeys which will fail and short-circuit on null. No need for null coalescing here.

GuySartorelli · 2022-04-21T00:59:51Z

code/GroupImportForm.php

@@ -36,7 +36,7 @@ public function __construct($controller, $name, $fields = null, $actions = null,
            $importer = new GroupCsvBulkLoader();
            $importSpec = $importer->getImportSpec();

-            $columns = implode(', ', array_keys($importSpec['fields']));
+            $columns = implode(', ', array_keys($importSpec['fields'] ?? []));


$importSpec['fields'] here comes from SilverStripe\Dev\BulkLoader which explicitly casted it to array.

https://github.com/silverstripe/silverstripe-framework/blob/1261926911aa5fc59af7287f9895c212db101158/src/Dev/BulkLoader.php#L230

And since GroupCsvBulkLoader is instantiated with the new keyword there's no chance of someone injecting their own class that has null there instead. No need for null coalescing here.

GuySartorelli · 2022-04-21T01:13:16Z

code/LeftAndMain.php

@@ -1765,7 +1765,7 @@ public function CMSVersionNumber()
            return '';
        }
        $version = $lockModules[$moduleName];
-        if (preg_match('#^([0-9]+)\.([0-9]+)#', $version, $m)) {
+        if (preg_match('#^([0-9]+)\.([0-9]+)#', $version ?? '', $m)) {


By this point if $lockModules[$moduleName] was not null (i.e. !isset()) we've already returned out of the method. No need for null coalescing here.

GuySartorelli · 2022-04-21T01:17:49Z

code/MemberImportForm.php

@@ -37,7 +37,7 @@ public function __construct($controller, $name, $fields = null, $actions = null,
            $importer = new MemberCsvBulkLoader();
            $importSpec = $importer->getImportSpec();

-            $columns = implode(', ', array_keys($importSpec['fields']));
+            $columns = implode(', ', array_keys($importSpec['fields'] ?? []));


This is ultimately the same as the GroupCsvBulkLoader import spec as mentioned above.
No need for null coalescing here.

emteknetnz · 2022-04-21T03:12:44Z

It's definitely overkill the way this is being done so far and it'll be good to have it recorded that this is a conscious decision.

Yes agree it's overkill, though I'm not sure how else we can do this. There is just vastly too many places in all the supported modules that needed to be updated to go through all of this manually.

The code writing simply looks to see if a variable is being passed in as a parameter, rather than something like a scalar or a nested native function call with a single return type.

Ideally the code writer would have more smarts about it where it could judge if modification being made to a variable could lead it to being in a nullable state.

However even if we did have a method that could trace back changes made to variables, we're still stuck with the fact that the vast majority of Silverstripe methods have dynamic return types, and the docblock return types are very often wrong, so you cannot rely on them. Since variables are largely a result of calls to Silverstripe methods, we won't very far with an automated cleanup.

At a future date, once Silverstripe method params and return types are fully strongly typed, I would like to do an automated cleanup run that does trace what type variables could be, and then based on that remove the redundant null coalescing statements

Note: this is the code writer

GuySartorelli · 2022-04-21T03:19:03Z

In general I agree - doing this in an automated fashion we're either going to go overboard and miss a few cases, or be light-handed and miss heaps of cases. But on the otherhand doing this all by hand (like I was for the review) is a long laborious process with little payoff. Ultimately having some null coalescing operators where they aren't strictly needed isn't going to hurt anybody, and like you say it'll be much easier to go back and clean that up in a future major release when things are strongly typed.

I'll give Max and other core committers a chance to review this discussion before moving on but I think this is ultimately aan acceptable approach.

maxime-rainville · 2022-04-21T10:08:17Z

I think Steve and I had a similar discussion in a previous Zoom call. Generally, my mind is in the same place yours are. There's not really a way to support PHP8.1 without warning and without having a big automating process that will type cast everything.

To a degree, this will bite us in the ass later once we strongly type everything ... and will potentially neglect some of the benefit of strongly typing because it will obfuscate logic errors.

In the end, we have to begrudgingly accept the automated approach and as a short term necessary evil.

GuySartorelli

As per the above conversation, the purposes of this PR is to avoid passing null (specifically null) from built-in functions that as of php 8.1 will throw deprecation warnings if null is passed in. The approach is necessarily heavy-handed, and while there are many situations where it isn't needed, it would be prohibitively laborious to find all of those situations by hand.

With that context, this looks good to me.

GuySartorelli

Just a question here before I merge.

code/ModelAdmin.php

emteknetnz mentioned this pull request Mar 7, 2022

PHP 8.1 compatibility silverstripe/silverstripe-framework#10250

Closed

emteknetnz force-pushed the pulls/1/php81 branch 3 times, most recently from 2fa6161 to b536605 Compare April 4, 2022 06:47

emteknetnz changed the title ~~NEW PHP 8.1 compatibility~~ ENH PHP 8.1 compatibility Apr 4, 2022

emteknetnz force-pushed the pulls/1/php81 branch 2 times, most recently from d652048 to 0c18046 Compare April 13, 2022 05:36

ENH PHP 8.1 compatibility

d3ae28b

emteknetnz force-pushed the pulls/1/php81 branch from 0c18046 to d3ae28b Compare April 20, 2022 03:01

emteknetnz marked this pull request as ready for review April 20, 2022 05:08

GuySartorelli requested changes Apr 21, 2022

View reviewed changes

GuySartorelli approved these changes Apr 22, 2022

View reviewed changes

GuySartorelli reviewed Apr 22, 2022

View reviewed changes

code/ModelAdmin.php Show resolved Hide resolved

GuySartorelli merged commit 7f7fc72 into silverstripe:1 Apr 22, 2022

GuySartorelli deleted the pulls/1/php81 branch April 22, 2022 04:17

GuySartorelli mentioned this pull request Apr 25, 2022

ENH PHP 8.1 compatibility tractorcow-farm/silverstripe-fluent#763

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ENH PHP 8.1 compatibility #1294

ENH PHP 8.1 compatibility #1294

emteknetnz commented Mar 7, 2022 •

edited

Loading

GuySartorelli left a comment

GuySartorelli Apr 21, 2022

GuySartorelli Apr 21, 2022

GuySartorelli Apr 21, 2022

GuySartorelli Apr 21, 2022

GuySartorelli Apr 21, 2022

emteknetnz Apr 21, 2022

GuySartorelli Apr 21, 2022

GuySartorelli Apr 21, 2022

GuySartorelli Apr 21, 2022

GuySartorelli Apr 21, 2022

GuySartorelli Apr 21, 2022

emteknetnz commented Apr 21, 2022 •

edited

Loading

GuySartorelli commented Apr 21, 2022

maxime-rainville commented Apr 21, 2022

GuySartorelli left a comment

GuySartorelli left a comment

	if (!is_array($ids)) {
	user_error("Bad \$ids passed to applicablePagesHelper()", E_USER_WARNING);
	}

	return array_filter($ids ?? []);
	return array_filter($ids ?: []);

ENH PHP 8.1 compatibility #1294

ENH PHP 8.1 compatibility #1294

Conversation

emteknetnz commented Mar 7, 2022 • edited Loading

GuySartorelli left a 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

emteknetnz commented Apr 21, 2022 • edited Loading

GuySartorelli commented Apr 21, 2022

maxime-rainville commented Apr 21, 2022

GuySartorelli left a comment

Choose a reason for hiding this comment

GuySartorelli left a comment

Choose a reason for hiding this comment

emteknetnz commented Mar 7, 2022 •

edited

Loading

emteknetnz commented Apr 21, 2022 •

edited

Loading