Problem/Motivation

It can be quite difficult to get Content Blocks revisioning with the entity to which they are attached. This is complicated by the fact that scenarios in which content blocks are heavily used can clutter the block placement UI with an enormous number of blocks that were likely never intended for reuse.

Proposed resolution

We need a new way of interacting with Content Blocks that does not save them in a traditional entity approach. A null save that allows us to get the result of this save and serialize it into the block configuration. Serialized content blocks would be treated as a revisionable string in panelizer's normal operation which would mean that content blocks used this way would automatically revision with the rest of the "config" blocks because the storage would all be on the panelizer field.

Remaining tasks

We need to really clean up the following patch, update schema, make certain our UI stuff all makes sense and test out weird stuff like dates and file/image fields. Also, it's best if this field hides the configuration block's label form elements and just overrides the normal label with the title of the content block entity. We do that already for panelizer's IPE integration but need to figure out how to do it more generically.

User interface changes

A new section under IPE's block placement will be "custom" (I'd like to probably change this to inline). Within that section you'll find "Inline ..." blocks. These are what you want to test with. New ones should appear there automatically when you create new content block types.

API changes

This code depends on Inline Entity Form module. Be certain to download it if you want to play with this.

Data model changes

We introduce a new block type (inline block). So the existing data model's not changed, we're just abusing it a bit. :-D

Initial credit

I didn't do this first pass at a patch on my own, phenaproxima was instrumental in helping to make this happen as was tedbow.

CommentFileSizeAuthor
#46 2844054-46.patch38.18 KBphenaproxima
#44 2844054-44.patch43.69 KBphenaproxima
#39 2844054-39.patch51.47 KBphenaproxima
#38 2844054-38.patch54.51 KBEclipseGc
#36 2844054.interdiff.txt11.56 KBEclipseGc
#36 2844054-36.patch50.33 KBEclipseGc
#35 interdiff-2844054-33-35.txt8.99 KBphenaproxima
#35 2844054-35.patch40.91 KBphenaproxima
#33 2844054-33.patch40.24 KBphenaproxima
#26 ctools-n2844054-26.patch39.43 KBDamienMcKenna
#21 interdiff-2844054-19-21.txt6.46 KBphenaproxima
#21 2844054-21.patch41.22 KBphenaproxima
#19 2844054-19.patch36.73 KBphenaproxima
#18 2844054-18.patch31.52 KBphenaproxima
#17 2844054-17.patch27.83 KBEclipseGc
#14 2844054-14.patch24.67 KBEclipseGc
#12 interdiff-2844054-10-12.txt6.39 KBphenaproxima
#12 2844054-12.patch24.54 KBphenaproxima
#10 interdiff-2844054-6-10.txt7.61 KBphenaproxima
#10 2844054-10.patch25.45 KBphenaproxima
#6 2844054-6.patch18.5 KBphenaproxima
#4 interdiff-2844054-0-4.txt5.11 KBphenaproxima
#4 2844054-4.patch14.93 KBphenaproxima
inline-blocks.patch12.82 KBEclipseGc
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

EclipseGc created an issue. See original summary.

DamienMcKenna’s picture

Oh, I like the idea! This is very similar to what Paragraph Panes does.

DamienMcKenna’s picture

It might help to add a test method that includes a file field.

phenaproxima’s picture

Added a little more rigorous testing and fixed bugs discovered by it. This is going to need really thorough test coverage.

DamienMcKenna’s picture

Status: Needs work » Needs review

Would it be ok to finish this after the 3.0 release?

phenaproxima’s picture

Here's a new version of the patch that fixes some stuff. I have added thorough kernel-level test coverage of the panelizer_block_content shadow entity type, proving that it behaves identically to a block_content entity as far as Drupal is concerned, and transparently so.

Status: Needs review » Needs work

The last submitted patch, 6: 2844054-6.patch, failed testing.

samuel.mortenson’s picture

I noticed that the current test coverage doesn't assert that rendering blocks will result in the same HTML - is that still going to be covered?

phenaproxima’s picture

Yes. Absolutely. And I want to test Quick Edit compatibility too. But all of that, in my mind, is contingent upon the shadow blocks being transparently identical to normal ones. So the storage stuff is the foundation.

phenaproxima’s picture

Another iteration. This patch:

  • Plugs a bunch of holes in the null storage class that were making Panelizer uninstallable. As in, not possible to install. Which is why #6 failed.
  • Added a test assertion proving that one can transparently load a block_content as a panelizer_block_content using a UUID. (This is the mechanism that the block_content plugin uses to load blocks for display.)
  • Added a functional test proving that displaying a panelizer_block_content using the block_content plugin produces identical markup as displaying a normal block_content.
