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.