Cache EditorManager::getAttachments()

+++ b/core/modules/editor/src/Plugin/EditorManager.php
@@ -64,6 +76,32 @@ public function listOptions() {
+      foreach ($format_ids as $format_id) {
+        $editor = editor_load($format_id);
+        if (!$editor) {
+          continue;
+        }
+      }

I'm sorry but this piece of code is confusing ... does that do anything? In case it does, it feels magic for me, at least add a comment.

What about fixing the actual code, and make the actual loading code not public and improve the code to even in case of a cache miss, not load invidiual entities over time ...

As wim explained these numbers are a little bit unfair. This seems to be the first thing which loads the entity type definitions.
If this does not load it, something else might?
Let's have a profiling to see how much actually is saved.

Please look at editor_load(), you'll see that this is untrue.

OH, the fuck. I think then not using it is perfect. How likely is that we need all editors on every request?

Seems this is waiting on profiling,but just looking into the fails. Why is the editor settings array different? Do the tests just need to update due to the changes in the patch? Here is an attempt.

The tests are failing since they are testing hook_editor_js_settings_alter(), and the result of this hook is now cached in getAttachments(), providing the test with stale cached settings-data.

Either we move the result of altering the settings through hook_editor_js_settings_alter() outside of caching, or we include it in the cache (as I think is the default way of caching in core) and we have the test explicitly invalidate the cached attachments.

Besides the test failures, when calling EditorManager->clearCachedDefinitions(), only the definitions are cleared, while the attachments-cache remains. This seems like a bug, patch attached that ensures that the attachments-cache is cleared whenever the definitions cache is cleared by setting the editor_list cache tag on the definitions cache backend.

Thanks Wim! Removed the unused use statement and altered the wording on the comment.

Just some driveby review.

+++ b/core/modules/editor/editor.services.yml
@@ -1,7 +1,7 @@
+    arguments: ['@container.namespaces', '@cache.discovery', '@module_handler', '@cache.data', '@entity.manager']
   element.editor:

+++ b/core/modules/editor/src/Plugin/EditorManager.php
@@ -22,20 +25,40 @@
+  public function __construct(\Traversable $namespaces, CacheBackendInterface $discovery_cache, ModuleHandlerInterface $module_handler, CacheBackendInterface $attachments_cache, EntityManagerInterface $entity_manager) {

@@ -64,15 +87,33 @@ public function listOptions() {
+    $editors = $this->entityManager->getStorage('editor')->loadMultiple($format_ids);

Let's use the entity type manager.

Thanks dawehner, replaced EntityManager with EntityTypeManager.

+++ b/core/modules/editor/src/Plugin/EditorManager.php
@@ -64,15 +86,33 @@ public function listOptions() {
+    $cid = 'editor_attachments:' . implode($format_ids);
+    if ($cache = $this->attachmentsCache->get($cid)) {
+      $attachments = $cache->data;
+    }

I presume we need tests for this bit of additional code.

if still a valid task can we get an updated issue summary please. What exactly is being proposed to fix?

Updated the issue summary, I doubt anything much has changed here looking at editor_load() in 9.5.x, so probably original findings are valid but we'll need to validate.