Skip to content

Implement DeclarePositionRule #2759

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 9 commits into from
Nov 22, 2023
Merged

Implement DeclarePositionRule #2759

merged 9 commits into from
Nov 22, 2023

Conversation

@staabm
Copy link
Contributor

@staabm staabm commented Nov 21, 2023

@mad-briller
Copy link
Contributor

mad-briller commented Nov 21, 2023

does this ignore declare used for purposes other than strict_types?
@staabm
Copy link
Contributor Author

staabm commented Nov 21, 2023

good catch - fixed thanks
@ondrejmirtes
Copy link
Member

ondrejmirtes commented Nov 21, 2023

Please name this accordingly "DeclareStrictTypesRule" and also check nonsense in the directive, like 'foo' instead of 0/1.
ondrejmirtes
ondrejmirtes requested changes Nov 21, 2023
View reviewed changes
src/Rules/Keywords/DeclareStrictTypesRule.php Outdated
Copy link
Member

@ondrejmirtes ondrejmirtes Nov 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please don't use the type system for this, just the AST. I'm pretty sure some stuff is not allowed that would slip through this.
ondrejmirtes
ondrejmirtes requested changes Nov 22, 2023
View reviewed changes
src/Rules/Keywords/DeclareStrictTypesRule.php Outdated
) {
return [
RuleErrorBuilder::message(sprintf(
'Declare strict_types must have 0 or 1 as its value.',
Copy link
Member

@ondrejmirtes ondrejmirtes Nov 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This message could be improved with 'Declare strict_types must have 0 or 1 as its value, %s given.'
@ondrejmirtes ondrejmirtes merged commit 37d0a83 into phpstan:1.10.x Nov 22, 2023
@ondrejmirtes
Copy link
Member

ondrejmirtes commented Nov 22, 2023

Thank you!
@ondrejmirtes ondrejmirtes mentioned this pull request Nov 22, 2023
Closed
@staabm staabm deleted the decl branch November 22, 2023 09:20
@ondrejmirtes
Copy link
Member

ondrejmirtes commented Nov 25, 2023

Hey @staabm this rule now causes a false positive: https://phpstan.org/r/adec71c6-2f0f-41b0-afc2-5df3f9110bce

Please look into it as it's currently blocking a release. Thank you.
@staabm
Copy link
Contributor Author

staabm commented Nov 25, 2023

I am on vacation and can have a look on sunday
@staabm staabm mentioned this pull request Nov 26, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

3 participants

@staabm @mad-briller @ondrejmirtes