Fix incorrect instance of ReflectionProperty created for public typed properties

[joaojacome]

Resolves #102

[SenseException]

Thanks for your contribution. Your switch in the if-block is supposed to fix an issue with the created ReflectionProperty instance. Can you please create a test that covers this issue and prevents regression in this project?

[joaojacome]

I've added the tests for this issue. Can you check again, please?

[SenseException]

This test got changed by your PR. If the old test using RuntimePublicReflectionProperty would fail with your changes, you might have implemented a BC-break here and 1.3.x wouldn't be the appropriate target branch.

[joaojacome]

The old test fails with the fix - but wouldn't the old behaviour be considered a bug?

[SenseException]

A bugfix that introduces BC break could only be merged into a major release. The fix should also be reviewed by other maintainers too.

[guilliamxavier]

This fix has been hold off for too long... as it is we might not even have it for version 3.0... Wouldn't it be possible to preserve BC? e.g. use a new class that extends RuntimePublicReflectionProperty but uses the same code as TypedNoDefaultReflectionProperty?

[joaojacome]

Thanks for the suggestion. To avoid code duplication, I've added a trait to hold the common methods, although I'm not sure if it's the best approach.

The test now covers the old and the new behaviours.

[guilliamxavier]

Well my question was primarily to @SenseException (and other maintainers), but I was thinking to something like that yes 🙂 Thanks, now let's wait for new reviews 😉

[oalphenaar]

Will this be merged? As of today it is still impossible to remove entities where the id property is public and this seems to be the fix.

[malarzm]

There's #103 (comment) that needs to be checked/addressed

[greg0ire]

Please associate your email address with your Github account, or change the
email in your commits to an address already associated with it. If you do not
want to expose your personal email address, you may use
$userName@users.noreply.github.com, that way we can still reach you through Github.
Additionally, you will get credit for this contribution on your Github profile.

[greg0ire]

My review has been requested. Before I review this, I obviously have to read #102, which mentions an incorrect type but does not explain what makes it incorrect, and refers me to doctrine/orm#7999.
doctrine/orm#7999 is a lengthy issue with 17 replies, a wall of text and 11 participants. It mentions doctrine/orm#7940 a pull request with 35 replies, which fixes doctrine/orm#7864, which is another wall of text.

I might find the time and energy to review this one day, but today is not that day, sorry.

If you want to speed things up, please squash your commits into one, and improve your commit message according to the contributing guide. It would be great if it mentioned the reason you are doing this, #103 (comment) , what changes you made and why they fix the issue.

[guilliamxavier]

@greg0ire: I can at least recap what led me here: The original issue is indeed doctrine/orm#7999: you get a TypeError (from deep in Doctrine's internals) when you try to delete an entity with a generated id on a non-nullable typed property. E.g.:

/** @ORM\Entity */
class MyEntity {
    /** @ORM\Id @ORM\GeneratedValue @ORM\Column(type="integer") */
    public int $id;
}

$myEntity = $entityManager->find(MyEntity::class, 42); // let's assume it exists

$entityManager->remove($myEntity);
$entityManager->flush(); // Typed property Entity::$id must be int, null used

(because Doctrine tries to "reset" the id to null).

Then doctrine/reflection#35 fixed it but only for private/protected properties, not public (maybe because #94 thought that "RuntimePublicReflectionProperty already works with typed no default properties", only considering lazy loading at that time).

Now this PR aims to fix it also for public properties (note: the first fix was simply to reverse the order, resulting in what was initially proposed in #90, but since eventually #94 chose the "wrong" order, there was some BC-change requested).

(As for the EXTRA_LAZY issue, it actually seems unrelated.)

But indeed, there is place for improvement of the commit message (@joaojacome 😉)

[greg0ire]

Thanks, I will try to look at this today, it looks a bit more approachable now :)

[alcaeus]

Nit in tests, but LGTM.

[joaojacome]

@greg0ire:
But indeed, there is place for improvement of the commit message (@joaojacome wink)

I'll squash it and give it a proper message :)

[greg0ire]

Now that's a commit message! It feels a lot safer to merge this, thanks a lot!

[greg0ire]

Thanks everyone!