\Drupal\block_content\Plugin\Derivative\BlockContent::getDerivativeDefinitions could use an aggregate query instead of loadMultiple

Tested this on a large site with over 3000+ block content entities where I was running into OOM issues.

After saving a block and therefore having the Block definition caches cleared:

No patch: ~152MB memory usage
With patch: 76MB memory usage

This looks great! One point on the MR to add a code comment. I still think we should do 🐛 block_content block derivatives do not scale to thousands of block_content entities Needs work but this will stop a lot of sites crashing in the meantime.

Added a comment

Looks great. We've got the other issue(s) open to move away from derivatives, I think we could backport this to 10.2 as well.

What happens when you add translations? I suspect that a translation with a different info/label will result in a separate key, and then it gets unpredictable which one you actually end up with? Maybe add a default langcode condition?

FWIW, this feels like a misuse of aggregate queries to me, but it is obviously a big improvement, so...

FWIW, this feels like a misuse of aggregate queries to me, but it is obviously a big improvement, so...

Can you elaborate on why, I do this fairly regularly in contrib and custom code. Trying to understand the implications.

Maybe add a default langcode condition?

Done

> Can you elaborate on why, I do this fairly regularly in contrib and custom code. Trying to understand the implications.

We always say that the only correct way to interact with entities is to load them, so that the appropriate hooks are running which *might* alter the data or also things like language negotiation*. We do not support any partial loading of field values or direct DB queries and an aggregate query is essentially that. Usually an aggregate query is combined with an aggregation function, where you count all nodes grouped by type or some other field and don't access the raw values. This is different.

*I just realized that this isn't working at all right now anyway, block content labels always show up in the default language only (we kind of implicitly assume in most plugin caches that all labels are t() objects and translated when accessed so we don't cache per language), that's not the case here. This is already not working in any useful way in HEAD, but I could imagine something like a ContentTranslation object that you feed with all the translations of a given content entity that then acts like a TranslatableMarkup object.

We always say that the only correct way to interact with entities is to load them, so that the appropriate hooks are running which *might* alter the data or also things like language negotiation*

Thanks, I was thinking that might be the reason, so its good to get confirmation.

Does that mean that this approach is a BC break, because someone could be implementing a load hook for block content and modifying the titles? Do we need to put this behind a feature flag that's opt-in? Thoughts @catch with your RM hat on?

I'm not aware of how trash module works but I got a bug report for a contrib module that leads me to suspect it might unset entities in a load hook if they're trashed. That might be something that has a side effect here.

Seems 10.2.3 is out. Should this go to 10.3?

Per @larowlan in #12.

Patch doesn't seem to apply anymore.

Rebased and fixed the PHPCS issue, also updated the deprecation message and CR.

Random failures seem to be biting us hard here. Will leave it for now and rerun later.

Indeed was random, re-running fixed.

left 1 comment on MR.

Replying to the bc question from... this time last year.

The main case for block titles being altered would be previewing forward revisions, but workspaces will handle that when you're in a workspace via entity query rewrites.

For trash module I think that it will rewrite entity queries to not include the entity when it's trashed - but we should ideally check with @amateescu.

I think this is a sufficiently serious issue that we should not put the fix behind a feature flag but would be good to confirm the trash case first.