Allow block and layout plugins to determine if they are being previewed

Tested this with the Facet Block and it works. Good way to determine if rendering in the Preview.

Other suggestion I made to Tim is potentially adding an isEmpty() method to the PreviewFallbackInterface that would use the Element::isEmpty($content) by default but allow custom blocks to have their own logic for “empty”.

Came up in #3002608-74: Remove contextual links not related to layout administration inside layout builder blocks.

+++ b/core/modules/layout_builder/src/Plugin/SectionStorage/SectionStorageBase.php
@@ -106,7 +108,9 @@ public function removeSection($delta) {
+    $contexts = $this->getContexts();
+    $contexts['in_preview'] = new Context(new ContextDefinition('boolean'), TRUE);
+    return $contexts;

Would it make sense to have getContexts() return a FALSE version of 'in_preview', then make getContextsDuringPreview() overwrite that with a TRUE one? That way, external code calling either of these methods doesn't need to check isset($contexts['in_preview']), and there's a consistent way to check if we're in preview mode.

This is working as expected. I've created this custom block type:

namespace Drupal\mymodule\Plugin\Block;

use Drupal\Core\Block\BlockBase;

/**
 * Provides a 'TestBlock' block.
 *
 * @Block(
 *  id = "test_block",
 *  admin_label = @Translation("Test block"),
 *  context_definitions = {
 *    "in_preview" = @ContextDefinition("boolean", required = FALSE),
 *   },
 * )
 */
class TestBlock extends BlockBase {

  /**
   * {@inheritdoc}
   */
  public function build() {
  	$in_preview = $this->getContextValue('in_preview');
  	if ($in_preview) {
  	  $markup = 'Preview of test block.';
  	}
    else {
      $markup = 'This is a fancy test block with lots of context.';
    }

    return [
      '#markup' => $markup,
      '#cache' => [
        'max-age' => 0,
      ],
    ];
  }

And it shows the regular text when viewing, and the preview text when previewing in layout builder.

The problem is, using an annotation to control whether the block cares about being in preview is not easily discoverable by a developer creating block plugins. Would it be possible to implement this with a property on the block base class?

Also, there is a bug in the context setup. With the code as above, where the annotation says required = FALSE, I get a select option in the block config that asks me to "Select a in_preview value" where the options are "- None -" or a blank string. See screenshot with with code inspection:

And if I remove the required = FALSE, the block cannot be placed using the regular block layout, it's only available in layout builder.

Rerolled patch for 8.8.1

fixing some test errors

TestInPreviewContext.php got removed during re-roll in #18

#16 still needs to be addressed

In response to #16: we're going to need a separate issue to fix this.

My recommendation for blocks that need to support both Layout Builder & the classic block layout page is to have a required context definition with a default value. Unfortunately, this isn't currently possible using the boolean data type; we first need to make sure that FALSE is considered a non-empty value for contexts, whether default or explicitly assigned (see this method for more information). (Currently, a default value of FALSE causes \Drupal\Component\Plugin\Context\Context::hasContextValue() to return FALSE.)

General review + updated patch notes:

1. I think it's best that we set the context value in four separate locations:
   1. \Drupal\layout_builder\Entity\LayoutBuilderEntityViewDisplay::getContextsForEntity()
   2. \Drupal\layout_builder\Plugin\SectionStorage\SectionStorageBase::getContextsDuringPreview()
   3. \Drupal\layout_builder\Section::toRenderArray()
   4. \Drupal\layout_builder\SectionComponent::toRenderArray()

   At a minimum, I believe it's most appropriate to unconditionally set the context value according to $in_preview in Section::toRenderArray() and SectionComponent::toRenderArray().

   The rationale behind this decision is to ensure that this context value is always available when using layout builder. Since neither Section::toRenderArray() or SectionComponent::toRenderArray() are internal APIs, they could ostensibly be called from contrib; because of this, I think it's most safe to ensure that a fresh context value is set in these methods. The other two locations that set the context value ensure that the context is available, for example, in the block selection form (since it's filtered by context availability).

2. I've also added some extra test assertions for robustness.

Fix spell check error.

Just posted #3194767: Required contexts with default values should not fail the requirements check in the context handler and #3194768: The plugin system should only consider NULL context values to be empty to facilitate the use of otherwise FALSE values which are blockers for #16 being resolved.

I'd appreciate a review on those issues (and this one) from anyone who has time.

Hi @clayfreeman,
Patch #25, works good for me.
Thanks

Adding tag to request subsystem maintainer review.

Please let me know if the direction taken in the issues referenced in #26 is appropriate. If not, please suggest a recommended approach to finding a solution to the problem identified in #16.

(Slack thread for reference)

Escalating as per What timeframes are given for sign-offs before decisions are escalated?

The primary concerns that needs to be addressed are from #16:

[W]here the annotation says required = FALSE, I get a select option in the block config that asks me to "Select a in_preview value" where the options are " - None - " or a blank string.

[I]f I remove the required = FALSE, the block cannot be placed using the regular block layout, it's only available in layout builder.

It may also be prudent to review the discussions on MR !932 after reading this comment.

I've taken steps to address the concerns in #16 by submitting two additional issues; however, I'm not sure if the direction I've taken is acceptable for inclusion in Drupal core, and if so, I've not been given clear direction regarding the management of this issue.

Since the select box for the in_preview context value only appears when the context definition is not required (which is currently a prerequisite for being able to use the block plugin outside of Layout Builder), I wanted to find a way to be able to make the context definition required so that the select box does not appear, but also still allow the block to be placed outside of Layout Builder. The approach that I ended up taking was a combination of two supplementary changes to Drupal core:

The first of these changes, in #3194767: Required contexts with default values should not fail the requirements check in the context handler, ensures that required context definitions with default values are capable of passing the requirements check. Presently, if you have a required context definition with a default value, it will be ignored when checking requirements which will prevent the block from appearing in the Block layout UI.

The next change, in #3194768: The plugin system should only consider NULL context values to be empty to facilitate the use of otherwise FALSE values, ensures that a default value of FALSE for context definitions is not considered "empty" by Drupal. This is required since by default, in_preview should be considered FALSE unless explicitly set to TRUE by Layout Builder's preview functionality.

I want to clearly state that I'm not married to the approach that I've taken in MR !932 and the two referenced issues. While I think it'd be nice to fix up the requirements check & default value logic, I can live without it if there is a better alternative to getting this issue committed in a way that addresses the concerns in #16. I just need to be given some direction on how this issue should be handled.

In summary, this issue has the following outstanding questions:

1. Is the approach taken in MR !932 for this issue acceptable for addition to Drupal core?
2. If the answer to the previous question is yes, then should the referenced issues be made blockers for this issue?
3. What additional steps should be taken to get this issue ready to be committed?

(Here's a Slack thread concerning this issue & the referenced issues which may provide useful supplementary information.)

Just rebased the existing MR, thanks!

Took a look at this at the request of @clayfreeman - left some comments.

I think that perhaps postponing this in favour of the other two bugs may be the best approach

Postponing this as per @larowlan's recommendation until the two child issues are committed.

In the interim, I'll try to accommodate the review criteria that can be addressed now, and save the remaining criteria for when the other issues are committed.

Thanks for your review!

Thinking of another way to solve this - what if there are more than two states for previewing? e.g. think of how we use view-modes.

If we go with boolean are we locking ourselves into a binary setting if in the future we need a third, fourth or n-th option?

Should we instead have a context definition that has string values, which could be backed by constants in D9 and Enums in D10?

Default
Preview
...Some future state.

Would that also render #3194767: Required contexts with default values should not fail the requirements check in the context handler redundant and solve the impasse there?

At the very least, I think YAGNI comes into play when trying to envision other view modes for Layout Builder. I doubt that we'd want to even consider view modes in Layout Builder other than "preview" and "regular" since view modes are a concept that exist outside of Layout Builder before it is ever invoked for rendering, and this could cause user & developer confusion.

The problem with using the context system is that #3194767: Required contexts with default values should not fail the requirements check in the context handler is ultimately going to be required because of how ContextAwarePluginAssignmentTrait:: addContextAssignmentElement() works. If a context definition is optional, then a context mapping configuration element will be exposed to the end user, allowing them to make blocks unaware of the in_preview context. If a context definition is required, the block will work as expected in Layout Builder, but it cannot be placed outside of Layout Builder unless the referenced issue is committed.

Ultimately, we can't use contexts at all if we consider the referenced issue to be a dead end and also want blocks to be compatible with both Layout Builder and Structure > Block layout. I still think the goal of that issue is valid, and facilitates more flexible control over context definitions.

One alternative I've been able to come up with is sort of a "caveman" approach to the problem, which would be creating a centralized Layout Builder service to track whether preview mode has been entered for the current request. Blocks can then use this service during rendering to check if they're being rendered in preview mode. The problem with this approach is that modules which provide blocks that are sensitive to preview mode will either need to establish an artificial dependency on Layout Builder, or conditionally inject the service dependency into blocks based on whether Layout Builder is installed for the current site. Neither of these side effects are ideal in my opinion. This theoretical service may also need to be written with the assumption that only a single Layout Builder element is used in each response, which isn't currently a documented restriction for Layout Builder.

Another alternative is refactoring the Block API to add support for injection of preview information directly into every block plugin. I'm imagining something like PreviewAwareBlockPluginInterface::setBlockInPreview(bool $in_preview = TRUE): void. The block would then track this value internally as needed. We could even update BlockBase to enable preview tracking for all blocks if desired. The only downside to this approach is scope creep outside of Layout Builder. We can even extend this in the future if we have concepts beyond "preview" and "regular" (even though I can't imagine this at present). I would also hope that this interface could exist in Drupal core, not in the Layout Builder module, so that contrib doesn't need to depend on Layout Builder for preview detection.

I definitely think the latter of the two is the more tenable option, at least as far as backward compatibility and developer experience are concerned. It may even be more clear than using contexts. Regardless, these ideas are just from me spitballing at 10pm on a Friday, so let me know what you think.

I agree the second option would be better. We can even put it on BlockInterface without it being a BC break under the 1:1 rule because we have BlockBase

Would it be okay to expand the scope of this issue to also include the injection of preview information into layout plugins? (This is the original reason my company took interest in this issue. The context solution allowed this out of the box, but if we drop that in favor of my proposal in #40, then this will no longer be possible.)

Given my previous naming convention, we'd have PreviewAwarePluginInterface::setInPreview(bool $in_preview): void in Drupal core instead, which would then be usable by both block plugins & layout plugins.

Here's an example patch detailing what I think would be optimal as an implementation strategy. Obviously, there are a few outstanding things requiring discussion/review:

1. Where should PreviewAwarePluginInterface live in Drupal core? I've placed it in Drupal\Core\Plugin for now to split the difference between where this interface is used (i.e., block & layout plugins for now), but I could see an argument for this living in Drupal\Core\Layout also.
2. Do we need to worry about cacheable metadata? I think not, especially since preview mode for blocks shouldn't be cacheable in theory.
3. I need to write tests (obviously).

Overall, I think this approach is much simpler than using the context system. Hopefully maintainers are amenable to including injection support for layout plugins too!

Adding a functional test to cover preview-aware block & layout plugins for initial code review.

Thanks for your review! Here are the requested revisions.

Updating issue title and summary to match the new approach. Draft change record posted here.

This turned out really great, awesome work.

I think there's one thing we should add, which is an implementation of hook_preprocess_layout that sets an 'is_preview' variable that can be used in layout/block twig templates. This would allow, e.g. a layout plugin to ensure the region attributes/wrappers were present in preview, but could be stripped in actual view. Or you could conditionally add a class to a region that might trigger some Javascript that you don't want to execute on the preview (e.g. you probably don't want a carousel to be interactive in layout builder preview).

I think that should be a follow-up though, as we should focus on shipping something useful and iterate from there.

Adding issue credits.

@BhumikaVarshney, I'm unchecking the credit box for your screenshot - we don't provide credit for screenshots showing a patch applies, the automated testbot will tell us that.

I've queued a run of the patch against 10.0.x - if that passes I'll commit this tomorrow.

The failure on 10.0.x appears to be transient after a rerun.

Committed 1c4a6d6 and pushed to 10.0.x. Thanks!

Backported to 9.4.x

Published change record.

Can we close #3194767: Required contexts with default values should not fail the requirements check in the context handler now?

@clayfreeman can you create the follow-up discussed in #51 please - thanks!

Follow-up issue added here: #3273317: Add 'in_preview' variable to block and layout templates. Closed #3194767: Required contexts with default values should not fail the requirements check in the context handler as won't fix.