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!

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:
#3057545: ResourceTypeRepository wrongly assumes that all entity reference fields have the setting "target_type": 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.

Committed 71cc5fd and pushed to 11.x. Thanks!

As we're in the RC phase going to backport this to 11.2.x once 11.2.0 is out.

Thanks everyone!

Contributed project blockers are generally major by default. Also, even if it weren't a contrib blocker, the bug sounds major to me on its own.

I think this is safe to backport during RC (and I would actually prefer it before 11.2.0 rather than in a patch release given the small internal change to behavior and assumptions as per the CR).

Crediting @alexpott for the previous commit; he was uncredited somehow.

@catch also said in Slack that he thought it was RC-safe FWIW; crediting RM reviews.

Thanks @xjm and @catch for considering this for backport. Committed 8d70081 and pushed to 11.2.x.

It's not patch-eligible, but @catch and I did agree on backporting it to 10.6 since it's a major bug and a contributed project blocker, so it will at least be addressed with the December release. (Of course the bug will also go away when the site is upgraded to Drupal 11.) 😉

Looks like it's not a clean backport, unfortunately:

  Line   core/modules/jsonapi/tests/src/Functional/JsonApiRelationshipTest.php  
 ------ ----------------------------------------------------------------------- 
  52     Call to static method createBundle() on an unknown class               
         Drupal\entity_test\EntityTestHelper.                                   
         💡 Learn more at https://phpstan.org/user-guide/discovering-symbols    
 ------ ----------------------------------------------------------------------- 
 [ERROR] Found 1 error    

So we would need a separate MR for backport.

The merge request might need another attempt. :)

Committed the backport to 10.6.x. Thanks!