Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Return values from preg_replace* are not checked #1225

Open
JakeQZ opened this issue Sep 9, 2023 · 0 comments
Open

Return values from preg_replace* are not checked #1225

JakeQZ opened this issue Sep 9, 2023 · 0 comments
Assignees
Labels
bug cleanup investigation needed Further investigation is required to determine the cause of the problem and/or the best solution
Milestone

Comments

@JakeQZ
Copy link
Contributor

JakeQZ commented Sep 9, 2023

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 JakeQZ added bug cleanup investigation needed Further investigation is required to determine the cause of the problem and/or the best solution labels Sep 9, 2023
@JakeQZ JakeQZ added this to the 8.0.0 milestone Sep 9, 2023
@JakeQZ JakeQZ self-assigned this Sep 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug cleanup investigation needed Further investigation is required to determine the cause of the problem and/or the best solution
Projects
None yet
Development

No branches or pull requests

1 participant