Skip to content

Conversation

smhg
Copy link
Contributor

@smhg smhg commented Dec 11, 2024

@gechetspr this fixes the single phpstan error about the condition always being true. It has been adressed as an add-on in other pull requests, but I think it makes sense to handle this in a dedicated PR so CI improves for everyone.

However (this is pushing the limits of my PHP/regex knowledge): I think phpstan might actually be wrong here?
My reasoning: the function uses a regex on the column definition (e.g. VARCHAR (25)). The second match of the regex refers to anything between the brackets (column size). I think, because of the comments inside the regex, phpstan detects there will always be at least one space between the brackets. In that case the if statement is indeed always true.
But the regex contains an /x modifier, which ignores those whitespaces, no?

@staabm would you be able to chime in on what's happening here?

Edit: FWIW the other failing (mysql) tests is addressed separately in #2021.

@smhg smhg mentioned this pull request Dec 11, 2024
@gharlan
Copy link
Contributor

gharlan commented Dec 11, 2024

There seems to be an issue with regex comments in phpstan (with /x modifier).
https://phpstan.org/r/5616feb8-13fb-4e32-8f8c-c92656da3d7e
When removing the # ( comments, the issue is gone.

@smhg
Copy link
Contributor Author

smhg commented Dec 11, 2024

Thank you @gharlan. I've reverted the change and made phpstan ignore the error.

@gharlan
Copy link
Contributor

gharlan commented Dec 11, 2024

phpstan/phpstan#12242

@staabm
Copy link
Member

staabm commented Dec 16, 2024

fixed via phpstan/phpstan-src#3735

@smhg smhg force-pushed the fix-phpstan-regex-whitespace branch from 0c7fca2 to 659da78 Compare April 15, 2025 12:18
@smhg
Copy link
Contributor Author

smhg commented Apr 15, 2025

This has been fixed in phpstan.

@smhg smhg closed this Apr 15, 2025
@smhg smhg deleted the fix-phpstan-regex-whitespace branch June 19, 2025 05:56
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.

3 participants