MyIntervals / emogrifier

Converts CSS styles into inline style attributes in your HTML code.
https://www.myintervals.com/emogrifier.php
MIT License
907 stars 154 forks source link

Return values from preg_replace* are not checked #1225

Closed JakeQZ closed 1 month ago

JakeQZ commented 1 year ago

preg_replace and preg_replace_callback may return null if an error occurs. A malformed regular expression would be picked up immediately, but catastrophic backtracking would depend on the input string.

This issue is picked up by Psalm only on a curious system configuration (#1223).

There's a already a private method CssInliner::pregReplace to cater for this (either throwing an exception or triggering an error and returning the input string unmodified, depending on debug setting). This would need moving to a new Utilities class, extending to support array inputs (as well as single strings), with tests (obviously).

Ideally we'd like to specify that the return value is the same type as the input value, which may be string or array<array-key, string>. Psalm clearly does this for the built-in PHP functions, but I don't know how - that needs to be researched.

I'd expect that the Utilities class consists of only static method(s), and a static property (maybe private with getter and setter) defining whether to throw an exception in the error case, or attempt to muddle on with the string replacement not done. CssInliner::setDebug can set this, though users would have the option to set the property of this class differently immediately after. Maybe a sixth parameter, bool $alwaysThrowException might be useful for cases where failure is not an option, but that can be revisited later.

JakeQZ commented 1 month ago

Fixed by #1308. Closing.

Ideally we'd like to specify that the return value is the same type as the input value, which may be string or array<array-key, string>. Psalm clearly does this for the built-in PHP functions, but I don't know how - that needs to be researched.

We now use PHPStan instead of Psalm. It is possible to to implement a Dynamic Return Type PHPStan Extension for this purpose. However, since we have no usage instances of preg_replace/preg_replace_callback where $subject is an array, it is currently not needed. (The wrapper solution in #1308 does not allow $subject to be an array, though allows arrays for the other parameters which can be arrays or strings.)