Optimize the getMentionsFromEntity() method

Definitely makes sense! I wonder if we could specify this even more through checking whether the field widget actually uses CKEditor.

This way, we avoid the Text fields that don't even use CKEditor. If this introduces too much overhead, we can always use the current approach as it improves the current state a LOT.

Actually, I would really like to merge this feature as is. What do you think about this @dinazaur?

This change would theoretically break compatibility with any text fields that do not extend "TextItemBase". But all core text fields and any other important contrib text fields I know, are extending TextItemBase anyway.

Definitely makes sense! I wonder if we could specify this even more through checking whether the field widget actually uses CKEditor.

We are already doing this, check the getTexformatsUsingMentions method, it will return only the formats that use CKEditor5 and have the mentions plugin enabled.

But I think loading all formats when saving any entity with fields is also a bit of overhead. I would like to add one more optimization.

I removed unused dependency injections and replaced the use of getTexformatsUsingMentions for each entity with a lazier and cacheable approach in isFormatMentionable.

I also replaced my own check for inheritance from TextItemBase::class with a call to the _editor_get_formatted_text_fields function from core. This removes from us the responsibility of deciding what should be considered a text field, but adds a few extra iterations, since first the function prepares the fields, and then we iterate over them. However, these are very small overhead costs, and in the future the core might make more active use of generators for such functions, and this drawback may disappear entirely.

Great stuff! Exactly what I had in mind! Also didn't know about "_editor_get_formatted_text_fields". Good stuff!

Although, I am not a big fan of the in memory caching for "mentionableFormats". It is a good idea to cache the formats, to have less performance overhead, but I don't like using a class variable for caching. I'd rather use the Drupal caching system instead!

Otherwise, LGTM!

Thank you for the feedback. I agree that a persistent cache would further improve performance, so I have added it.

What do you think about removing the getTextformatsUsingMentions() method?

I don't like using a class variable for caching, since the "caching" is only per PHP request. I'd rather use the Drupal caching system instead!

Regarding the in-memory caching, I see your point about per-request caching being limited. However, Drupal itself often uses memory caching to avoid repeatedly loading the cache from the database. In this case, it’s a similar situation: editor entities are already cached and likely loaded during text field save operations, and our calculations to determine which formats can be mentioned are not very heavy. So using a class variable for per-request caching still provides a small performance benefit without adding complexity.

Changes are looking good! Thank you! :)

Commented a few more things, otherwise this issue looks good to go afterward!

@kksandr one last thing, we can remove "getTexformatsUsingMentions", but there is also a test covering the method in "CkeditorMentionsEventsTest" could you add a replacement covering "isFormatMentionable"?

Only if it is done quickly.

I removed the getTexformatsUsingMentions() method and updated the test to cover isFormatMentionable().

After reviewing issue 🌱 [policy] Standardize how we implement in-memory caches Needs work , I decided to replace the custom static cache implementation with BackendChain to combine cache backends, similar to what jsonapi does. This removes the need for a custom resetCache() method and allows us to fully delegate cache invalidation to the core.

Very nice! LGTM!

Let's wait for the tests to pass. People had problems in the past with newly injected dependencies on already existing services. We never had a problem with that. Simply clearing all caches resolved the issue, although we introduced running "DrupalKernel::rebuildContainer" inside an update hook once, which fixed the issue for people having these problems. I am thinking whether we should introduce such an update hook, but I personally find it very unnecessary.

RTBC.

Tests are green! Merging.

Now that this issue is closed, please review the contribution record.

As a contributor, attribute any organization helped you, or if you volunteered your own time.

Maintainers, please credit people who helped resolve this issue.

Thanks again @kksandr very informative fixes as well!