Fetching entities with Composite Key Relations and null values

[michalbundyra]

Recreation of #8425 as it was closed but never actually resolved and the issues still persist.

Description from the previous PR (Background):

In our database design we are using entities with composite keys, and we are using composite keys relations between entities.

Let say we have the following entities with Primary Keys (PK):

• Company (PK: company_code)
• Customer (PK composite: company_code + customer_code)
• Invoice (PK composite: company_code + invoice_code)

and the following relations in Invoice:

• to Company (using company_code), not nullable
• to Customer (using composite key company_code + customer_code), nullable

Now we have company_code (not null) but no customer_code (null).
We want to fetch Invoice entity, so that we get not null Company, and null Customer.

The problem is that loading Invoice with null Customer is not working - we are getting the following exception:

Exception: [Doctrine\Common\Proxy\Exception\OutOfBoundsException] Missing value for primary key code on Doctrine\Tests\Models\CompositeKeyRelations\CustomerClass

This PR provide a fix, to load the Customer in the described case.
The change is pretty simply, and no other tests are affected.

NOTE: Loading Company is not a problem as it is using relation on just one column (not a composite key). The description includes it just for completeness of the example.

Workaround

We have discovered that we could use one of these workarounds - but both are not perfect:

• do not use null field, use instead empty value (empty string)
• use fetch=EAGER, what might be not efficient and actually might be not needed

Solution

The solution proposed in this PR is very minimal - just removal od one of the condition, which seems to be not covered, and I couldn't find track why it has been even added there. Based on the example provided in this PR, the removed condition seems to be invalid.

PR contains two commits:

• tests with failing scenario
• fix for the presented issue

Additional considerations

The following comment confused me a lot:

orm/src/UnitOfWork.php

Line 2456 in 9d4f54b

// TODO: Is this even computed right in all cases of composite keys?

and I did additional investigation if the proposed solution here is correct.
In the below we are removing just first part of the condition:

orm/src/UnitOfWork.php

Lines 2470 to 2476 in 9d4f54b

	} elseif (
	$targetClass->containsForeignIdentifier
	&& in_array($targetClass->getFieldForColumn($targetColumn), $targetClass->identifier, true)
	) {
	// the missing key is part of target's entity primary key
	$associatedId = [];
	break;

and I was wonder if the target entity could/should know if it is a part of a FK somewhere else - and I can't even see the reason behind that, so I believe this is the correct fix as it is the case when the filed "is part of target's entity primary key".

Hope it all makes sense and we can get it in. Thanks 🙌

[darrenedale]

This would definitely help with some of our use cases. 👍

[greg0ire]

Does this affect 3.x only? If not, you should target 2.19.x and then we merge this up.

[michalbundyra]

@greg0ire thanks, this is also a problem on 2.19.x, so I have recreated the PR here: #11506

Rebasing this one is quite tricky, as 2.19.x is still using annotations, support older PHP versions and has slightly different CS rules.

Thanks

[SenseException]

I assume this will wait until the 2.19 branch gets upmerged before the work of this PR can proceed

[greg0ire]

Here we go: #11525

[michalbundyra]

@greg0ire thanks a lot! 🎉

Sorry for asking but any ETA when it will be actually released? Thanks 🙏🏻

[greg0ire]

No worries, but why did you close this? Did I maybe cover everything you wanted to do in my merge up? I can publish a release when that's cleared up.

[michalbundyra]

@greg0ire thank you - yes, correct - your merge up from 2.19.x covered this completely PR completely (the fix was exactly the same in 2.19.x as it is here, just the difference with annotations->attributes in tests).

Just checked, and the only difference (between this and your merge up) was importing the models in tests:

vs

Thanks again! 🙌