[TASK] Use safe preg functions in `CssVariableEvaluator`

[oliverklee]

Part of #919

[coveralls]

coverage: 96.425% (-0.09%) from 96.518% when pulling 46ca58369829d0193f1386f432a8b6bb4100649a on cleanup/preg/css-variable-evaluator into 84b03c7465a9653e163b0cc6b1f1574a5ef80416 on main.

[JakeQZ]

We can't use the Safe version of preg_replace_callback universally until we have PHP 8.1 as a minimum version. It was added late to the library and not included until the 3.0 release.

What you have done elsewhere is use a function_exists() test, falling back to the standard PHP function, as in #1457. People using PHP<8.1 will have to live with a possible failure. Those PHP versions are EOL anyway, and it would be good to eliminate the Preg class since we have found a maintained alternative.

It might be worth our adding a Pelago\Emogrifier\preg_replace_callback() function in the meantime to handle the situation, so that the main code isn't cluttered with function_exists() tests. WDYT?

[oliverklee]

It might be worth our adding a Pelago\Emogrifier\preg_replace_callback() function in the meantime to handle the situation, so that the main code isn't cluttered with function_exists() tests. WDYT?

I'd prefer living with these function_exists for some more time, avoiding the work of creating and maintaining a utility class, and then feel even more satisfied when we can drop these switches after dropping support for PHP < 8.1 hopefully quite soon.

Would you be okay with that approach?

[JakeQZ]

It might be worth our adding a Pelago\Emogrifier\preg_replace_callback() function in the meantime to handle the situation, so that the main code isn't cluttered with function_exists() tests. WDYT?

I'd prefer living with these function_exists for some more time, avoiding the work of creating and maintaining a utility class,

It wouldn't be a utility class, it would be a utility function, and it would simply encapsulate the function_exists() test to avoid it being duplicated throughout the code. I.e.:

namespace Pelago\Emogrifier;

function preg_replace_callback($pattern, callable $callback, $subject, int $limit = -1, int &$count = null, int $flags = 0)
{
    if (\function_exists('Safe\\preg_replace_callback')) {
        $result = \Safe\preg_replace_callback($pattern, $callable, $propertyValue);
    } else {
        $result = \preg_replace_callback($pattern, $callable, $propertyValue);
    }
}

I doubt it would require any maintenance.

and then feel even more satisfied when we can drop these switches after dropping support for PHP < 8.1 hopefully quite soon.

As you know, I'm keen to retain compatibility with the EOL PHP versions unless we have a compelling reason to drop them. It is up to the end user whether they continue to use them; in some situations upgrading is problematic. So I don't see dropping support for PHP < 8.1 as "quite soon" at all.

Would you be okay with that approach?

I don't mind having these branches in each piece of code that uses preg_replace_callback(). I just thought that it could be tidied up for now, as outlined above.

[oliverklee]

It wouldn't be a utility class, it would be a utility function,

I see.

I don't know how Composer autoloading would works for functions instead of classes. Do you have any experience with that?

[JakeQZ]

I don't know how Composer autoloading would works for functions instead of classes. Do you have any experience with that?

Me neither. But the Safe library manages it, so we could just replicate whatever they do.

[JakeQZ]

Will look into logging issue(s) with PHPStan or seeing if there are any already existing...

I found https://github.com/phpstan/phpstan/issues/7544 where the callback had incorrect PHPDoc. After a bit of playing around, I think I found that the callback parameter needs to be array<string>, not array<int, string>. It seems that the internal functions are not as tightly-typed as they should be. Watch this space (or another, if it comes as a PR)...

[JakeQZ]

Watch this space (or another, if it comes as a PR)...

I found that using array<array-key, string> as the callback parameter type resolves the PHPStan warning for the internal function.

However, it will always be list<string>. This is an issue with PHPStan and/or the PHP documentation, particularly if PHPStan lifts the types from the online PHP docs (which I know 'Safe' does).

The above is not sufficient not resolve the warning for 'Safe':

 ------ ---------------------------------------------------------------------------------------------------------------
  Line   emogrifier\src\HtmlProcessor\CssVariableEvaluator.php
 ------ ---------------------------------------------------------------------------------------------------------------
  129    Parameter #2 $callback of function Safe\preg_replace_callback expects callable(array): string, Closure(array<
         string>): string given.
         🪪  argument.type
         💡  Type array<string> of parameter #1 $matches of passed callable needs to be same or wider than parameter ty 
         pe array<int|string, mixed> of accepting callable.
 ------ ---------------------------------------------------------------------------------------------------------------

The 'Safe' library has only determined the type to be a generic array. I am sure that changing the parameter of the callback method to that will get rid of the warning, but don't think it is the approach to take.

We should not change our code to cater for bugs in third-party tools (unless absolutely necessary). Rather, we should report the issues found. That's for tomorrow...

(Note to self: Issues are with Safe, PHPStan, and PHP documentation.)

[JakeQZ]

However, it will always be list<string>. This is an issue with PHPStan and/or the PHP documentation, particularly if PHPStan lifts the types from the online PHP docs (which I know 'Safe' does).

The array syntax for a callable is all-of-a-sudden now working in the PHPStan Playground, though it is still not working for a Closure.

[JakeQZ]

However, it will always be list<string>. This is an issue with PHPStan and/or the PHP documentation, particularly if PHPStan lifts the types from the online PHP docs (which I know 'Safe' does).

The array syntax for a callable is all-of-a-sudden now working in the PHPStan Playground, though it is still not working for a Closure.

https://github.com/phpstan/phpstan/issues/13824