DamienMcKenna’s picture

Thinking through it, this might be better in Panels so that it could be used by *all* Panels-based display systems.

phenaproxima’s picture

Status: Needs work » Needs review
Issue tags: +Needs tests, +quickedit
FileSize
24.54 KB
6.39 KB

IT WORKS!

This has no tests yet, and there are known bugs (particularly a bizarre double-save that might be IPE's fault, but I don't know yet) but it proves the concept. Behold, the prototype of inline revisionable content blocks!

Props to @EclipseGc for his invaluable help in making this happen, and for writing the original versions of the InlineBlock plugin and its deriver.

Still need Quick Edit integration and tests.

Status: Needs review » Needs work

The last submitted patch, 12: 2844054-12.patch, failed testing.

EclipseGc’s picture

FileSize
24.67 KB

Ok, fixed an issue we had with custom blocks actually getting created in the previous patch.

Eclipse

PS If you want to try this against imagefield blocks, you need to hack the shared tempstore class and use the dependency serialization trait. Still trying to figure out how to move something that will suffice for that shortcoming into the patch.

phenaproxima’s picture

DamienMcKenna’s picture

Project: Panelizer » Chaos tool suite (ctools)

Per today's scotch meeting, I'm moving this over to CTools so it can be a more foundational system that all Panels-ish modules can use.

EclipseGc’s picture

Ok, so I moved this all over to ctools and added some small checks to make it work inside of regular block layout as well. Tested with imagefield and it worked (which seems to be the high bar here). I moved the tests over but didn't get them working yet. Removing quickedit tag for the moment. We'll do that in a follow up.

Eclipse

phenaproxima’s picture

Won't pass the tests yet, but this formalizes the ctools_inline_block module and introduces a mechanism for loading inline blocks like normal entities.

phenaproxima’s picture

Status: Needs work » Needs review
FileSize
36.73 KB

OK. I've updated all the tests, so this should have parity (and then some) with the original patch for Panelizer. I'm leaving the "needs tests" tag applied because we should probably have coverage of how inline blocks interact with image fields -- that was a point of possible pain, so let's head off those problems at the pass.

Status: Needs review » Needs work

The last submitted patch, 19: 2844054-19.patch, failed testing.

phenaproxima’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
41.22 KB
6.46 KB

Added a functional JS test of using image fields with inline blocks! And it passes. On my localhost, anyway...

Status: Needs review » Needs work

The last submitted patch, 21: 2844054-21.patch, failed testing.

phenaproxima’s picture

Status: Needs work » Needs review
Issue tags: +Needs manual testing

The failures are not due to anything wrong with the patch; I suspect wonkiness in Drupal CI, and the fact that it cannot load in dependencies. Therefore, I'm marking this as needing manual testing because you need to run the test suite on a local machine.

The last submitted patch, 17: 2844054-17.patch, failed testing.

The last submitted patch, 18: 2844054-18.patch, failed testing.

DamienMcKenna’s picture

Note: the tests will (most likely) fail because it needs #2858103.

Status: Needs review » Needs work

The last submitted patch, 26: ctools-n2844054-26.patch, failed testing.

DamienMcKenna’s picture

I'm thinking of listing this as a blocker for Panelizer 3.0/4.0 final.

  • EclipseGc committed 8204ef7 on 8.x-3.x
    Issue #2858103 by phenaproxima, DamienMcKenna, EclipseGc: Add...
phenaproxima’s picture

Status: Needs work » Needs review

Queued #26 for re-testing now that #2858103: Add serializable tempstore service (extracted from #2844054) is in. I don't think it'll pass the tests unless Drupal CI knows how to download dependencies (seeing as how this depends on Inline Entity Form). But, let us wait and see...

Status: Needs review » Needs work

The last submitted patch, 26: ctools-n2844054-26.patch, failed testing.

phenaproxima’s picture

Status: Needs work » Needs review
FileSize
40.24 KB

Probably won't pass the tests. Don't care, because this really fixes problems with the storage layer.

As of this patch:

- Inline blocks will report their entity type ID as 'inline_block_content'. Themes can figure out on their own how to handle any CSS classes generated from that.
- Thanks to some good advice from @Berdir, this uses a few hooks to link block_content bundles' fields to inline blocks, without having to actually duplicate the configuration in the database.
- It works with entity reference fields of all kinds. I tested briefly with a user reference and an image field.
- The most arcane parts of the previous approach are gone. Everything just kinda...makes more sense. It's really nice.

Status: Needs review » Needs work

