Inline Revisionable Content Blocks

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

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

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

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

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.

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?

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.

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.

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

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.

This is blocked by #2854542: PanelsIPEBlockPluginForm should use a real submit function.

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.

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.

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.

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

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.

I moved the new service into its own issue: #2858103: Add serializable tempstore service (extracted from #2844054)

This patch contains everything else. All changes by EclipseGc and phenaproxima.

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

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

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...

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.

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".

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.

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

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?

@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. :-)

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 ^_^

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.

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.

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.

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

This blocks #2883154: Support inline blocks in Panels display variants.

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?

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 :-)

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.

Related issue shows limit of string translation, so translations revisions will abuse string translations that used to translate config

#46 is sadly not compatible with newer D8 versions or D9. When forcing the install of ctools_inline_block and ctools_entity_mask via Backward Compatibility it WSODs with the following error messages:

Warning: Declaration of Drupal\ctools_inline_block\Entity\InlineBlockStorage::cleanIds(array $ids) should be compatible with Drupal\Core\Entity\ContentEntityStorageBase::cleanIds(array $ids, $entity_key = 'id') in include() (line 15 of /modules/contrib/ctools/modules/ctools_inline_block/src/Entity/InlineBlockStorage.php)

Drupal\Core\Entity\Exception\UnsupportedEntityTypeDefinitionException: The entity type inline_block_content does not have an "revision_created" entity revision metadata key. in Drupal\Core\Entity\EditorialContentEntityBase::revisionLogBaseFieldDefinitions() (line 35 of /core/lib/Drupal/Core/Entity/RevisionLogEntityTrait.php).

Even though I know that Layout Builder has more or less replaced Panels/Panelizer/FPP, etc, it is still incredibly useful when using Panels Everywhere since Layout Builder Everywhere still has quite a few bugs.

Edit: It looks like Page Manager now has a Layout Builder variant type. Adding inline-blocks should now be possible without problems. However there is still an issue with Panels Everywhere (for those overriding the entire page) here: #3143487: Not compatible with "Layout Builder" variant as site template (Workaround found)