Problem/Motivation

Layout Builder currently allows instances of Block Plugins to be added to Sections, but does not allow creation of new content.

Today, if you wanted to add a Basic Custom Block, you would need to create it in a new tab, then place the derived Block Plugin in a Section. Once that Custom Block is placed, changes to its fields are not reflected in the parent entity's revision history or configuration. This leads to incompatibilities with modules like Content Moderation, as users expect everything they see when viewing a Draft to be tied to the latest revision.

Proposed resolution

We should improve this experience by allowing Custom Blocks to be created and placed inline. For an MVP, a new Block Plugin should be created that embeds Custom Block entity forms inside the normal configuration form. This will allow Custom Block field values to be tracked in that configuration, which will have the added benefit of Content Moderation support.

Remaining tasks

Write a patch.

User interface changes

The Layout Builder's interface will have an added "Create Content" section to creating Custom Blocks inline. The mockups already include this.

API changes

Undecided.

Data model changes

Undecided.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

samuel.mortenson created an issue. See original summary.

tim.plunkett’s picture

Issue tags: +Blocks-Layouts, +sprint

Tagging as this is currently on our shortlist.

samuel.mortenson’s picture

I'm adding an issue that blocks the storage of Custom Block fields in JSON string or array - right now we would have to use serialize/unserialize to get this working.

bkosborne’s picture

This was done for Panelizer somewhat recently right? I think Lightning drove the effort to make this work. One use case that was established was removing the annoyance of having "one-off" blocks that were created for landing pages from polluting the custom block library.

samuel.mortenson’s picture

Assigned: Unassigned » samuel.mortenson

@bkosborne It never got committed but there's an inline block patch for CTools that this work will largely be based on: #2844054: Inline Revisionable Content Blocks. I have a starter patch ready locally for Layout Builder but haven't had time to write tests - will hopefully have it up this week.

samuel.mortenson’s picture

Status: Active » Needs review
FileSize
13.69 KB

Here's a start to this functionality.

This patch:

  1. Adds a new Block Plugin that allows you to create inline Custom Blocks, which are stored as serialized objects in the plugin configuration. The form for this plugin uses the EntityFormDisplay, and does not use sub-forms, which is how Inline Entity Form accomplishes this in contrib.
    It seems to work, even for complex AJAX widgets like Entity Reference or Image.
  2. Adds a new Block Plugin Deriver for each Custom Block Type. The deriver adds the correct configuration dependencies and is otherwise pretty basic.
  3. Adds a test which validates that the EntityFormDisplay works, the rendering works, and the serialization works.

Please try the patch out and break the Block Plugin form! I'm sure there's some widget that doesn't work.

Things that need to be addressed before this goes into final review:

  1. The Custom Blocks this Block Plugin creates are essentially always new - which means that when they're rendered or edited anything that relies on an ID existing will not work. What are the ramifications of this? Is this OK?
  2. WYSIWYG looks pretty bad in the off-canvas dialog. I wouldn't consider this done until all core field widgets look presentable in Layout Builder.
  3. We should have UX review to discuss the term "inline", I'm not sure it makes sense to everyone outside of the layout space.
  4. How does file usage work with these inline blocks? Do we need to hook into that system somehow?
  5. Quick Edit only works on non-new entities, so it will not work out of the box with this. Do we want to support Quick Edit? I used to think this was fairly important but since you can't use Quick Edit at all while visiting the "Layout" tab of entities, I don't think it's worth us doing.
bkosborne’s picture

FileSize
73.1 KB

Thanks for working on this!

You weren't kidding about WYSIWYG looking bad in off-canvas:

ckeditor in the "off-canvas" dialog

bkosborne’s picture

I think the width of the off-canvas sidebar is the primary issue. I can imagine a few use cases where a custom block type would have many fields that would make the sidebar very clunky. I can think of two ways to address that:

1) Instead of loading the block form in the sidebar, load it in modal. Could get pretty crazy with the number of clicks needed to get to this form though, and it introduces another paradigm for manipulating layout builder content.

2) Have someway to temporarily expand the sidebar to better accommodate larger forms.

