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.
Comment | File | Size | Author |
---|---|---|---|
#6 | interdiff.txt | 2.11 KB | Wim Leers |
#6 | editor-2204129-6.patch | 7.57 KB | Wim Leers |
Comments
Comment #1
tim.plunkettI 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.
Comment #2
Wim LeersOverall +1 of course.
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 sincefilter_caption
is provided by the Filter module, this is always available.Genius simplification — thank you :)
In the interdiff:
Comment #3
Wim LeersComment #4
Wim LeersDidn't mean to RTBC.
Comment #6
Wim LeersThe failures are correct/expected; they happen because we modify a
FilterFormat
object, which then still remains statically cached by anEditor
object, hence resulting in a failure.Comment #7
Gábor HojtsyLooks like great cleanup, but...
The only thing that stands out is the empty filter format case is handled before the patch but not after.
Comment #8
Wim Leers$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.Comment #9
Gábor HojtsyWell, its clearly not happening on our current code paths. I take your word for that, no other issue so looks good :)
Comment #10
tim.plunkettEditor::$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.
Comment #11
webchickHm. 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!
Comment #12
tim.plunkettWell, 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"...
Comment #13
Wim LeersI 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! :)