Problem/Motivation
Draft mode returns wrong revision of includes. I get default revision of second level paragraph instead of latest revision.
Technical problem:
When trying to access the working revision entity data with resourceVersion=rel:working-copy and using includes, the current revision of the referenced entities is being returned by the method "resolveIncludeTree" in IncludeResolver class. This method should return the data as per the revision being passed in the API query or the working revision.
Steps to reproduce
Install Content moderation;
Add support version negotiation for any entity type: "https://git.drupalcode.org/project/drupal/-/merge_requests/1365.patch"
Create paragraph type A;
Create paragraph type B;
Add paragraph reference field to paragraph A;
Create node with paragraph A (+ paragraph B inside A)
Publish node;
Create draft with changed B;
Check node json data with resourceVersion=rel:working-copy. Returned wrong revision of includes.
Proposed resolution
web/core/modules/jsonapi/src/IncludeResolver.php
loads included entity by id
$targeted_entities = $entity_storage->loadMultiple(array_unique($ids));
I suggest to load entities by revision id:
$targeted_entities = $entity_storage->loadMultipleRevisions(array_unique($revision_ids));
Comment | File | Size | Author |
---|---|---|---|
#50 | 3088239-50.patch | 3.32 KB | jkdaza |
Issue fork drupal-3088239
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #2
druellan CreditAttribution: druellan commentedI'm having the same issue. Using ?resourceVersion=rel:working-copy or ?resourceVersion=id:1110 JSON:API returns the correct revision of the node and all the fields, BUT all referenced entities including paragraphs are returned as the "Current Version".
Comment #3
Wim LeersComment #4
druellan CreditAttribution: druellan commentedThis is our quick fix for this problem, near the line 145, replacing the Entity::loadMultiple, making sure we are using target_revision_id for revisions.
Sorry I have no patch for this, we messed a lot with the file, since we faced also a permissions problem, but this snippet is actually working.
Comment #5
pratik_kambleComment #6
abhisekmazumdarI will work on this.
Comment #7
abhisekmazumdarAfter some digging around I found out that there an issue Add a version negotiation to revisionable resource types
An admin/authenticated doesn't have the proper permissions to get the revision data.
Comment #8
johnennew CreditAttribution: johnennew at Deeson commentedThis patch is only a temporary solution but may be of use to people that need something now before the API first initiative can provide the standard entity revision access checks.
This patch allows revisions of included entities to be loaded, so the correct revision based on what is referenced by the revision of the entity being requested.
This patch also provides a new hook which allows module developers the ability to dangerously provide their own custom revision access logic for entities which are not nodes or media. By default other entities, for example the popular paragraphs module, will always come back as an access denied via JSON API unless its referencing the default version.
After applying this patch, you may need to implement your own `hook_jsonapi_revision_view_access()` function. You will see that the patch adds the paragraphs module example into the jsonapi.api.php file which you can copy straight into one of your own custom modules (renaming the function appropriately and clearing caches first of course!)
This patch is for 8.8.x branch.
Comment #9
johnennew CreditAttribution: johnennew at Deeson commentedAnother patch which hopefully will stop the 500 errors from the tests (can't seem to run all the tests locally for some reason).
Comment #10
Wim LeersI hate to say this but … 😞
We specifically don't want to add more JSON:API-specific APIs to Drupal core. We need to add the missing APIs in Entity API.
See #3035113: JSON:API is typehinting LatestRevisionCheck because there is no Entity Revision Access API yet, remove that typehint once such an API exists. This is a duplicate of that AFAICT. Or am I mistaken?
Comment #11
johnennew CreditAttribution: johnennew at Deeson commentedYeah, I didn't expect this to go in - I just needed to host the patch file for people that need a solution now!
Comment #13
johnennew CreditAttribution: johnennew at Deeson commentedThis still doesn't address the comments in #10.
Patch attached improves 9 by allowing sub referenced paragraphs to come through the json API i.e. where you might have a paragraph referencing a paragraph - JSON API by default seems to forget its dealing with revisions after its gone two deep in the include tree. Can't tell if this is Paragraphs fault or not. Paragraphs seem to implement RevisionableInterface but $resource_type->isVersionable() when wrapping a paragraph always says no :(
Comment #14
drup4lNub CreditAttribution: drup4lNub commentedAlright! I need your help. So I have content types with paragraph types nested inside them. But when I try to access a draft version of the content fields inside the paragraph type it's not returning any data. I came across this thread and applied a few of these patches but I'm not seeing a change in the api call. (I'm using postman for testing this). am I missing something?
Comment #15
johnennew CreditAttribution: johnennew at Deeson commentedRerolled patch for Drupal 8.9.4 attached.
drup4lNub - did you implement the hook mentioned in the patch?
you'll need something like the code below - note by doing this you are explicitly overriding core behaviour to provide access control to non node / media entities (core blocks access to all revisions of paragraphs via JSON API):-
Comment #17
Ara Martirosyan CreditAttribution: Ara Martirosyan commentedThank you johnennew your patch of comment #13 saved my life
Comment #18
iivanov CreditAttribution: iivanov commentedReapplied patch for Drupal version 9.1.4.
Replaced `\Drupal::moduleHandler()` with Module Handler container - added as a service in `EntityAccessChecker.php` class.
Removed `hook_jsonapi_revision_view_access` from `jsonapi.api.php` file and placed in `jsonapi.module` file - in order to be able to invoke the hook.
Comment #21
acbramley CreditAttribution: acbramley at PreviousNext for Service NSW commentedAlong with #3031271: Support version negotiation for any entity type (currently only Node & Media are supported) I was able to get forward revisions of Node -> Paragraphs through JSON:API with just the IncludeResolver changes. I agree that these still need finessing but this is a good step forward.
Comment #22
quadrexdevI can confirm that the patch from #21 within the changes from #3031271: Support version negotiation for any entity type (currently only Node & Media are supported) issue works for my project.
Comment #24
bbralaThis would be great to get this working as advertised and really happy it already seems to work! To get a clear view of what we are doing here i'd like to see a few things:
The use cases might just be something like:
Structure: We have a article with 2 reference fields (foo and bar) to pages.
State: A published node with a forward revision with 1 foo reference to a published page, and 1 bar reference to a published page with a forward revision.
Expect:
When asking for the forward revision jsonapi should send the forward revision of the article, current revision of the foo reference, and the forward revision of the bar reference.
When asking for the current revision jsonapi should senf the current revision of the article, the foo reference and bar reference.
This way we can describe this change, and someone with testing skills could write some tests :)
Comment #25
Kristen PolSeems to be a similar issue and patch but I don't know which is better. Ideally, we'd close one and continue work on the other.
#3291095: Content moderation + json api includes wrong revision issue
Comment #26
Rakhi Soni CreditAttribution: Rakhi Soni at Smashing Infolabs Pvt. Ltd. for Smashing Infolabs Pvt. Ltd. commentedAttached rerolled against branch 9.5x,,
Kindly review patch
Comment #27
bbralaThanks for the reroll, but this issue also needs a few updates as mentioned in #24.
Comment #28
bnjmnmThe reroll from @Rakhi Soni was not necessary and no credit will be granted.
Here's how to see if a reroll is necessary:
Find the most recent patch and choose "add test/retest"
In the Add test / retest queue choose the most recent branch and click the "queue" button to run the tests against the current dev branch.
If you don't get an error that the patch didn't apply, then a reroll isn't needed, the patch is able to apply to the current dev branch.
Comment #30
bbralaAdding some credit from #3291095: Content moderation + json api includes wrong revision issue. There is.
Below the feedback on the code of the related issue.
Compared to this issue i like this code better.
We do still need tests, and we will need a change record too.
Comment #31
pooja saraah CreditAttribution: pooja saraah at Material for Drupal India Association commentedThanks for your suggestions @bbrala
I have attached Patch Against Drupal 9.5.x
Comment #32
ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commented@pooja saraah, now I think
$field_item_properties
variable is unused and can be removed.also, the use statement should be there for
EntityReferenceRevisionsItem
class.Comment #33
varshith CreditAttribution: varshith as a volunteer and commentedI had this with paragraph references and fixed it before coming across this issue. While the core idea of the fix is the same, the recent patch is missing a 'use' statement and has an unused variable. So I am attaching my patch anyway for review. Thanks.
Comment #34
ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commented@varshith thanks for the patch, please provide the interdiff as well.
Comment #35
varshith CreditAttribution: varshith as a volunteer and commentedAdding interdiff
Comment #37
Rassoni#33 Re-roll against 10.1.x-dev branch. Patch applied successfully, but custom command failing. required NW
Comment #38
ameymudras CreditAttribution: ameymudras at Salsa Digital commentedI just had a look at #33 and I'm not sure why we are using
Comment #41
mstrelan CreditAttribution: mstrelan at PreviousNext for Service NSW commentedStarted to re-roll for 11.x but the changes to
\Drupal\jsonapi\IncludeResolver::resolveIncludeTree
from #3057545: ResourceTypeRepository wrongly assumes that all entity reference fields have the setting "target_type" have complicated this.Comment #42
Nadim Hossain CreditAttribution: Nadim Hossain as a volunteer and at Department of Premier and Cabinet - Victoria, Australia commentedRe-rolled the patch against 10.2.x, still testing needs to be done on this re-rolled patch.
Comment #43
acbramley CreditAttribution: acbramley at PreviousNext for Service NSW commentedWe can't reference classes from entity_reference_revisions in core, this needs a reroll starting from #21
Comment #44
Nadim Hossain CreditAttribution: Nadim Hossain as a volunteer and at Department of Premier and Cabinet - Victoria, Australia commentedThanks @acbramley
I have re-rolled it from #21 against 10.2.x
Comment #45
mstrelan CreditAttribution: mstrelan at PreviousNext for Service NSW commentedI don't think the re-roll is that simple. Now we're only adding the
revision_ids
andids
bit to$references
if theis_subclass_of
condition fails.Comment #47
mstrelan CreditAttribution: mstrelan at PreviousNext for Service NSW commentedOpened MR against 11.x which is a re-roll of #21, but moves bits out of the
is_subclass_of
condition. This also applies to 10.2.x.Comment #48
jberube CreditAttribution: jberube commentedAs mentioned in #45, the patch for v10.2.x in #44 doesn't cover both conditions in the if/else. Using that patch, I wasn't seeing the results I expected, so I made this patch that covers both conditions, and now I see the results I expected.
Comment #49
acbramley CreditAttribution: acbramley at PreviousNext for Service NSW commentedHiding patches to avoid bot confusion.
Comment #50
jkdaza CreditAttribution: jkdaza at Agile Collective commentedPatch #48 works for me, but
assumes that the
$target_type_and_rev
will always have 2 values. This is not the case, so you end up with tons of warnings>I've changed it to
and no more warnings.
Attaching the revised patch.