Layout builder fails to assign inline block access dependencies for the overrides section storage on entities with pending revisions

This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request → as a guide.

Can the MR be updated for 10.1 please.
Left some comments on the MR 1106 also.

If we do end up changing the parameters for the eventSubscriber we will need a change record also.

Hiding the files to avoid confusion as the solution is being worked on in the MR.

The merge request does not appear to work in the scenario where a media item is being added to a field in an inline block.

The problem is that this line does not work with future revisions:

      $layout_entity = $this->entityRepository->getActive($layout_entity_info->layout_entity_type, $layout_entity_info->layout_entity_id);

We either need a different method to get the newest revision instead of the active revision, or pass the appropriate context definition to getActive() so it gets the correct revision.

I've run into a similar problem where the block_content entity has translations (because of core patch which adds symmetric translations to layouts). The media library widget was unable to update the media file once a translation was created. It would only work for the translation, but not the original language.

Checking block usage with latest revision ID of the block_content entity won't work, because the section storage stores the latest translation affected revision ID in the layout. I'm not sure if this is a problem which should be taken into account here or solved elsewhere. But considering that layout_builder_st should at some point, it might be worth fixing it here.

I've attached a patch which uses getLatestTranslationAffectedRevisionId on the entity storage to work around this (requires changes from the MR to be applied first). I can push it to the MR if this is useful, but since it needs entityTypeManager I wanted to check first if there is a better solution to the problem.

The merge request does not appear to work in the scenario where a media item is being added to a field in an inline block

Could you elaborate or provide steps to reproduce?

Unrelated, but after spending a lot of time reviewing this problem area, isn't the best solution here for layout builder to track what entity revision ID an inline block is attached to, instead of just its ID? If we had that in place, then there wouldn't be a need for us to have all this logic in place to check if we should be trying to load the canonical revision vs the active revision. We'd know exactly which revision to load. Maybe that's not compatible with translations or something?

I guess that's a bit more complicated to accomplish and requires more refactoring though.

The problem is that this line does not work with future revisions:

$layout_entity = $this->entityRepository->getActive($layout_entity_info->layout_entity_type, $layout_entity_info->layout_entity_id);

We either need a different method to get the newest revision instead of the active revision, or pass the appropriate context definition to getActive() so it gets the correct revision.

That code should be getting the latest revision. If you have a future revision, isn't that considered the latest revision? In what scenario is it not?

@bkosborne getActive will get the latest draft/translation - the naming isn't great, but the docs on the interface are good.

Rebased off 11.x

As the view operation only assertTrue if the route has option "_layout_builder". So, the expected is to assertFalse for view and True for update and delete.
@larowlan correct me if I'm wrong.
Please review.

Not sure the change to false should be done. Seems like an odd test if you can update/delete but can't view.

I believe this should be re-triaged as major, if not critical.

Once content enters this state, it becomes impossible for content managers to update certain parts of it unless:

1. The content is saved as a default revision
2. The troublesome blocks are removed and replaced with new blocks end-user confirmed, but still data loss?

https://www.drupal.org/project/drupal/issues/3053881 → -- which I was also involved in -- was promoted to critical due to the same sort of potential for a "data-loss-like" condition.

As a follow-up to my triage recommendation, this flaw can be reproduced via only core modules in common configurations.

1. install layout builder, node, media, media library, and content moderation
2. Add a block type with a media reference field, assigning the media library widget in the form display settings
3. enable layout builder on the article content type and enroll it in content moderation
4. Create a new node and edit the layout, adding an inline block that references a media item
5. Save as draft
7. Edit the inline block and attempt to swap out the media, notice that it fails with an AJAX error.

Wonder if the media issue is related to 🐛 Block content permissions required to select or upload new Media with media library when using Layout Builder Active ? What is the AJAX error?

The AJAX error contains...

non-reusable blocks must set an access dependency for control

when going to insert the new media.

Yow. I can reproduce #98 as user 1 with full admin privileges.

JS console error:
ResponseText: {"message":"Non-reusable blocks must set an access dependency for access control."}

Watchdog:

