- Issue created by @pwolanin
- First commit to issue fork.
- π³π±Netherlands bbrala Netherlands
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.
- First commit to issue fork.
- Status changed to Needs work
4 months ago 5:23pm 8 April 2025 - πΊπΈUnited States smustgrave
Seems there has already been a review.
Can the branches/MRs be cleaned up some for easier reviews
Thanks!
- πΊπΈUnited States pwolanin
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
- πΊπΈUnited States pwolanin
pwolanin β changed the visibility of the branch 3476224-jsonapi-assumes-entity to hidden.
- πΊπΈUnited States pwolanin
pwolanin β changed the visibility of the branch 3476224-target-id-only to hidden.
- heddn Nicaragua
Working on some more test coverage and path to patch the addition of a relationship.
- heddn Nicaragua
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.
- π©πͺGermany tstoeckler Essen, Germany
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.
- π©πͺGermany tstoeckler Essen, Germany
Did not intend to unassign, apologies.
- π©πͺGermany tstoeckler Essen, Germany
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. - π³π±Netherlands bbrala Netherlands
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" - heddn Nicaragua
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.
- π©πͺGermany tstoeckler Essen, Germany
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. - π©πͺGermany tstoeckler Essen, Germany
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.
- π³π±Netherlands bbrala Netherlands
Nightwatch tests fail sometimes ramdomly, at that point, just hit retry and they will succeed most of the time.
- Assigned to heddn
- Status changed to Downport
about 2 months ago 6:32pm 13 June 2025 -
alexpott β
committed 71cc5fd8 on 11.x
Issue #3476224 by pwolanin, heddn, bbrala, tstoeckler: JSON:API assumes...
-
alexpott β
committed 71cc5fd8 on 11.x
- πΊπΈUnited States xjm
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 assumptions as per the CR).
Crediting @alexpott for the previous commit; he was uncredited somehow.
- πΊπΈUnited States xjm
@catch also said in Slack that he thought it was RC-safe FWIW; crediting RM reviews.
-
alexpott β
committed 8d70081b on 11.2.x
Issue #3476224 by pwolanin, heddn, bbrala, tstoeckler: JSON:API assumes...
-
alexpott β
committed 8d70081b on 11.2.x
- πΊπΈUnited States pwolanin
Thanks!
We have been using a variant on this patch for a long time in production, would love to see it bakcport to 10.x so we don't have to keep applying a core patch.
-
xjm β
committed 45a4cd9d on 10.6.x authored by
alexpott β
Issue #3476224 by pwolanin, heddn, bbrala, tstoeckler: JSON:API assumes...
-
xjm β
committed 45a4cd9d on 10.6.x authored by
alexpott β
- πΊπΈUnited States xjm
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.) π
- πΊπΈUnited States xjm
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.
- Merge request !12487Issue #3494194 by plopesc, smustgrave, quietone: Menu Link Edit form is not... β (Closed) created by pwolanin
- πΊπΈUnited States xjm
The merge request might need another attempt. :)
- πΊπΈUnited States pwolanin
It looks like that static method
EntityTestHelper::createBundle()
replaced a module functionentity_test_create_bundle()
in π Move entity_test_create_bundle from entity_test.module Needs work in 11.x, so for the backport we can switch it back. - πΊπΈUnited States pwolanin
Fixing the backport was just that 1-line fix to use the prior version of that test code helper function
Automatically closed - issue fixed for 2 weeks with no activity.