Hidden dependency on block_content in layout_builder

This is blocking native attributes adoption, so major

#5 looks better option, not clear what's left to rtbc. Probably some test to catch similar situation in contrib/custom code, otoh maybe change record is enough

Triggering for 10.1.x tests.

@andypost should this ticket have a change record you think?

And could the test be as simple as

Steps to reproduce
Install minimal
Enable layout_builder
Use drush php to run $rc = new \ReflectionClass(\Drupal\layout_builder\Plugin\Block\InlineBlock::class);

as block plugin now abstract and new class introduced, it needs CR and update_hook for existing sites

+++ b/core/modules/layout_builder/src/Plugin/Block/InlineBlock.php
@@ -2,22 +2,15 @@
- * Defines an inline block plugin type.
+ * Defines an inline block plugin placeholder.

@@ -28,270 +21,9 @@
-class InlineBlock extends BlockBase implements ContainerFactoryPluginInterface, RefinableDependentAccessInterface {
...
+abstract class InlineBlock extends BlockBase {

+++ b/core/modules/layout_builder/src/Plugin/Derivative/InlineBlockDeriver.php
@@ -51,6 +51,7 @@ public function getDerivativeDefinitions($base_plugin_definition) {
+        $this->derivatives[$id]['class'] = 'Drupal\layout_builder\BlockContent\InlineBlock';

last line could use InlineBlock::class

Any reason why we don't just make LB depend on block content module?
It's pretty useless without it

We did some rediscovery of the options discussed here in the PHP attribute issue.

One more option that came up is whether it's actually worth to maintain layout_builders independence of block_content or if we should just not bother, add a dependency and move on. Not sure, I guess there are some arguments to be made that some use cases of layout builder like using it for overriding default view displays or page manager or something can function quite well without it. But it's also not a big hassle to have block_content enabled, although some sites might use alternative options to provide content blocks. Or we introduce a glue module that depends on layout_builder and block_content. Not sure what implications that would have on existing content and dependencies and such.

It's more of a BC hassle, but I think extracting that access concept out of block_content could actually be useful for others as well, like paragraphs? we struggle with similar issues there when access depends on specific parent revisions.

Another idea I brought up was providing explicit support for such cases with provider-as-subfolder feature in discovery, in which we traverse if the given provider exists.

If we eventually fix #2940755: block_content block derivatives do not scale to thousands of block_content entities with the single block plugin + configuration and remove the deriver (which is about 2-3 issues down from that issue), that might work in such a way that it doesn't fatal, and then couldn't we remove the dependency on block_content again then? If so, then I think we should add the dependency now, and remove it again when we've killed off the deriver.

I'm not sure.

This isn't a problem caused by the deriver. Any attribute-based plugin that uses an unknown trait (not catchable) or unknown base class (catchable but we don't do that yet) will trigger that. Quite the opposite in fact, the deriver allows for that workaround, because the deriver can do that class trickery. Without it, that would need to be done in an alter hook, exposing a BC issue if another module's alter hook comes first.

And this deriver is per block content *type*, so doesn't have the scalability issue, but actually exposes a useful thing for users (you don't need to add a Block, you add a "Hero", or a "Media gallery" or whatever you name your block content types).

That said, somehow combining the InlineBlock experience and the regular Block experience seems like it *could* be feasible from a technical standpoint, we could even keep the block type deriver, combined with a re-use existing or create new UI similar to media library. Would be a modal-in-a-model UI though in the current case? There is also that issue about allowing to create reusable blocks from within the InlineBlock UI, that's already one step in that direction.

To summary, yeah maybe, but that sounds like it's many more steps off than dropping the block_content block deriver.

And this deriver is per block content *type*, so doesn't have the scalability issue, but actually exposes a useful thing for users (you don't need to add a Block, you add a "Hero", or a "Media gallery" or whatever you name your block content types).

Doh I missed that. Even so, I think we should add the dependency, and open a follow-up to remove the dependency, because it's exchanging a fatal error for a minor annoyance (at worst).

So I realized that the dependency approach is going to cause some update problems and had another idea that is not very nice but quite localized to this single file, we just define that trait and the interface on the fly if we need it. Looks a bit ugly, but at least one test that I
tried worked. Lets see if some of our weirder test coverage doesn't like this.

There is also a slight variation of this, where we just do:

if (!trait_exists(RefinableDependentAccessTrait::class)) {
  return;
}

That kind of works, but the attribute discovery throws an exception then:

ReflectionException: Class "Drupal\layout_builder\Plugin\Block\InlineBlock" does not exist in ReflectionClass->__construct() (line 125 of core/lib/Drupal/Component/Plugin/Discovery/AttributeClassDiscovery.php).

We *could* handle that or check first with a class exists, which I think makes sense, because annotations didn't cause such a problem, and I expect there will be cases out there with with stuff in plugin folders. But I'm not sure if we really want to do it silently or log something.

Change makes sense. But could we add to the comments link to the CR or this issue and maybe mention this isn't encouraged. So others don't see it and decide they can implement that too.

Didn't break anything which could be good sign.

> Change makes sense. But could we add to the comments link to the CR or this issue and maybe mention this isn't encouraged. So others don't see it and decide they can implement that too.

Oh, I plan to do the opposite of that. Once approved/commited, I want to add this on the CR record for attribute discovery. This isn't something you do for fun. This is a last resort thing when you have no other good options, and just like layout_builder module, when you run into this during the conversion, you need to do _something_, at least as an intermediate step.

