-
Notifications
You must be signed in to change notification settings - Fork 112
Add failing tests for MissingReadOnlyPropertyAssignRule
#339
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
Add failing tests for MissingReadOnlyPropertyAssignRule
#339
Conversation
d4606d0 to
84328de
Compare
30c140f to
34ec40e
Compare
34ec40e to
c5a8966
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also the PropertieExtension is completely untested - when I initially wrote it, I was happy it removed all false-positives from a project I test each release with. (And I thought I'll never have to touch it again, haha.)
Can you write a few tests for the current behaviour too? Especially - if the whole entity is marked as readonly by Doctrine metadata + doesn't have a constructor, it means that the entity is inserted into database by other means and the ORM is used only for reading it -> properties should be always initialized. But if it's not readonly / has a constructor, it means the properties should not be initialized.
c5a8966 to
fd75bfb
Compare
fd75bfb to
c3834aa
Compare
ok, that looks better I think. The isAlwaysWritten and isAlwaysRead are still not tested though. Any good test case ideas? UPDATE: ah, that's another rule.. let me quickly add some tests for that too
Or anything else you need? I can add it here or in follow-ups, whatever is preferred.
d95669e to
e88fff5
Compare
Most of PropertiesExtension should be covered now, sorry for the many force-pushes 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just got an idea for a rule improvement in phpstan/src. If an extension says isInitialized=true about a property, even the first assignment should say "readonly property is already assigned" :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
interesting, I guess I can take a look at this in phpstan-src
e36b309 to
7c5f72a
Compare
7c5f72a to
909516a
Compare
based on your comments I found another simplification that I did. Not sure why it is there exactly, but I also took over that getIdentifierFieldNames exception handling. do you know still why this was added?
@herndlm Because ClassMetadata can throw various exceptions and I don't want PHPStan crashing on that. There's a new bleedingEdge rule (https://github.com/phpstan/phpstan-doctrine/blob/1.3.x/src/Rules/Doctrine/ORM/EntityMappingExceptionRule.php) that's gonna tell you if Doctrine doesn't like your mappings.
Thank you very much :)
@herndlm Because ClassMetadata can throw various exceptions and I don't want PHPStan crashing on that. There's a new bleedingEdge rule (https://github.com/phpstan/phpstan-doctrine/blob/1.3.x/src/Rules/Doctrine/ORM/EntityMappingExceptionRule.php) that's gonna tell you if Doctrine doesn't like your mappings.
thx, makes sense. I assumed something like that, but didn't see the exception in the class/interface and wasn't sure. but I didn't look too long, maybe I just missed it ;)
Uh oh!
There was an error while loading. Please reload this page.
Closes phpstan/phpstan#7337
Looks like the problem is actually here in
PropertiesExtension(which is now used at least). https://github.com/phpstan/phpstan-doctrine/blob/1.3.6/src/Rules/Doctrine/ORM/PropertiesExtension.php#L85 returns alwaysfalse, because$metadata->isReadOnly(which marks if this is a Doctrine readonly entity?) is already false.Does this need another early exit dealing with native readonly or smth like that? I gave it a shot, but don't know much about this here. Feel free to finish it :)