Skip to content

Conversation

@stof
Copy link
Contributor

@stof stof commented Nov 4, 2025

🤔 What's changed?

This upgrades the tooling used for static analysis of the PHP code, and applies fixes for issues reported by the new version (which are all about cases where version 5 was not enforcing proper error handling in cases where PHP functions use a union type with either false or null returned on errors).

⚡️ What's your motivation?

Version 5 of Psalm is not maintained anymore and is incompatible with the latest versions of PHP, preventing adding those versions in the CI setup.
I plan to add newer PHP versions in the CI setup as a follow-up.

🏷️ What kind of change is this?

  • 🏦 Refactoring/debt/DX (improvement to code design, tooling, etc. without changing behaviour)

♻️ Anything particular you want feedback on?

I decided to add a dependency on the composer/pcre package. This is a small package (maintained by the composer team for usage in Composer itself) providing a type-safe API wrapper around the PCRE API. In particular, failures are reported using exceptions instead of returning null and letting you call preg_last_error_msg() to get info about the cause of the error.
If you prefer not adding such a dependency, I could add explicit error handling for the PHP API itself in each place using the regex. But it would basically duplicate part of that package, so it might not be worth it.

📋 Checklist:

@stof
Copy link
Contributor Author

stof commented Nov 4, 2025

Note that another option could be to switch to phpstan, which is more actively maintained these days (and is already used as static analyzer for the tag-expressions PHP implementation).
This would not remove the need to handle failure cases for IO and regex functions though.

@mpkorstanje
Copy link
Contributor

I've requested a review from @ciaranmcnulty but if he doesn't get around to it by next week, please request one from me. PHP isn't my forte so it would be quicker if he reviews it.

If you prefer not adding such a dependency, I could add explicit error handling for the PHP API itself in each place using the regex. But it would basically duplicate part of that package, so it might not be worth it.

As a test framework it would be preferable to have no production dependencies. You could potentially lift somethings from #229. Though I'm not sure about the quality. 😅

@stof
Copy link
Contributor Author

stof commented Nov 5, 2025

#229 solved those cases of failures in preg_replace by casting the null value to an empty string instead of properly throwing an exception (which means it would produce corrupted parsing result if a regex fails for a weird reason). I'd rather have proper error handling.
I updated my PR to do the error handling for the native PHP API instead of using composer/pcre. There was only a few usages (thanks to StringUtils wrapping most usages) so this is maintainable.

@mpkorstanje mpkorstanje self-requested a review November 24, 2025 11:49
@mpkorstanje mpkorstanje merged commit dd89f62 into cucumber:main Nov 24, 2025
3 checks passed
@stof stof deleted the update_php_sa branch November 24, 2025 15:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants