Hidden dependency on block_content in layout_builder

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 🐛 block_content block derivatives do not scale to thousands of block_content entities Needs work 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 attributes 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.

longwave → closed merge request !5276

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

Made another attempt in MR!5276 where the dependent code is moved to a subclass that only the deriver knows about, but also not sure this is any better either.

...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: 📌 Convert MigrateSource plugin discovery to attributes Needs work .