Path: /media-library?destination=/node/4/layout&_wrapper_format=drupal_ajax&ajax_form=1&media_library_opener_id=media_library.opener.field_widget&media_library_allowed_types%5Bimage%5D=image&media_library_selected_type=image&media_library_remaining=1&media_library_opener_parameters%5Bfield_widget_id%5D=field_image%3A-settings-block_form&media_library_opener_parameters%5Bentity_type_id%5D=block_content&media_library_opener_parameters%5Bbundle%5D=image&media_library_opener_parameters%5Bfield_name%5D=field_image&media_library_opener_parameters%5Bentity_id%5D=2&media_library_opener_parameters%5Brevision_id%5D=2&hash=RZpw_Ue-CORgfs34fAruBGXBLZUhx_qIoGwlq0UPRZU&views_display_id=widget&_wrapper_format=drupal_ajax.

Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException: Non-reusable blocks must set an access dependency for access control. in Drupal\Core\Routing\AccessAwareRouter->checkAccess() (line 118 of /var/www/html/public/core/lib/Drupal/Core/Routing/AccessAwareRouter.php).

Current MR applies to 10.1.2 and fixes it. Not able to test in 11.x at present sorry.

Addressed all remarks on the MR.

Some small change requests. Also for the deprecation will need a change record and that link pasted into the deprecation messages.

Thanks for picking this one back up!

Note that there are at least two contrib modules (just in our current project) that will need changes to avoid "Declaration must be compatible with Drupal\layout_builder\EventSubscriber\SetInlineBlockDependency::getInlineBlockDependency(Drupal\block_content\BlockContentInterface $block_content, string $operation): ?Drupal\Core\Access\AccessibleInterface":

• Layout Builder Asymmetric Translation → (patch at https://www.drupal.org/project/layout_builder_at/issues/3090261 🐛 SetInlineBlockDependency override might no longer be needed. Needs work )
• Workspaces Extra → (no patch yet)

I have tested the latest version of the MR and it works well in Drupal 10.1.7

Uploading the latest version of the MR in a patch.

Changed the version by mistake.

As mentioned in #106, I currently have an issue with:

"Node access should be granted, but the node is assigned to NULL, since the associated revision IDs are checked against the default entity, not the active entity."

I was able to reproduce the bug and the patch solved my issue on a vanilla install.

However, on different projects where we are using the module Layout Builder Asymmetric Translation → , we can't use this current patch.

Hello there.
This might be related: https://www.drupal.org/project/drupal/issues/3442910 🐛 Error "Non-reusable blocks must set an access dependency for access control." with layout builder and media library Active
I hope it helps.

This bug does actually affect Media library fields in Blocks as well, 🐛 The media library should perform access checks against the revision of the entity being edited Fixed did not fix it.

While the Block Content revision is now loaded in MediaLibraryFieldWidgetOpener, and ->access() is called on the revision, SetInlineBlockDependency::getInlineBlockDependency still just loads the default revision of the parent of the block (e.g the Node) and checks if that block revision exists in the Node, which it doesn't when you're editing a forward revision of the Node (because that'll have different block revision ids). In fact, 🐛 The media library should perform access checks against the revision of the entity being edited Fixed probably caused this regression with media library because it used to check the default revision of the block which would have been in the default revision of the node!

Rebased, fixed tests and linting, added CR. This is ready for review.

@acbramley - The latest updates done are making this MR diff (applying it as a patch) not work now with Drupal 10 environments. When trying to apply the diff as a patch on Drupal 10.4.1, Composer throws an error saying that the patch cannot be applied.

For anyone needing to apply this as a fix for Drupal 10, I've been able to go and create a patch file that can be used.

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Hiding patches so they don't trip the bot.

In an attempt to refactor the complicated getInlineBlockDependency function, I noticed there was no test coverage added for the code that was inside the case when $layout_entity_info was empty.

This code was added in the following 2 commits

https://git.drupalcode.org/project/drupal/-/merge_requests/1106/diffs?co...
https://git.drupalcode.org/project/drupal/-/merge_requests/1106/diffs?co...

I tried to debug various ways of adding/updating blocks with media library in them and hit a breakpoint inside that code but I was unable to.

The first commit mentions fixing #98 and #101 but, again, I was unable to hit that code when following those steps. That is, without that code I am able to successfully update a media reference via media library on an existing block in a draft revision of a node.

Without test coverage, it's hard to figure out what the code was actually doing it so I have reverted it.

If anyone is able to provide steps to reproduce the issues that this was fixing, and those steps still fail on the latest version of this branch, please let me know!

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.