The last submitted patch, 33: 2844054-33.patch, failed testing.

phenaproxima’s picture

YAY! This passes all tests locally. Drupal CI will fail it because it can't load up Inline Entity Form, but consider this as "needs review".

EclipseGc’s picture

FileSize
50.33 KB
11.56 KB

Added dispatching for block add/update/delete events on the BlockVariantTrait and panelizer support during those events.

Eclipse

DamienMcKenna’s picture

Per discussion in Slack, this needs to accommodate for displays which are exported and then reimported via CI, so that the appropriate {inline_block} records are created.

EclipseGc’s picture

FileSize
54.51 KB

Current patch. Sorry for the lack of interdiff, kind of drive-by-patching today since #drupalcon.

I'll try to get an interdiff later.

Eclipse

phenaproxima’s picture

Status: Needs work » Postponed
FileSize
51.47 KB

Re-rolled on top of #2876732: Implement an API to "mask" entity types. This issue now blocked on that one.

sylus’s picture

I took this patch for a spin against latest dev on lightning + landing_page and works really great :)

I did notice that when I saved the block once I wasn't able to subsequently change the title unless I changed the description (info property).

I'd be happy to do any further testing :)

Out of curiosity and sorry for the naivety but down the line could this potentially work with translations?

samuel.mortenson’s picture

@sylus I believe that, with the way the patch is currently written, we assume that the storage for the Panels Display (i.e. the "panelizer" field) should handle translation, which means that inline blocks get translated the same way everything else (i.e. block titles) in the Panels Display does.

So hypothetically, if translation was enabled for the panelizer field for instance, you would just visit the translated version of a Panelized node and edit the inline block, and everything should just work. We should test this. :-)

sylus’s picture

Ah that is great news! I was only asking because that is what I tried and wasn't able to translate. That being said it might be because as mentioned earlier I am unable to save a different title for an already saved inline block item. This might be why when on my french panelized page it didn't work either.

I really appreciate you taking the time to respond and great news that in theory this functionality should work ^_^

EclipseGc’s picture

Status: Postponed » Needs work

Ok, this shouldn't be postponed because we committed the patch it depended on.

Eclipse

phenaproxima’s picture

Status: Needs work » Needs review
Issue tags: -Needs manual testing
FileSize
43.69 KB

Okay, here we go. A re-roll on top of the committed Entity Mask submodule.

I pulled out all the stuff relating to Panelizer and Panels IPE -- those integrations belong in those modules. I'll file patches against those modules which integrate with this one. Plus dedicated tests.

Status: Needs review » Needs work

The last submitted patch, 44: 2844054-44.patch, failed testing.

phenaproxima’s picture

Status: Needs work » Needs review
Issue tags: +Needs manual testing
FileSize
38.18 KB

Here's a version without the block variant stuff. To keep things manageable, I'll re-file that in a separate patch.

Note that this will not pass Drupal CI because it depends on Inline Entity Form. So I'm restoring the "Needs manual testing" tag.

Status: Needs review » Needs work

The last submitted patch, 46: 2844054-46.patch, failed testing.

phenaproxima’s picture

Adding #2883103: BlockVariantTrait should fire events as a related issue, since it will be needed to integrate inline blocks with Page Manager and the Panels ecosystem.

phenaproxima’s picture

Filed an initial patch against Panels to load inline blocks out of tempstore: #2883154: Support inline blocks in Panels display variants

phenaproxima’s picture

dsnopek’s picture

So, I ended up here because I was trying to figure out how to implement #2886202-2: Make the Custom Block creation and placement process into a single form (see my thoughts there) and came to more-or-less the same conclusion as the issue summary here

However, I skimmed the patch, and I have question: Why does this need a new content entity type? Why not just a new block plugin that stores \Drupal\entity_mask_test\Entity\BlockContent entities from core in the block plugin's configuration?

The extra content entity type is present all the way back to the earliest patch, and I didn't see an explanation in the comments here.

But I haven't dug too deep into this yet, so maybe I'll figure it out on my own when I do, but just thinking it through in my head it seems like that should work and be simpler... I think the problem really is \Drupal\block_content\Plugin\Block\BlockContentBlock and that its using derivatives (rather than configuration) - not the BlockContent entity type. What am I missing?

dsnopek’s picture