tim.plunkett’s picture

Can #8 be moved to a new issue? Even if it has to block this issue, it'd be better to keep this issue's scope focused.

samuel.mortenson’s picture

@bkosborne We have to load it in off-canvas - using normal dialogs conditionally for Block Plugin forms would be tough, and would break stylistic consistency. Luckily we can make the default width for off-canvas larger in Layout Builder, so your #2 suggestion should be do-able.

@tim.plunkett Yes - it will need to be. Now that 8.5.0 is out and off-canvas is stable, we can't technically change the stable theme's CSS which makes issues like this really tough. But that's not Layout Builder's problem.

bkosborne’s picture

japerry’s picture

samuel.mortenson’s picture

Assigned: samuel.mortenson » Unassigned
samuel.mortenson’s picture

mglaman’s picture

I'm going to RTBC given that the WYSIWYG was moved as a non-blocker. I was able to create a custom block content type for media embeds and add it to my node's layout overrides.

Popped up in sidebar, modal launched perfectly.

I was able to upload / select and see preview

Placed and could be viewed post-layout save.

mglaman’s picture

Status: Needs review » Reviewed & tested by the community

forgot to change status.

samuel.mortenson’s picture

Status: Reviewed & tested by the community » Needs review

Thanks @mglaman - I'm glad the patch is already working for you!

I'd like to see responses (or follow ups) to my concerns in #6 before moving this to RTBC, so I'm putting it back into review.

tim.plunkett’s picture

Assigned: Unassigned » tim.plunkett

Assigning to myself for further review.

tim.plunkett’s picture

  1. +++ b/core/modules/layout_builder/src/Plugin/Block/InlineBlockContentBlock.php
    @@ -0,0 +1,224 @@
    +    $block = $this->getEntity();
    ...
    +      '#block' => $block,
    ...
    +    $block = $block_form['#block'];
    ...
    +    $block = $block_form['#block'];
    

    Why not continue to use $this->getEntity() throughout?

  2. +++ b/core/modules/layout_builder/src/Plugin/Block/InlineBlockContentBlock.php
    @@ -0,0 +1,224 @@
    +      '#process' => [[self::class, 'processBlockForm']],
    

    s/self/static

  3. +++ b/core/modules/layout_builder/src/Plugin/Block/InlineBlockContentBlock.php
    @@ -0,0 +1,224 @@
    +    $element['revision_log']['#access'] = FALSE;
    +    $element['info']['#access'] = FALSE;
    

    This seems brittle. Future idea: ship with a form mode that excludes these two.

johnwebdev’s picture

Issue summary: View changes

How is translation intended to work on inline created blocks?

samuel.mortenson’s picture

@johndevman To translate an inline block, you would edit/create a translation of the entity with the Layout Builder field (i.e. the node), then edit the inline blocks (or place new ones, for localization). I'm not sure what the status is on Layout Builder and multilingual, but once that all works inline blocks should just work as well.

phenaproxima’s picture

To translate an inline block, you would edit/create a translation of the entity with the Layout Builder field (i.e. the node), then edit the inline blocks (or place new ones, for localization). I'm not sure what the status is on Layout Builder and multilingual, but once that all works inline blocks should just work as well.

Is there any chance that we could have a test of this? If not, can we open a follow-up to test this?

Also, regarding #19.3: That sounds like a follow-up.

phenaproxima’s picture

