Updated: Comment #N

Problem/Motivation

The editor entity has a 1:1 relationship with a filter format, even its ID matches exactly.
Almost any time an editor entity is used, its corresponding filter format entity is loaded.

However, calling code throughout editor.module has entity_load('filter_format', $editor->format), which is bad for at least three reasons.

Proposed resolution

Add an EditorInterface::getFilterFormat() method that loads the filter format, stores it in a protected property, and returns it.

Remaining tasks

Decide if EditorInterface should have a setFilterFormat(), and call it in editor_form_filter_format_form_alter().

User interface changes

N/A

API changes

API addition, calling code should use the new EditorInterface::getFilterFormat() method.
Any modules swapping out the Editor entity class for their own (?!) will need to implement this new method.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett’s picture

Issue summary: View changes
FileSize
4.46 KB

I found this while looking into PluginBag changes.
In HEAD, loading admin/config/content/formats/manage/basic_html will instantiate the FilterBag five times.
Once for the filter format form itself.

Then once in ckeditor_ckeditor_css_alter().

CKEditorPluginManager::getEnabledPluginFiles() called 2x by CKEditor::getJSSettings() which call:
DrupalImageCaption::isEnabled() x 2
Internal::generateAllowedContentSetting() x 1

which each load the format again.

With the patch, the FilterBag is only instantiated two times.

If we chose to pass the loaded format entity to the editor entity in editor_form_filter_format_form_alter(), we could cut it all the way down to 1 time.
However, I didn't want to add a setFilterFormat() method to the interface, since it would just be for saving the extra load, and would never be called on runtime.

Wim Leers’s picture

Status: Active » Reviewed & tested by the community
Issue tags: +Spark
FileSize
5.5 KB
2.04 KB

Overall +1 of course.

+++ b/core/modules/ckeditor/ckeditor.module
@@ -48,17 +48,10 @@ function ckeditor_theme() {
-  if (isset($filters['filter_caption']) && $filters['filter_caption']->status) {
+  if ($editor->getFilterFormat()->filters('filter_caption')->status) {

At first I thought this might not work if the filter_caption filter wasn't enabled for a given text format, but then I realized that this always lists all available filters, and since filter_caption is provided by the Filter module, this is always available.

Genius simplification — thank you :)


In the interdiff:

  1. Where 80 col rules allow, we always say "text editor", not "editor". So changed that for the additions in this patch.
  2. In doing so, I noticed that — GASP — the text editor entity is called "text editor entity" in the entity docblock, but not in the label. So fixed that too.
  3. Fixed: A text editor does not "use" a text format, a text editor is associated with a text format.
Wim Leers’s picture

Issue tags: +sprint
Wim Leers’s picture

Status: Reviewed & tested by the community » Needs review

Didn't mean to RTBC.

Status: Needs review » Needs work

The last submitted patch, 2: editor-2204129-2.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
7.57 KB
2.11 KB

The failures are correct/expected; they happen because we modify a FilterFormat object, which then still remains statically cached by an Editor object, hence resulting in a failure.

Gábor Hojtsy’s picture

Status: Needs review » Needs work

Looks like great cleanup, but...

+++ b/core/modules/ckeditor/ckeditor.module
@@ -48,17 +48,10 @@ function ckeditor_theme() {
-    if (!empty($editor->format)) {
-      $filters = entity_load('filter_format', $editor->format)
-        ->filters()
-        ->getAll();
-    }

+++ b/core/modules/editor/lib/Drupal/editor/Entity/Editor.php
@@ -75,4 +82,14 @@ public function __construct(array $values, $entity_type) {
+    if (!$this->filterFormat) {
+      $this->filterFormat = \Drupal::entityManager()->getStorageController('filter_format')->load($this->format);
+    }

The only thing that stands out is the empty filter format case is handled before the patch but not after.

Wim Leers’s picture

Status: Needs work » Needs review

$editor->format is never empty, because an editor always is associated with a text format. In other words: I don't know why we had that check in the first place! That's why there's 0 fails, 0 exceptions.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Well, its clearly not happening on our current code paths. I take your word for that, no other issue so looks good :)

tim.plunkett’s picture

Editor::$format is the ID entity key, instead of 'id', see the annotation.
Since a config entity must have an ID to be saved, we can rely on this.

We could consider opening a followup to replace $editor->format with $editor->id() elsewhere to remove this confusion.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Hm. Not sure. I would never in a million years guess that $editor->id() would be a format ID. So while it's non-standard (which I don't particularly like), it definitely makes the calling code a lot more obvious/readable (which I do).

This looks like an ok API addition that gets us out of checking arrayPIs, so..

Committed and pushed to 8.x. Thanks!

tim.plunkett’s picture

Well, it's really weird that an editor always has a 1-to-1 match with a format, and therefore has an identical entity ID.
But if you take that weirdness as an architectural necessity, the rest makes "sense"...

Wim Leers’s picture

Issue tags: -sprint

I agree that it's weird. IIRC quicksketch and others wanted it to work this way, and I didn't consider it important enough to have a bikeshed over, so I conceded, so that's what we still have today :)

In any case: thanks Tim! :)

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.