Deprecate core/modules/editor/js/editor.admin.js JS APIs in Drupal 10, for removal in Drupal 11

Drupal 11 is getting close, so … let's finally get this done 🙈

CR created: https://www.drupal.org/node/3422372 →

Followed the guidelines in https://www.drupal.org/node/3082634 → to deprecate JS APIs.

Ran the javascript tests 3 times and they still fail so believe they are legit to the change.

@smustgrave Yes, they still fail because core is still calling these APIs. And it has to, to avoid breaking BC. Which is what @lauriii indicated in his review on the MR.

it could be tricky since I'm not sure there's a good way for us to retrieve the currently registered event listeners.

Indeed. There is no way AFAICT.

But there is one thing we can do, which I'd like to get @lauriii's thoughts on:

1. editor.admin.js is loaded automatically for FilterFormatEditForm thanks to editor_form_filter_format_form_alter() doing
   

       '#attached' => [
         'library' => [
           'editor/drupal.editor.admin',
         ],
       ],
   

2. but it's already the responsibility of each text editor that uses these APIs to depend on the editor/drupal.editor.admin asset library (otherwise it's possible it be loaded too late!) — the CKEditor 4 module does that too for example: https://git.drupalcode.org/project/ckeditor/-/blob/1.0.1/ckeditor.librar...
3. therefore the solution is fairly simple: remove the auto-loading in #1, and instead rely on asset library dependencies, which it should've been doing all along

(AFAICT the reason the auto-loading was still present: #1894644: Unidirectional editor configuration -> filter settings syncing → introduced that asset library and predates strong use of dependencies, which happened a year later, in #1996238: Replace hook_library_info() by *.libraries.yml file → .)

Implemented that.

Looks like core/modules/filter/filter.filter_html.admin.js has dependency on it, but it's not declaring it's dependencies correctly... 😅

That's because I should've deleted that too 😅

Note that this now blocks #3239012, see #3239012-35: [PP-1] [11.x] Remove the CKEditor 4 upgrade path → .

Seems to have a failing Filter javascript test.

Ah, the sole failing test was one added (in #2556069: JS error with elements in "allowed HTML tags" that can't be direct descendants of a div → ) for the functionality this is deprecating and no longer executing by default. It was to avoid this functionality regressing to the same bug again, but since this JS functionality is A) deprecated, B) has not been touched in years and won't be, there is no more need for this regression test 👍

I don't know why but I chuckled reading "They were buggy ever since they were introduced in Drupal 8.0". Tests are showing green and deprecation links appear correct

LGTM!

already knew it was not great. It was a necessary evil. The follow-up work to make it decent (#2567801: Deprecate core/modules/editor/js/editor.admin.js JS APIs in Drupal 10, for removal in Drupal 11) never happened.

This links back to here instead of linking to the issue. Not really a commit blocker but nice not to have circles.

Text Filters reacting to Text Editors
drupalEditorFeatureAdded → 1 result: linkit
drupalEditorFeatureRemoved → 1 result: linkit
drupalEditorFeatureModified → 0 results
Conclusion: only one.

Is this in linkit's CKEditor 4 integration? I checked and it's in the latest branch. If it's only for ckeditor4 integration then I think we just need a follow-up since maybe linkit can drop ckeditor4 support at this point rather than trying to update for this, if they're somehow relying on this for ckeditor5, then I thnk we would have to deprecate for removal in Drupal 12 at this point.

#32: see #17 — I repurposed this issue 😅 Clarified that 👍

Is this in linkit's CKEditor 4 integration? I checked and it's in the latest branch. If it's only for ckeditor4 integration then I think we just need a follow-up since maybe linkit can drop ckeditor4 support at this point rather than trying to update for this, if they're somehow relying on this for ckeditor5, then I thnk we would have to deprecate for removal in Drupal 12 at this point.

You're >right. Yes, it's only for CKEditor 4. It can only be for CKEditor 4, because CKEditor 5 is not using any of the JS APIs that are being removed here.

Note that LinkIt 6 supports both CKEditor 4 and 5 at our request: because it makes switching from 4 to 5 a lot less painful (both Editor plugins must be available simultaneously to be able to switch without shenanigans). I don't think a follow-up is necessary; this will just mean that the automatic updating of the filter_html format will stop working if you're setting up a NEW text format+editor to use CKEditor 4. But we're only deprecating it here (in Drupal 10.3), not removing it yet. You won't be able to have CKEditor 4 on Drupal 11 at all, so it getting removed then won't break anything 👍

Thanks for confirming. linkit presumably will need to remove its own ckeditor4 support but that should probably happen in a branch that drops support for Drupal 10, so a bit further ahead than us doing so in Drupal 11.

Committed/pushed to 11.x and cherry-picked to 10.3.x, thanks!

Am a bit confused, but shouldn't the 11.x commit remove the core/modules/editor/js/editor.admin.js JS APIs?
Or is this happening in a follow-up?
Or am I just not reading this correctly?

Thanks! Really nice to see this code removed from Drupal core, it's probably the code I got in core that I was most ashamed of, so … relieved to see it gone 🌅 😄

CR published.

Am a bit confused, but shouldn't the 11.x commit remove the core/modules/editor/js/editor.admin.js JS APIs?
Or is this happening in a follow-up?
Or am I just not reading this correctly?

Since we've only just started removing deprecated code from 11.x, I've been assuming that new deprecations for 11.x removal will get caught by 'remove deprecated code in x' module issues or a final sweep before beta. It would be fine to do 11.x removal and 10.3.x deprecation MRs on one issue, but also there aren't many things we're deprecated for 11.x removal any more, and don't want to encourage people to do separate removal MRs for issues where we end up deprecated for removal in 12.x after all.

I've been assuming that new deprecations for 11.x removal will get caught by 'remove deprecated code in x' module issues or a final sweep before beta.

Same!

Especially for this one, where removal will be trivial 👍 (Delete a few lines in editor.libraries.yml, delete two *.js files, done!)

Since we've only just started removing deprecated code from 11.x, I've been assuming that new deprecations for 11.x removal will get caught by 'remove deprecated code in x' module issues or a final sweep before beta.

Fine by me, although I think this is optimistic and at some point (maybe not in 10.3.x, but still, at some point) we will discover a big BC-issue at the last minute when doing such a final scan.

Then again, there's always a fine balance between too many and too few rules and extra work for deprecations, so let's run with this :)

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