I've been reading this patch, trying to understand it. It still seems like the architecture is more complicated than necessary, and could be done simpler, but I'm assuming the extra complexity must be there to support something or workaround some issues, but it's not clear from the code. So, I'm going to review the patch and treat the parts I don't understand as documentation issues to make those things explicit. :-)

  1. +++ b/modules/ctools_inline_block/src/Entity/BlockContent.php
    @@ -0,0 +1,45 @@
    +class BlockContent extends CoreBlockContent {
    

    Could this class be renamed InlineBlockContent? It makes the other code really confusing to read because everytime I see BlockContent I have to keep doing Ctrl-Q in PHPStorm to see if it's Drupal\block_content\Entity\BlockContent or \Drupal\ctools_inline_block\Entity\BlockContent. This was part of my quest to understand why this entity type is even necessary :-)

    Speaking of which, can the docblock for this class give the purpose of this entity type? My best guess so far is that it's to allow using inline_entity_form without saving a real content block to storage? Anyway, please document the why we need this entity type and what it's used for.

  2. +++ b/modules/ctools_inline_block/ctools_inline_block.module
    @@ -0,0 +1,49 @@
    +function ctools_inline_block_block_insert(BlockInterface $block) {
    +  $plugin = $block->getPlugin();
    +
    +  if ($plugin instanceof InlineBlock) {
    +    \Drupal::database()
    +      ->insert('inline_block')
    

    Since (I think) this is storing data for the InlineBlockStorage, maybe this code could be moved there and the hook implementation could just load the entity type storage handler and call some method? That way the implementation of that service isn't spread between the class and these hooks...

  3. +++ b/modules/ctools_inline_block/ctools_inline_block.module
    @@ -0,0 +1,49 @@
    +    $plugin->getEntity()->delete();
    

    What? Ok, so is the entity is going to be core BlockContent or the ctools_inlink_block BlockContent entity? (Another reason to rename that class ;-)) Is this again just bookkeeping for the InlineBlockStorage? This deserves a comment explaining or at least making the code explicitly call into the storage class so it's clear what's going on, and why.

  4. +++ b/modules/ctools_inline_block/src/BlockLoaderInterface.php
    @@ -0,0 +1,23 @@
    +/**
    + * Defines an interface for loading inline blocks in a standard way.
    + */
    +interface BlockLoaderInterface {
    

    Can you please document what this is for? Why do we need it?

    Since this patch allows the actual value of the content block to be stored in the block plugin configuration, why do we need classes to load blocks? It doesn't appear to be used in ctools_inline_block so it must be an external API - can some information be given here for why another module would want to use this?

  5. +++ b/modules/ctools_inline_block/src/Entity/InlineBlockStorage.php
    @@ -0,0 +1,129 @@
    +/**
    + * A content entity storage handler that does not save to the database.
    + */
    +class InlineBlockStorage extends MaskContentEntityStorage {
    

    Except that it does save to the database! It doesn't store data on normal entity and field tables, true, but it does store stuff in the database. Can something be added to the docblock about why it's storing what it's storing to the database?

    Also, this is "container aware", and while it's not necessary to implement the ContainerAwareInterface to work, I think it'd be nice so it's declaring to the outside world (or anyone reading this code) that you need to call ->setContainer($container) on it for the class to function correctly. Sort of "self documenting" code.

  6. +++ b/modules/ctools_inline_block/src/Plugin/Block/InlineBlock.php
    @@ -0,0 +1,78 @@
    +/**
    + * Defines a generic custom block type.
    + *
    

    So, this isn't something I'm confused about, but this could be more descriptive. Maybe "Block plugin that stores block content entities in configuration rather than referencing them"? That's kinda technical, but speaks more to the purpose of the class

  7. +++ b/modules/ctools_inline_block/src/Plugin/Derivative/InlineBlock.php
    @@ -0,0 +1,68 @@
    +/**
    + * Retrieves bundle types for inline blocks.
    + */
    

    Could be clearer. Maybe something like "Derives inline block plugins for every bundle on the content block entity type" ?

If I'm getting some this wrong (which, actually, I'm sure I am), I'd love an explanation! If that can work it's way into the patch itself, even better :-)

dsnopek’s picture

I did some manual testing with this patch. Placing a block via core (ie. in Structure -> Block layout) worked great! I placed a custom block which had an image field on it, and the creation form worked perfect, and the block rendered fine on display too.

However, I tried to place a block with the Panels IPE with Panelizer and had problems. I used the patches at #2883154: Support inline blocks in Panels display variants and #2883394: Allow Panelizer to load inline blocks as well. I'm not sure if this is a problem with the patch here or one of those. But placing a custom block which has an image field in it fails on the "Upload" button with an AJAX error in the Javascript console - submitting the form without pressing the "Upload" button will complete, but the preview of the block isn't shown in the IPE or after saving when the Panels display is rendered. I tried with a custom block which just had a text field (ie. no image field) and the results were the same.