Add failing tests for MissingReadOnlyPropertyAssignRule

[herndlm]

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 always false, 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 :)

[ondrejmirtes]

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.

[herndlm]

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.

[herndlm]

Most of PropertiesExtension should be covered now, sorry for the many force-pushes 😅

[ondrejmirtes]

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" :)

[herndlm]

interesting, I guess I can take a look at this in phpstan-src

[herndlm]

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?

[ondrejmirtes]

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

[ondrejmirtes]

Thank you very much :)

[herndlm]

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