Possible alternative approach in MR!5276 moving the plugin to block_content, not sure this is any better than @Berdir's approach though.

If we move it to block_content we'd need to ensure block_content is enabled on any site using that plugin. I guess we could do something like:
- post update to enable block_content if layout_builder is enabled, this could be in layout_builder itself
- don't make block_content an actual dependency of layout builder.

There's a potential UX issue when people then install layout_builder and not block_content and can't do what they expect, but could add help text for that.

Only question is would we run into issues before the post update runs with other post updates hook_update_N() if the plugin provider is 'missing', that seems fairly likely tbh. We might want to quickly get that update into 10.2.x without moving the plugin, then move the plugin itself in 10.3?

block_content already *has* to be enabled on any site using the plugin; the deriver returns a derivative for each block content type, so if block_content is not enabled there will not be any inline_block plugins available.

The only issue here is the discovery issue with attributes, because the class is now loaded at discovery time, whereas with annotations the docblock was parsed without actually loading the class; the deriver then returned nothing and so the plugin itself was never loaded or instantiated.

See layout_builder_plugin_filter_block_alter(). inline_block has some special behaviour and *only* exists for us in layout builder. moving it to block_content breaks that, that's the reason it works like this.

As written in Slack, I can see a future where we merge the two existing block_content plugin implementations into a new one that uses no or block type only derivatives and works for inline and reusable and not reusable block_content items both, but that sounds like quite a bit of discussion and figuring how exactly that would all work together.

For now, inline_block IMHO either needs to stay in layout_builder or in a new module that depends on both layout_builder and block_content.

Agree now that moving the plugin to block_content won't work.

Made another attempt in MR!5279 where the dependent code is moved to a subclass that only the deriver knows about, but also not sure this is any better either. This is kinda the same as #5 except that only the dependent code is moved.

...but it can't live in the same namespace because discovery will still try to load it.

I give up, none of these solutions are optimal, but still not sure what the best interim fix is. Do we know of any similar instances in contrib that we could try to analyse for a solution as well?

Apart from the fact it involves an upgrade path, why not making layout builder depend on block content + post update to enable it. And then defer the attributes change to the minor after that update so that most sites will do one solidly before the other? Or am I missing something really obvious/not reading the above properly?

Agree with Lee in #9 and others here that I think it makes sense to make layout_builder depend on block_content especially if it's going to make our lives easier!

The same issue has come up with MigrateSource plugins using a Trait on from another module: #3421014: Convert MigrateSource plugin discovery to attributes.

I think this has been addressed by #3502913: Add a fallback classloader that can handle missing traits for attribute discovery. The InlineBlock plugin class also was converted to attributes there.

Oh good point, I wouldn't have realised without re-reading that this was exactly the same problem. Closing as outdated and adding issue credit.

Not sure about closing this. We just ignore the error now, but it's not "fixed" that layout_builder depends on block_content, it's weird and pretty limited if you don't have that enabled. Maybe keep it open and downgrade to normal?

There are been some reports about things failing with the fallback autoloader, such as this slack thread: https://drupal.slack.com/archives/C1BMUQ9U6/p1748549559293769.

It can apparently happen that the missing trait gets autoloaded as the fallback, but is then actually needed later and things fail when the trait doesn't provide methods that classes require for their interfaces.

We agreed to reopen this.

At this point with the upgrade path worries mostly out of the way, I'd say we should just make layout_builder depend on block_content and add an update function that enables it if that's not yet the case.

One alternative would be to move these to the Core namespace. There doesn't seem to be anything particularly block_content specific in this code, either:
Drupal\block_content\Access\AccessGroupAnd
Drupal\block_content\Access\DependentAccessInterface
Drupal\block_content\Access\RefinableDependentAccessInterface
Drupal\block_content\Access\RefinableDependentAccessTrait

Should be pretty easy to do using https://www.drupal.org/node/3509577

+1 to #36, if we move this to core but keep it tagged internal then we solve the problem and I don't think we are adding any new debt.

FWIW, it looks like this Drupal CMS recipe installs layout_builder w/o block_content: https://git.drupalcode.org/project/drupal_cms/-/blob/1.x/recipes/drupal_...

MR 12284 for #36 is up.
Since the test only job wasn't failing in the way expected, created test only MR 12285 that does: https://git.drupalcode.org/issue/drupal-3255804/-/jobs/5417607

CR for the class moves: https://www.drupal.org/node/3527501

This is looking good, great work on the tests!

Aren't the added types a bc break considering that the old classes are automatically aliased to the new ones?

I think PHPStan was complaining about some missing types with the moved classes, so I added them. It is a break, but the classes are all @internal. I can remove the types and put in phpstan ignores as needed if preferred.

Sorry I think we should add the return types in their own issue, especially given the short amount of time before 11.2.0, back to needs work for that.

Looks like fixing docblocks in AccessGroupAnd is enough for PHPStan.

Last couple of commits look good.

Made one more change here directly on the MR - deprecation was for 11.3, but this is breaking things in 11.2 with to the fallback class loader - I'm not entirely sure if the fallback classloader is making things worse, or if it's dealing with a regression elsewhere but failing to fully compensate for it.

Went ahead and committed/pushed this to 11.x and 11.2.x so it's in 11.2.0-rc1, thanks!

For what it's worth, I think there is a duplicate of this issue out there somewhere.