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

Trigger error on unsafe StrictGroups usages #29

Merged
merged 5 commits into from
Jul 13, 2024

Conversation

Seldaek
Copy link
Member

@Seldaek Seldaek commented Jul 11, 2024

@Seldaek
Copy link
Member Author

Seldaek commented Jul 12, 2024

@staabm if you have a sec to review my Rule 🙏🏻 but it seems to work well. I'll try it on the composer repo now see if it catches anything.

@staabm
Copy link
Contributor

staabm commented Jul 12, 2024

I think it looks really good already. good job!

I would separate the type-expectations which require phpstan/phpstan-src#3229 into a separate PR, because ondrey seems to be on holiday and the PR might need a few days before getting merged

@Seldaek
Copy link
Member Author

Seldaek commented Jul 12, 2024

It still fails on 7.2/7.3 but that would require my other PR with the override to fix..

@Seldaek Seldaek merged commit 5b103b3 into composer:2.x Jul 13, 2024
11 of 12 checks passed
@Seldaek Seldaek deleted the more_tweaks branch July 13, 2024 11:21
@staabm
Copy link
Contributor

staabm commented Jul 13, 2024

awesome

@staabm
Copy link
Contributor

staabm commented Jul 13, 2024

wdyt about marking Preg->match as @deprecated use the more static analysis friendly isMatch() instead and giving a hint to use isMatch instead?
(you don't need to remove match in the long run, but this could give people hints to use the more static analysis friendly versions of the api)

@Seldaek
Copy link
Member Author

Seldaek commented Jul 13, 2024

Yes why not i guess. Will do later

@Seldaek
Copy link
Member Author

Seldaek commented Jul 14, 2024

Changed my mind about deprecation, calling isMatch when you don't really check the return value looks/reads weird so I'd rather not force that.

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