fix: OneToManyPersister does not take custom identifier types into account for orphan removal

[wtfzdotnet]

I've taken a look at the test suite, but I am kind of mind-boggled as of where to begin, could you please point me in the right direction to make sure this change gets merged? (I'll tidy up this PR later when it's done)

Thanks! :-)

[mpdude]

Maybe the functional tests named GHxxxTest according to issue numbers give you enough inspiration to get started?

[wtfzdotnet]

Maybe the functional tests named GHxxxTest according to issue numbers give you enough inspiration to get started?

Yeah I would've just placed this next to the existing ManyToManyPersister test, so good I asked, thanks :-)

[wtfzdotnet]

Okay I've made a unit test that validates this behavior ( tested on mysql, and it fails as expected ), adding Ramsey as a vendor obviously doesn't sense, but I could use a little help with ideas as of how to go about testing this via another route :-).

[wtfzdotnet]

@greg0ire think we are good now for a review :-)

[wtfzdotnet]

Also fixes #9745

[greg0ire]

think we are good now for a review

There are PHPStan issues. See https://www.doctrine-project.org/contribute/#running-checks-before-pushing for how to run checks before pushing.

[wtfzdotnet]

think we are good now for a review

There are PHPStan issues. See https://www.doctrine-project.org/contribute/#running-checks-before-pushing for how to run checks before pushing.

I ran this earlier locally, and again after your comment ( I did mess up phpcs, and psalm apparently ).

➜  orm git:(feature/fix-one-to-many-custom-id-orphan-removal) vendor/bin/phpstan
Note: Using configuration file /home/m/git/doctrine/orm/phpstan.neon.
 454/454 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100%


                                                                                                                        
 [OK] No errors                                                                                                         

PHPCS issue is fixed, inconsistency is fixed, but I cannot grasp how the PHPStan errors from the report are related to my PR, could you please double check if I am a dummy?

As for the psalm error, it seems related to the underlying existing documentation which already supports $types, most of the code is just borrowed from the ManyToManyPersister to begin with so I am not sure what I'd have to fix here, I am not so familiar with psalm (unfortunately).

[greg0ire]

PHPCS issue is fixed, incosistency is fixed, but I cannot grasp how the PHPStan errors from the report are related to my PR, could you please double check if I am a dummy?

Ah I didn't check myself, maybe it's related to a recent release, let me check.

[greg0ire]

Some of it is definitely related to a recent release, I'll fix it.

[wtfzdotnet]

Thanks let me know when I should rebase, and if I'd have to do anything else in regards to the psalm error :-), and thanks for your prompt feedback!

[greg0ire]

Ok so you can ignore the second Psalm error, and focus on the 2 other ones, they must be caused by this. See #10754 for the other errors.

[wtfzdotnet]

You'd want me to just rebase on top of that branch or wait until it merges?

[wtfzdotnet]

@greg0ire as I said I am really not familiar with all the ins and outs of psalm, I ran into these baseline errors, after some searching around I ran the vendor/bin/psalm --update-baseline. ( Assuming the baseline is ignoring stuff now that is irrelevant, which cause the errors? )

Ok so you can ignore the second Psalm error, and focus on the 2 other ones, they must be caused by this. See #10754 for the other errors.

I updated the psalm baseline and committed this, assuming that's what is supposed to happen at this stage. As far as I understand it's a sort of "ignore stuff from here", and the errors triggered seem related. I might be misunderstanding this so if I need to do something else please give me a little more guidance, psalm noob :-).

I could imagine this might have more changes than it should, but I am not familiar as of how to treat this file.

[greg0ire]

You will need to wait for #10754 to be merged. The baseline is a file that allows you to ignore errors. Setting Psalm to a very high level generates many errors, too many for us, so we baselined many of them. That way, new code is held to a very high standard, and we can reduce the baseline progressively and make old code also comply with high standards. In your case, it's OK to remove stuff from the baseline if your changes somehow fix them, or make them no longer relevant.

[wtfzdotnet]

You will need to wait for #10754 to be merged. The baseline is a file that allows you to ignore errors. Setting Psalm to a very high level generates many errors, too many for us, so we baselined many of them. That way, new code is held to a very high standard, and we can reduce the baseline progressively and make old code also comply with high standards. In your case, it's OK to remove stuff from the baseline if your changes somehow fix them, or make them no longer relevant.

Assumed that was the case, thanks for taking the time to explain :-)

[wtfzdotnet]

phpstan happy, psalm happy, phpcs happy, I hope we are finally good now 🙏

[wtfzdotnet]

Let me know if I still need to squash these commits, sorry about the going back and forth ❤️

[greg0ire]

Let me know if I still need to squash these commits, sorry about the going back and forth heart

No worries. I think there should be 2 commits:

• one commit that adds the assertions and changes the baseline
• the commit with the changes you wanted to add

That's because right now, I don't think there is a relation between the 2 changes. Let me know if you need help going from 3 to 2 commits.

[wtfzdotnet]

Let me know if I still need to squash these commits, sorry about the going back and forth heart

No worries. I think there should be 2 commits:

• one commit that adds the assertions and changes the baseline
• the commit with the changes you wanted to add

That's because right now, I don't think there is a relation between the 2 changes. Let me know if you need help going from 3 to 2 commits.

@greg0ire okay that's done as well 😓

[greg0ire]

The code looks good, I think. The commit message should be improved, so you should probably read https://www.doctrine-project.org/contribute/#crafting-meaningful-commit-messages

Specifically, the commit message only mentions the problem you're addressing, but does not detail what happens (do you have an error message? Or nothing happens. Also, it does not explain why why the issue exists, what you are doing in this PR, and why that fixes the issue.

Maybe what's happening is that at some point, the ORM is comparing objects with their representation in the database, when it should really convert the objects first, is that it?

[wtfzdotnet]

The code looks good, I think. The commit message should be improved, so you should probably read https://www.doctrine-project.org/contribute/#crafting-meaningful-commit-messages

Specifically, the commit message only mentions the problem you're addressing, but does not detail what happens (do you have an error message? Or nothing happens. Also, it does not explain why why the issue exists, what you are doing in this PR, and why that fixes the issue.

Maybe what's happening is that at some point, the ORM is comparing objects with their representation in the database, when it should really convert the objects first, is that it?

As was in the example I've given before with the gnarly ramsey/uuid implementation ( for the test case ), basically what happens is a custom doctrine type of Uuid object is converted to string by simply casting, resulting in a hex DELETE FROM x WHERE id = ? query, whilst it should've been converted along the way to it's binary representation ( in our database choice case ). This leads to no deletions being made at all as you would expect making use of doctrine custom type's as an identifier.

[wtfzdotnet]

Rewrote last commit's message to address this.

[norkunas]

And #10075 just was ignored, thanks guys

[greg0ire]

Thanks @wtfzdotnet !

@norkunas , that's unfortunate your PR got ignored, but I don't see why you feel entitled to complain about it. I mean, we're not paid to review all incoming PRs, are we?

[norkunas]

I don't complain. I stated a fact, and then thanked. At least after so long somebody gave a duck and completed this

[greg0ire]

If you don't complain then we're cool 🙂 We in the team get drowned daily in a shitload of notifications, and it's indeed cool that people don't get discouraged by this and stuff ends up getting fixed. So thank you both for caring!