Only two tiny things, neither of which is a big enough deal to kick this back to Needs Work.

  1. +++ b/core/modules/layout_builder/src/Plugin/Block/InlineBlockContentBlock.php
    @@ -0,0 +1,224 @@
    +  public static function processBlockForm($element, FormStateInterface $form_state) {
    

    $element should be type hinted.

  2. +++ b/core/modules/layout_builder/src/Plugin/Block/InlineBlockContentBlock.php
    @@ -0,0 +1,224 @@
    +        $this->blockContent = BlockContent::create([
    +          'type' => $this->getDerivativeId(),
    +        ]);
    

    Why not use $this->entityTypeManager->getStorage()->create() here?

johnwebdev’s picture

It seems to be a bug when trying to change the layout of a translation. This is perhaps fine, if Layout Builder should only support symmetric translation, however you must be able to translate the inline blocks once this issue lands.
I created a new issue for this: https://www.drupal.org/project/drupal/issues/2957506

johnwebdev’s picture

#2946333: Allow synced Layout override Translations: translating labels and inline blocks solves the bug with not being able to translate. My only concern is that that patch would allow you to change the layout completely on a translated node.

Given you don't want that, the Layout field should remain untranslatable, but that means you would not be able to translate inline blocks since its stored on the layout field.
I think this is some-what related to how you work with Entity references sometimes, like taxonomies. The node and its translation should have the same references, but the references might be translated.

EclipseGc’s picture

we want to allow completely different layouts per language. that was an early goal of the architecture and we have test coverage for it.

Eclipse

johnwebdev’s picture

@EclipseGc I definitely don't argue against that, I think it's an awesome feature to be able to do so. I am simply making awareness of a common use case where you want symmetric layouts for translations and if inline blocks should be stored differently to solve that use case or not, or we go somewhere along the line of It's up to the editors to not make layout changes on the translation, if they want to be able to translate inline blocks

mtodor’s picture

I have tested this functionality and it looks really nice.

Maybe to push this issue forward I would like to give my opinion on open questions from #6:

  1. I would say, no one should base any functionality/styling/whatever on the ID (and it's also internal ID). I think we should not care about that case.
  2. "WYSIWYG looks pretty bad" -> so TRUE! UI looks ugly I have to agree and it will be even uglier when people start to use custom form widgets there. But that should be follow-up ticket and maybe also relevant for stable. We should also take in consideration that sidebar has limited width and that will be a problem too.
  3. It would be really nice to have edit form really "inline", not in the sidebar, but that would generate other problems. Like two or more open forms at the same time. I think that sidebar is providing some kind of separation between layout builder UI and block editor UI, what is ok for now. But UX review would be nice for sure.
  4. File usage. :'( - there are already few issues related to file usage and that counter is not so precise. Also, we should consider that media should be used in future and not direct files. It would be good to stay outside of that "file usage" mess, but it's not my call. :)
  5. It's better to keep Quick Edit outside of Layout Builder UI. Quick edit is available on "view" page and that's sufficient. I didn't test does it work for custom blocks, since another patch is needed for that.

There is one additional thing we should address. We are using layout builder to define the layout for custom block type. And there we can add the same custom block type with same display view mode and so on (Inception!!! O.o) -> that leads (at least on my machine) to out of memory issue.
We should somehow block circular dependencies, I guess. Or make them only one level deep. Or we can say, you can't use custom blocks for custom blocks layout. Or, any ideas?

But in general, this feature is GREAT!!!

hawkeye.twolf’s picture

Comparing this approach to "Custom Block (Non-)Reusability" (#2957425: Allow the inline creation of non-reusable Custom Blocks in the layout builder).

Pros of inline-block approach

  1. The data truly belongs to the node and cannot be (easily) misused; serializing content discourages other systems (Views, Services, etc.) from trying to use it.
  2. Content Moderation works out-of-the-box (no need for ERR in core, #2859995: Add Entity Reference Revisions to core)
  3. No need to solve the revision-bloat, node edit form timeouts, and other serious performance issues associated with a Paragraphs-style approach (see #2960887: Do not create field revisions when field data hasn't changed).

Cons of inline-block approach

  1. Entity reference dependencies can no longer be determined using ER field tables (i.e., an inline block could depend on another entity and there's no simple way to figure that out right now, for things like Deploy, "safe delete", etcetera). @EclipseGC proposes a solution for this via Dependency Calculation module.
  2. Philosophical objection to storing content as a blob (serialized PHP) instead of structured tables. I.e., we can no longer rely on the ability to write a SQL query to obtain a specific piece of data. See "pro" #1 for why this is not really an issue.
  3. Cannot translate individual inline-blocks; i.e., the translation must include the layout and all "blob" content. E.g., it's not possible to maintain a unified page layout and content placement across translations while translating actual block content from a separate interface.

Summary

We get a lot "for free" by storing this content as a blob in the layout field. Content moderation, translation, etc. are hard, tangible problems that we don't have to worry about with the inline-block approach. Working out entity dependencies (a downside to inline-blocks and a benefit to actually saving blocks to the database) is a less common requirement and one that we know we can solve.

So, in my view, the entity dependency problem should not be seen as a blocker to this approach. However, can anyone think of other potential stumbling blocks for the contrib space, i.e., modules for which the serialization of content presents a problem? Thanks!

EclipseGc’s picture

Yeah, just to clarify on "Con: #3" while there's not a separate translation interface for the inline blocks, the layout field is translate-able, so those blocks can have a translation in the sense that a customized layout for a given translation could include translated text for those blocks (or, a completely different block all together).

All in all, I agree with what 29 has to say, I just wanted to make this point of clarification for others who may read it.

Eclipse

johnwebdev’s picture

Quick Edit only works on non-new entities, so it will not work out of the box with this. Do we want to support Quick Edit? I used to think this was fairly important but since you can't use Quick Edit at all while visiting the "Layout" tab of entities, I don't think it's worth us doing.

I think this deserves to be a follow-up at the very least.

tedbow’s picture

  1. Validation for fields don't show. This is true for any blocks in layout builder. But more obvious with field validation #2968110: Layout Builder's ConfigureBlockFormBase forms do not display validation errors on submit
  2. Because inline blocks would also be available the Block Layout page it seems like it would be very confusing to the site builder in that context whether they should use inline blocks or a custom block. Hopefully this can be addressed with good help text.

    We could use the new hook hook_plugin_filter_TYPE__CONSUMER_alter to only show inline blocks in the layout builder context. This would help with the site builder confusion but Inline blocks could be super useful for placing blocks in the existing Block UI because the block placement, configuration, would not tied to a content entity.

tedbow’s picture

Just some nits as I was reviewing the patch.

tedbow’s picture

Using the new hook hook_plugin_filter_TYPE_alter to only expose Inline Blocks to the Layout Builder consumer.

The last submitted patch, 34: interdiff-33-34.patch, failed testing. View results

tedbow’s picture

Changing the category for inline blocks to "Create new block" and moving it to the top to be more similar to mockups https://projects.invisionapp.com/boards/KH3EPJMFNWR4Z

tedbow’s picture

Add expand testing to include using adding inline blocks to default layout.

@todo Add Config dependency logic to BlockContentDeriver

tedbow’s picture

  • +++ b/core/modules/layout_builder/src/Controller/ChooseBlockController.php
    @@ -67,7 +67,14 @@ public function build(SectionStorageInterface $section_storage, $delta, $region)
    -    foreach ($this->blockManager->getGroupedDefinitions($definitions) as $category => $blocks) {
    +    $grouped_definitions = $this->blockManager->getGroupedDefinitions($definitions);
    +    // Move 'Create new block' category to the top.
    +    if (isset($grouped_definitions['Create new block'])) {
    +      $new_block_definition = $grouped_definitions['Create new block'];
    +      $grouped_definitions['Create new block'];
    +      $grouped_definitions = array_merge(['Create new block' => $new_block_definition], $grouped_definitions);
    +    }
    +    foreach ($grouped_definitions as $category => $blocks) {
    

    Removing these changes to reduce the scope of this issue to just creating the new plugin type.

    Creating the UI for adding the blocks in Layout Builder can be a follow up.

    Other changes in patch will remove the ability to actually use the block plugins to avoid data storage BC before the follow up for the making the UI is done(incase there are data model changes).

    Follow up: #2968500: Change inline blocks workflow in Layout Builder to match mocks

    If you want to test adding inline blocks manually you can enable the layout_builder_test which is used in the test.

  • +++ b/core/modules/layout_builder/src/Plugin/Derivative/BlockContentDeriver.php
    @@ -0,0 +1,57 @@
    +      $this->derivatives[$id]['config_dependencies']['config'] = [
    +        'block_content.type.' . $id,
    +      ];
    

    This can be changed to use \Drupal\Core\Entity\EntityInterface::getConfigDependencyKey() and \Drupal\Core\Entity\EntityInterface::getConfigDependencyName() like \Drupal\layout_builder\Plugin\Derivative\FieldBlockDeriver::getDerivativeDefinitions() is doing.

Status: Needs review » Needs work

The last submitted patch, 38: 2948064-38.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

tedbow’s picture

Status: Needs work » Needs review
FileSize
17.29 KB
843 bytes

Bug fix for the if custom blocks not enabled.

phenaproxima’s picture

Status: Needs review » Needs work

I found only one major question -- otherwise this is simple and elegant.

  1. +++ b/core/modules/layout_builder/src/Plugin/Block/InlineBlockContentBlock.php
    @@ -0,0 +1,213 @@
    +      '#process' => [[self::class, 'processBlockForm']],
    

    Can we use static instead of self?

  2. +++ b/core/modules/layout_builder/src/Plugin/Block/InlineBlockContentBlock.php
    @@ -0,0 +1,213 @@
    +      '#description' => $this->t('Output the block in this view mode.'),
    

    Can we rephrase this? "The view mode in which to render the block" or similar?

  3. +++ b/core/modules/layout_builder/src/Plugin/Block/InlineBlockContentBlock.php
    @@ -0,0 +1,213 @@
    +      '#access' => (count($options) > 1),
    

    Nit: Superfluous parentheses.

  4. +++ b/core/modules/layout_builder/src/Plugin/Block/InlineBlockContentBlock.php
    @@ -0,0 +1,213 @@
    +    $form_display = EntityFormDisplay::collectRenderDisplay($block, 'edit');
    +    $form_display->buildForm($block, $element, $form_state);
    

    Nit: These calls can be one line: EntityFormDisplay::collectRenderDisplay($block, 'edit')->buildForm($block, $element, $form_state)

  5. +++ b/core/modules/layout_builder/src/Plugin/Block/InlineBlockContentBlock.php
    @@ -0,0 +1,213 @@
    +    $block_form = NestedArray::getValue($form, $form_state->getTemporaryValue('block_form_parents'));
    

    Should we remove the temporary value once we have the block form element?

  6. +++ b/core/modules/layout_builder/src/Plugin/Block/InlineBlockContentBlock.php
    @@ -0,0 +1,213 @@
    +    $block = $block_form['#block'];
    

    Let's add a little sanity check to be sure that $block_form['#block'] is actually an instance of BlockContentInterface, and set an error (or something) if it's not. Just for sanity's sake.

  7. +++ b/core/modules/layout_builder/src/Plugin/Block/InlineBlockContentBlock.php
    @@ -0,0 +1,213 @@
    +    $complete_form_state = ($form_state instanceof SubformStateInterface) ? $form_state->getCompleteFormState() : $form_state;
    

    Nit: The parentheses are superfluous.

  8. +++ b/core/modules/layout_builder/src/Plugin/Block/InlineBlockContentBlock.php
    @@ -0,0 +1,213 @@
    +    $block->setInfo($this->configuration['label']);
    

    Where was $this->configuration['label'] previously set?

  9. +++ b/core/modules/layout_builder/src/Plugin/Block/InlineBlockContentBlock.php
    @@ -0,0 +1,213 @@
    +  protected function blockAccess(AccountInterface $account) {
    +    if ($this->getEntity()) {
    +      return $this->getEntity()->access('view', $account, TRUE);
    +    }
    +    return AccessResult::forbidden();
    +  }
    

    $this->getEntity() will never return an empty/null result, so access will never be forbidden. It seems to me like we should probably determine access depending on whether the entity isNew() and the current user has permission to create blocks of that type.

  10. +++ b/core/modules/layout_builder/src/Plugin/Block/InlineBlockContentBlock.php
    @@ -0,0 +1,213 @@
    +   * Loads the block content entity of the block.
    

    We should say "...or creates if it it does not exist."

tedbow’s picture

Assigned: tim.plunkett » Unassigned
Status: Needs work » Needs review
FileSize
4.78 KB
17.88 KB

@phenaproxima thanks for the review!

1. fixed
2. fixed, used your wording
3. fixed
4. fixed
5. Not sure how we do this? Set key to NULL? Maybe there is not a way.
6. fixed
7. fixed
8. \Drupal\Core\Block\BlockBase::submitConfigurationForm
$this->configuration['label'] = $form_state->getValue('label');
9. Based on my understanding... Since ::getEntity() is protected method on this class I added $create_new<code> parameter. In <code>::blockAccess() $this->configuration['serialized_block'] *should* always be set because we checking ". For viewing the block it doesn't make sense to see the user can create new blocks of this type.
10. See 9. changed to "Loads or creates the block content entity of the block."

tedbow’s picture

This patch does a few things

  1. Add a 2nd node in InlineBlockContentBlockTestto confirm it still has default layout serialized block when 1st node changes.
  2. Override ::buildConfigurationForm() to take away the default title when adding a new block. The block type label doesn't make sense for the block title shown on entity view, i.e. "Basic Block" and the label will not be used anywhere else such as admin listing.
  3. Simplify ::getDerivativeDefinitions()
tedbow’s picture

The $create_new logic I added in #42 didn't actually work because it only worked if \Drupal\layout_builder\Plugin\Block\InlineBlockContentBlock::getEntity() had not been called before with $create_new *not* set.

So I added an $isNew property to the class which is set in the constructor based on whether is empty $this->configuration['serialized_block'].

johnwebdev’s picture

+++ b/core/modules/layout_builder/src/Plugin/Block/InlineBlockContentBlock.php
@@ -0,0 +1,241 @@
+  protected function getEntity() {

Can this method be public? It could be useful to be able to change block being rendered using SectionComponentBuildRenderArrayEvent etc.

So instead of doing something like:

    if (!empty($block->getConfiguration()['serialized_block'])) {
      /** @var BlockContentInterface $blockContent */
      $blockContent = unserialize($block->getConfiguration()['serialized_block']);

I could just do:

$blockContent = $block->getEntity();
tedbow’s picture

After talking with @xjm, @tim.plunkett, @eclipseGc and @japerry
Problems not addressed

  1. Field UI prevents certain changes to fields like reducing cardinality on field that already has values. This will not work for our serialized block entity fields values.
  2. Certain field like the Options field may use EntityQuery in hook_field_storage_config_update_forbidto determine if field config changes should be allow. If any option field value was in serialized block this would not show up. We could fix cores instances but contrib has been able to rely on querying available entities to determine field values.
  3. Update hooks for schema in custom block types won't work for serialized field values.
  4. If a serialized block is used in default it would exported in to config. If the block has a entity reference value then that entity would have to available when the block was reimported on different environment. Apparently this is problem that Views also has with embedded entities in headers
  5. File usage is currently not tracked for serialized file field values.

It seems like 5) is the only 1 we could really address with the serialized blocks. 1-3 look to like there is really no good way to solve.

So for now am going to look at #2957425: Allow the inline creation of non-reusable Custom Blocks in the layout builder as a possible solution.

johnwebdev’s picture

#46

3. What if we only store the actual values instead of the serialised object?
4. Do we really need to support Inline content blocks in defaults?

tedbow’s picture

#47
3. This actually won't solve the problem. However we store it we have to turn the values/object back into entity so it can be rendered. If hook schema don't run those values may not be valid for the fields/entity.
4. I don't think we need to use inline content blocks in defaults and #2957425: Allow the inline creation of non-reusable Custom Blocks in the layout builder may have the same problem.

tedbow’s picture

Title: Layout Builder should support inline creation of Custom Blocks » Layout Builder should support inline creation of Custom Blocks using entity serialization

updating to distinguish it from sub task needed for #2957425: Allow the inline creation of non-reusable Custom Blocks in the layout builder which actually seems more promising than this issue.

tedbow’s picture

Status: Needs review » Closed (won't fix)

I am closing this as "won't fix" because I don't see a way to clean solve the problems I mentioned in #47