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.
Comment | File | Size | Author |
---|---|---|---|
#44 | interdiff-43-44.txt | 3.95 KB | tedbow |
#44 | 2948064-44.patch | 20.21 KB | tedbow |
#43 | 2948064-43.patch | 20.02 KB | tedbow |
#43 | interdiff-42-43.txt | 7.01 KB | tedbow |
#42 | 2948064-42.patch | 17.88 KB | tedbow |
Comments
Comment #2
tim.plunkettTagging as this is currently on our shortlist.
Comment #3
samuel.mortensonI'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.
Comment #4
bkosborneThis 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.
Comment #5
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.
Comment #6
samuel.mortensonHere's a start to this functionality.
This patch:
It seems to work, even for complex AJAX widgets like Entity Reference or Image.
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:
Comment #7
bkosborneThanks for working on this!
You weren't kidding about WYSIWYG looking bad in off-canvas:
Comment #8
bkosborneI 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.
Comment #9
tim.plunkettCan #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.
Comment #10
samuel.mortenson@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.
Comment #11
bkosborneI searched for existing off-canvas issues and this one looks interesting: #2916781: Allow off-canvas dialog to be rendered at the top of the page.
Follow-up created here: #2951547: Modify Layout Builder's use of the off-canvas tray to improve UX of larger block forms
Comment #12
japerryComment #13
samuel.mortensonComment #14
samuel.mortensonCreated #2952390: Off-canvas styles override CKEditor's reset and theme for the CKEditor styling issue.
Comment #15
mglamanI'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.
Comment #16
mglamanforgot to change status.
Comment #17
samuel.mortensonThanks @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.
Comment #18
tim.plunkettAssigning to myself for further review.
Comment #19
tim.plunkettWhy not continue to use
$this->getEntity()
throughout?s/self/static
This seems brittle. Future idea: ship with a form mode that excludes these two.
Comment #20
johnwebdev CreditAttribution: johnwebdev commentedHow is translation intended to work on inline created blocks?
Comment #21
samuel.mortenson@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.
Comment #22
phenaproximaIs 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.
Comment #23
phenaproximaOnly two tiny things, neither of which is a big enough deal to kick this back to Needs Work.
$element should be type hinted.
Why not use $this->entityTypeManager->getStorage()->create() here?
Comment #24
johnwebdev CreditAttribution: johnwebdev commentedIt 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
Comment #25
johnwebdev CreditAttribution: johnwebdev commented#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.
Comment #26
EclipseGc CreditAttribution: EclipseGc at Acquia commentedwe want to allow completely different layouts per language. that was an early goal of the architecture and we have test coverage for it.
Eclipse
Comment #27
johnwebdev CreditAttribution: johnwebdev commented@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
Comment #28
mtodor CreditAttribution: mtodor at Thunder commentedI 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:
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!!!
Comment #29
hawkeye.twolfComparing 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
Cons of inline-block approach
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.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!
Comment #30
EclipseGc CreditAttribution: EclipseGc at Acquia commentedYeah, 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
Comment #31
johnwebdev CreditAttribution: johnwebdev commentedI think this deserves to be a follow-up at the very least.
Comment #32
tedbowWe 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.Comment #33
tedbowJust some nits as I was reviewing the patch.
Comment #34
tedbowUsing the new hook
hook_plugin_filter_TYPE_alter
to only expose Inline Blocks to the Layout Builder consumer.Comment #36
tedbowChanging 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
Comment #37
tedbowAdd expand testing to include using adding inline blocks to default layout.
@todo Add Config dependency logic to BlockContentDeriver
Comment #38
tedbowRemoving 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.
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.Comment #40
tedbowBug fix for the if custom blocks not enabled.
Comment #41
phenaproximaI found only one major question -- otherwise this is simple and elegant.
Can we use static instead of self?
Can we rephrase this? "The view mode in which to render the block" or similar?
Nit: Superfluous parentheses.
Nit: These calls can be one line:
EntityFormDisplay::collectRenderDisplay($block, 'edit')->buildForm($block, $element, $form_state)
Should we remove the temporary value once we have the block form element?
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.
Nit: The parentheses are superfluous.
Where was $this->configuration['label'] previously set?
$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.
We should say "...or creates if it it does not exist."
Comment #42
tedbow@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."
Comment #43
tedbowThis patch does a few things
InlineBlockContentBlockTest
to confirm it still has default layout serialized block when 1st node changes.::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.::getDerivativeDefinitions()
Comment #44
tedbowThe
$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']
.Comment #45
johnwebdev CreditAttribution: johnwebdev commentedCan 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:
I could just do:
Comment #46
tedbowAfter talking with @xjm, @tim.plunkett, @eclipseGc and @japerry
Problems not addressed
hook_field_storage_config_update_forbid
to 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.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.
Comment #47
johnwebdev CreditAttribution: johnwebdev commented#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?
Comment #48
tedbow#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.
Comment #49
tedbowupdating 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.
Comment #50
tedbowI am closing this as "won't fix" because I don't see a way to clean solve the problems I mentioned in #47