Fixed strlen(): Passing null to parameter in SalesRule/Model/Resource/Rule/Collection.php

[kiatng]

…rce/Rule/Collection.php

PHP8.1 OM v20.2

    [type] => 8192:E_DEPRECATED
    [message] => strlen(): Passing null to parameter #1 ($string) of type string is deprecated
    [file] => .../app/code/core/Mage/SalesRule/Model/Resource/Rule/Collection.php
    [line] => 75
    [uri] => /checkout/cart/add/uenc/.../product/7233/form_key/KHbMmBqhuxasoRHy/

[sreichel]

Strict type safe?

[kiatng]

In our context, $couponCode is expected to be string|null. If it's other types, let it throws error?

[sreichel]

The method says string (or empty string) but its an public method and we dont have type hint for parameters, so we can code this

$m = new Mage_SalesRule_Model_Resource_Rule_Collection();
$m->setValidationFilter(1, 1, false); # false couponCode

This would bypass !== null and run strlen(false). No error actually, but maybe later. And an error with strict types on.

[kiatng]

but maybe later. And and an error with strict types on.

That's the point, we want the error with strict type on. If we cast for scalar type, no error. If we want strict type, we shouldn't cast?

In short, my arguments

1. We want to fix the null deprecation notice for PHP8.
2. We fix it by checking for null only, no casting.
3. When we introduce strict type, no change is required as we want the errors for wrong type.

What do you think?

[sreichel]

If we cast for scalar type, no error. If we want strict type, we shouldn't cast?

If we want strict types, we have to cast ... that is what php does internally w/o strict types on.

"we shouldn't cast" as soon we have typed method parameters.

1. yes, but it is not only about null. Why not fix it "completly"?
2. casting only when it makes sense (passing scalar to php interal, like strlen)
3. dont get it. Turn on strict types should work w/o error.

[kiatng]

I have second thought. Compare

if (strlen($couponCode)) { // S0
// vs
if ($couponCode !== null && strlen($couponCode)) { //S1
// vs
if (is_scalar($couponCode) && strlen((string)$couponCode)) { //S2

If $couponCode is non-scalar, say an array, S0 and S1 will throw an error "Fatal error: Uncaught TypeError: strlen(): Argument #1 ($string) must be of type string, array given" whereas S2 won't. IMHO, S1 seems more correct.

[sreichel]

Mhh. With an non-scalar value (S2) should ignore that if-statement w/o throwing an error. Not? (thought is was the change about)

[kiatng]

This PR and PR #3775, I think an error should be thrown, it'll aid debugging. If there is a bug, it will be silent and make it hard to uncover the bug.

[edit] Like PR #3774, no exception on typo, the bug lives on and quite difficult, took hours, to uncover.

[edit]

(thought is was the change about)

This PR is to fix null deprecation, not about suppressing errors.

[fballiano]

For some reason I cannot merge because it triggers an error:

I'll try again later