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

Use ReadWritePropertiesExtensions in MissingReadOnlyPropertyAssignRule #1357

Merged
merged 2 commits into from May 29, 2022

Conversation

herndlm
Copy link
Contributor

@herndlm herndlm commented May 28, 2022

.. and MissingReadOnlyByPhpDocPropertyAssignRuleTest.

Should close phpstan/phpstan#6632
Should close phpstan/phpstan#7337

I kept it simple and chose a similar testing approach as UnusedPrivatePropertyRuleTest uses. I also double-checked - these 2 classes were the only ones were ClassPropertiesNode::getUninitializedProperties was used without passing extensions.

A dedicated test-case in the doctrine extension using readonly might be a good idea too maybe? I assume there are already cases where the ReadWritePropertiesExtension from there is used.

@herndlm
Copy link
Contributor Author

herndlm commented May 28, 2022

hmmm, had the same phpstan errors (Strict comparison using === between class-string and 'MissingReadOnlyProp…' will always evaluate to false.) first too, but they went away after an autoloader dump, which makes sense. why here though 🤔
UPDATE: looks like it only happens with PHP < 8.1
UPDATE2: generating a composer classmap for that file with PHP 7.4 seems to be working fine. pretty sure, I'm missing something really simple here. gonna wait a bit, maybe somebody can tell me :D

@herndlm herndlm force-pushed the read-write-properties-extension branch from acaeb2b to aa97cab Compare May 28, 2022 19:07
@herndlm
Copy link
Contributor Author

herndlm commented May 29, 2022

Though about this again, not verified yet, but I assume that simply autoloading those PHP 8.1 files won't work in earlier versions. Will check that later and fix it then by conditionally passing the extension.

@ondrejmirtes
Copy link
Member

Yeah, should be tested only on 8.1 😊

@herndlm herndlm force-pushed the read-write-properties-extension branch from 3fbbfa2 to ebf4b37 Compare May 29, 2022 09:05
@herndlm
Copy link
Contributor Author

herndlm commented May 29, 2022

still not sure how to solve the static analysis error, any ideas? any additional checks added will always resolve to ConstBooleanTypes before PHP 8.1. It's not a simple case of not running the test on earlier versions unfortunately..

btw the problem is indeed the autoloading, the phpstan checks are all correct here, the class simply cannot be autoloaded and therefore is not a valid class-string before 8.1.

@ondrejmirtes ondrejmirtes force-pushed the read-write-properties-extension branch from c7586b6 to 933c0f0 Compare May 29, 2022 10:49
@ondrejmirtes
Copy link
Member

I used your first commit and I'm trying to solve the failure in a similar way to how I solved these in the past: 933c0f0

@herndlm
Copy link
Contributor Author

herndlm commented May 29, 2022

aaah, thx Ondrej! I actually remember now touching something close to that ignore-by-php-version file already once in the past..

@ondrejmirtes ondrejmirtes marked this pull request as ready for review May 29, 2022 10:54
@ondrejmirtes ondrejmirtes merged commit 735c822 into phpstan:1.7.x May 29, 2022
@ondrejmirtes
Copy link
Member

Perfect, thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants