JSON:API assumes entity reference field's main property must be the entity ID

Ok, moved it to an issuefork so we can get tests.

Although this seems fine, we do need some testing around this. I guess testing the 2 changed methods might be good enough, but not entirely sure yet how to do that.

This has a failing test now on MR 11771

Seems there has already been a review.

Can the branches/MRs be cleaned up some for easier reviews

Thanks!

Discussed with @tstoeckler in person who has run into similar issues using jsonapi. His feeling was the the branch where we've started adding the test is the best way to move forward - basically assuming that all entity reference fields have an "entity" computed field that we can use to associate the loaded entity

pwolanin → changed the visibility of the branch 3476224-jsonapi-assumes-entity to hidden.

pwolanin → changed the visibility of the branch 3476224-target-id-only to hidden.

Working on some more test coverage and path to patch the addition of a relationship.

This adds more test coverage for adding relationships when target_id is not the main property. Interestingly, I stumbled upon the fact that JsonApiFunctionalTest does some very similar tests, but with more traditional entity reference fields. Of the 2 scenarios I've added, the first was the only one that actually didn't presently work. The second already worked but I added it so we don't have a regression in that scenario.

Yes, thanks for managing the MRs @pwolanin that makes it more manageable. Yes, I agree with the proposed resolution. The only way this would be a breaking change would be if a custom entity reference implementation either did not provide a computed entity property at all - which would (among lots of other things!) break the entity query joining across the entity reference while having absolutely no benefit - or did provide one but did not manage the sync from the entity property to its target ID property - which just seems like a straight up bug. So I don't think this should be a reason to hold this up, in particular since this is an actual contrib blocker. I can't really imagine that there are actually people impacted by this, but will write a change notice for this just in case anyway.

And will do a proper code review afterwards.

Did not intend to unassign, apologies.

In starting to write the change notice and looking at some of the code I found that - in addition to the current assumption of the main property that is discussed here - JSON:API currently also already assumes a computed entity property that is specifically called 'entity'. See \Drupal\jsonapi\IncludeResolver::resolveIncludeTree(). So what I said above is not true, there is no change that this property is required - because it already is. I think that is further evidence that the proposed approach is the correct and this is just consolidating different assumptions in different places to one assumption.

For reference. This was the commit:
🐛 ResourceTypeRepository wrongly assumes that all entity reference fields have the setting "target_type" Fixed : Issue #3057545 by acbramley, hchonov, bbrala, bradjones1, larowlan, yogeshmpawar, Leon Kessler, gease, joachim, gabesullice, kfritsche, jibran, Wim Leers, Berdir, smustgrave, alexpott, catch: ResourceTypeRepository wrongly assumes that all entity reference fields have the setting "target_type"

Thanks to the helpful input. I've addressed all feedback on the MR. I re-tested if scenario #2 still works before/after this change and that is still the case. All that wasn't working is scenario #1.

Wow, what awesome turnaround, thanks a lot. Looked through the changes and it looks great to me. I did spend some more time looking at the other usages of getMainPropertyName(), but I'm not yet convinced there's actually anything actionable there, so not opening a follow-up issue for that. So there's really nothing left to do here from my point of view.

Hmm... strange, don't really understand why the Nightwatch tests would fail, but I guess would be good to get a green pipeline before this goes in.

Nightwatch tests fail sometimes ramdomly, at that point, just hit retry and they will succeed most of the time.