Problem/Motivation

SectionComponent is passed a Boolean named $in_preview, which indicates whether or not the component is being previewed or being shown at runtime.
When it is being previewed, access on the plugin is not consulted, as all components should be shown for rearranging.

However, the plugin access is still computed and this can lead to side-effects and possible exceptions if the plugin does not have all the contexts it needs.

For example, when using the paragraph module the ParagraphAccessControlHandler is run for its corresponding FieldBlock.
ParagraphAccessControlHandler assumes that its parent entity will exist in the database, which is not true during preview.

Proposed resolution

Instead of computing access and then ignoring it, skip computing the access when in preview.

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#7 2942600-preview_access-7-PASS.patch4.6 KBtim.plunkett
#7 2942600-preview_access-7-FAIL.patch2.49 KBtim.plunkett
#4 2942600-preview_access-4.patch4.6 KBtim.plunkett
#4 2942600-preview_access-4-interdiff.txt590 bytestim.plunkett
#2 2942600-preview_access-2.patch4.6 KBtim.plunkett
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett created an issue. See original summary.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett at Acquia commented
Status: Active » Needs review
FileSize
2942600-preview_access-2.patch4.6 KB

Status: Needs review » Needs work

The last submitted patch, 2: 2942600-preview_access-2.patch, failed testing. View results

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett at Acquia commented
Status: Needs work » Needs review
FileSize
2942600-preview_access-4-interdiff.txt590 bytes
2942600-preview_access-4.patch4.6 KB

Ugh, reminder to not add code after running tests, without rerunning them again.

EclipseGc’s picture

EclipseGc CreditAttribution: EclipseGc at Acquia commented
Status: Needs review » Reviewed & tested by the community

This looks super reasonable to me.

Eclipse

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Issue tags: +Contributed project blocker 
  1. +++ b/core/modules/layout_builder/src/SectionComponent.php
@@ -103,11 +104,18 @@ public function toRenderArray(array $contexts = [], $in_preview = FALSE) {
-      $access = $plugin->access($this->currentUser(), TRUE);
...
+        $access = $plugin->access($this->currentUser(), TRUE);

    This is logically the same, but now only calculated when $in_preview is not true, so that's cool too (and this is the bugfix). 

  2. +++ b/core/modules/layout_builder/src/SectionComponent.php
@@ -103,11 +104,18 @@ public function toRenderArray(array $contexts = [], $in_preview = FALSE) {
-      $cacheability = CacheableMetadata::createFromObject($access);
+      $cacheability = CacheableMetadata::createFromObject($plugin);
...
-        $cacheability->addCacheableDependency($plugin);
...
+      $cacheability->addCacheableDependency($access);

    Also confusing, but logically the same. 

  3. +++ b/core/modules/layout_builder/src/SectionComponent.php
@@ -103,11 +104,18 @@ public function toRenderArray(array $contexts = [], $in_preview = FALSE) {
+      // Only check access if the component is not being previewed.
+      if ($in_preview) {
+        $access = AccessResult::allowed()->setCacheMaxAge(0);

    This comment scared me at first, but then @tim.plunkett pointed out that this is already the logic when $in_preview is true.

I asked @tim.plunkett to post a test-only patch. Leaving at RTBC since that is not an actual change that would need to be re-reviewed.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett at Acquia commented
FileSize
2942600-preview_access-7-FAIL.patch2.49 KB
2942600-preview_access-7-PASS.patch4.6 KB

Posting the test only patch, in addition to the identical patch from above.
The expected fail should be

    $block->access($this->account->reveal(), TRUE)->shouldNotBeCalled();

It should not be called, but it will be in HEAD.

The last submitted patch, 7: 2942600-preview_access-7-FAIL.patch, failed testing. View results

  • xjm committed 97a4389 on 8.6.x 
    Issue #2942600 by tim.plunkett: SectionComponent::toRenderArray() runs...
xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Version: 8.6.x-dev » 8.5.x-dev

Committed and pushed to 8.6.x, thanks!

This is a contrib blocker so marking RTBC against 8.5.x for backport once the freeze for beta is over.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 7: 2942600-preview_access-7-PASS.patch, failed testing. View results

EclipseGc’s picture

EclipseGc CreditAttribution: EclipseGc at Acquia commented
Status: Needs work » Reviewed & tested by the community

This still applies to 8.5.x so not sure what the deal with testbot is. Re-rtbcing.

Eclipse

EclipseGc’s picture

EclipseGc CreditAttribution: EclipseGc at Acquia commented
Issue tags: +Layout Builder stable blocker
tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett at Acquia commented
Priority: Normal » Major

This is major as a contributed project blocker.

  • xjm committed 08fbb0b on 8.5.x 
    Issue #2942600 by tim.plunkett: SectionComponent::toRenderArray() runs...
xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Status: Reviewed & tested by the community » Fixed

Now cherry-picked and backported to 8.5.x as well. Thanks!

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett at Acquia commented
Component: layout.module » layout_builder.module

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.