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

phpstan: column with whitespace-only size spec #2020

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

smhg
Copy link

@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.

@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
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

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