Learning about CKEditor 5 from reviewing a Drupal patch
“CKEdtior 5 is coming!” Just like the “Winter is coming!” meme from Game of Thrones, the upgrade from CKEditor 4 to CKEditor 5 has been shouted by many Drupal developers over the last year or so. Since the Drupal 9 end-of-life deadline is approaching in November 2023, these shouts have gotten louder.
And the loudest shouts always seem to come from devs who expect to see modules already upgraded to CKEditor 5 compatibility when they check the upgrade status table. “When will this be merged?” changes from a shout, to an impassioned plea, to a whimper…and then…and then… “Well maybe I will have a look at this patch and see if I can fix things myself” is whispered with a grunt.
This post will go over my journey learning how to use CKEditor 5 while reviewing and trying to improve a patch for the CKEditor Templates module.
I have looked a little into CKEditor 5, but that was months ago and just like most devs, I’ve completely forgotten anything I learned in lieu of cramming new tech trends into my brain.
Review the issue
The first thing I did to orient myself was to review the issue queue and try to figure out where the patch was at. I chimed in pretty early at comment #56…well, pretty early for a Drupal issue thread anyways.
My comment:
@Harlor hopefully you get this notification as I think other comments misspelled your handle…would be nice if drupal.org helped us complete usernames, but oh well. This thread has a few missed comments where people were one/two letters off the actual screen name. I will also ping @sonnykt as I think you meant to ask them a question.
But my main question is if your MR is good to be reviewed since you moved the status from “Needs Review” to “Needs Work”?
I’m trying to familiarize myself with the module now, but I can help review it if you are paused. I just don’t want to look too hard if you are planning to add updates soon.
I basically tried to notify the people who were working on the issue most recently, in case the autocomplete tagging on d.o. wasn’t working (I still don’t see this feature in my browser), and I plainly asked what needs work since I was new to the issue.
If you start reviewing things before you ask a question like this, you’ll likely waste time since you have no context on what the authors of the patch are trying to accomplish.
The reply:
@afinnarn, There is a username completion ;)
Did I miss something? I changes the status to needs work because there are at least the issues mentioned in #52 and #53.
Also I think we need to install the new entity type:
Great! Now I know which comments to focus on when I look at the code and what the author pushing the patch thinks is important to fix.
Review the code
Drupal switching to GitLab has made reviewing Drupal patches/merge requests much nicer than the old user interface. I use GitHub currently for work, but the platforms are so similar, it wasn’t hard for me to sort through the code in a pretty-looking UI.
Looking through the list of file changes, I skipped past the pieces I didn’t need to look much into before testing the code. I’ve worked through styling updates and schemas changes in the past, but the CKEdtior-related code changes were new to me.
CKEditor 5 JS Code
/**
* Generates a CKEditor 5 plugin.
*/
export default class CKEditorTemplates extends Plugin {
/**
* @inheritdoc
*/
static get requires() {
return [
CKEditorTemplatesUI,
CKEditorTemplatesEditing
];
}From that code, I now think there are at least two components to consider when making a CKEditor 5 plugin: a UI people use to enter content and a class to handle the upcasting/downcasting…but who knows, this is just my naive guess.
/**
* Command for injecting code into the CKEditor.
*/
export default class CKEditorTemplatesCommand extends Command {
refresh() {}
execute(htmlCode, replace) {}
}Next, we have a command that injects the code from our editing UI into CKEditor alongside other code. It implements two functions that I’ve left out for brevity’s sake, but I’ll take another guess at what they do.
refresh() looks like it checks something in the selection of text before the editing UI is loaded. If it doesn’t find the proper markup surrounding the selection, then you probably can’t use the CKEdtitor Templates editing UI.
execute(htmlCode, replace) looks like it inserts whatever you've added to the editing UI into CKEdtior content. If replace === true, then the code will update all of the data/model, but if the addition is new, then a new “fragment” is added to the existing CKEditor data/model.
import CKEditorTemplatesCommand from './ckeditorTemplatesCommand';
/**
* Handles the plugin functionality.
*/
export default class CKEditorTemplatesEditing extends Plugin {
init() {}
}Our command is added to CKEditor via the `CKEditorTemplatesEditing` class…so I was a bit wrong in guessing this class’s purpose. It should probably be renamed to `CKEditorTemplatesPluginRegistration` since the class’s responsibility is adding the command to CKeditor and has nothing to do with actual editing.
/**
* Generates a toolbar button.
*/
export default class EmbeddedContentUI extends Plugin {
init()
}Another class with just an init()…great…but that’s CKEditor’s plugin system I guess. This class clearly adds a button to the toolbar, listens for a click, and then opens a dialog passing the appropriate data to it.
Not bad. I generally understand what’s going on in the first pass-through. Now let’s move on to the Drupal/PHP code.
Drupal CKEditor 5 Code
/**
* Defines ckeditor_template annotation object.
*
* @Annotation
*/
class CkeditorTemplate extends Plugin {
public $id;
// But this is an "int"? Can't be null then...
public $weight = NULL;
public $label;
public $description;
}The first class I find seems to be some configuration class. It would be nice to use PHP 8+ attributes since Drupal requires PHP 8 now, but someday…someday.
Looks like the CKEditor Templates objects/instances have a few properties on them: id, weight, label, and description. These values (other than ID) are probably configurable by a user in a UI somewhere.
To note, the weight is reported as an integer but the code has it set to a NULL value. It should be 0 and I will suggest that when I get around to trying to update this merge request.
On second glance, since we are extending the Plugin class, I think this is a base definition of a Drupal plugin. So, the doc block comment should probably be updated to reflect that distinction from just declaring an annotation…but whatever the case, this class is confusing to me as-is.
/**
* Bunch of annotations...
*/
class CKEditorTemplates extends ConfigEntityBase implements CKEditorTemplatesInterface {
// Several properties...
protected $label;
protected $status;
protected $description;
protected $thumb;
// Most important variable.
protected $code;
// Bunch of other stuff...
}The config entity class makes a lot more sense to me since as a Drupal developer, I deal with entities (content or config) all the time. Unlike with plugins, where I rarely have to create them or read their source code.
The most interesting property is the code variable where the template code will be stored. Surrounding the code property are several pieces of metadata like a description or thumbnail.
And…there’s a bunch more code that I won’t go over. It looks standard and what you need to do to find the templates from config storage, load them into the CKEditor UI, and inject the HMTL.
Alternative Solutions
When you are reviewing a patch and considering whether or not you still need to use the module…like when going up major versions of Drupal…sometimes you see things or have “aha!” moments that make you stop and think.
My “aha!” moment was watching another contractor explain how to inject an HTML template into CKEditor without using the ckeditor_templates module that was installed and enabled on the Drupal site. I’m not sure if they knew about the ckeditor_templates module and features or if they just preferred to do it “their way”, but it got me thinking.
With the current way the ckeditor_templates module UI is set up, you can’t provide as much information as you can in a documentation tool or even something like Storybook.
So, my “solution” to reviewing this patch is to just leave it where it’s at and try to convince my company to use the simpler approach of copy/paste without needing another Drupal module to cover that functionality. Who knows, in the future the same issue might come up when needing to migrate to Drupal 11 and CKeditor 6. But if that were the case, the copy/paste solution “works as expected”. Good enough to convince me!
🤷♂
