Problem/Motivation

CKEditor 5 will not use the editor.module-andmedia.module provided link, image and media dialog forms:

  1. \Drupal\editor\Form\EditorLinkDialog
  2. \Drupal\editor\Form\EditorImageDialog
  3. \Drupal\media\Form\EditorMediaDialog

There are no longer Drupal-owned dialog forms to add/edit links or images. We did that for the CKEditor 4 integration because it was going to be a dialog in CKEditor anyway. In CKEditor 5, the UX is simpler (not a dialog).

This does mean that certain contributed modules will need to be rearchitected, since all they do with CKEditor 4 is alter one of these forms:

Proposed resolution

Since Drupal 10 will ship without CKEditor 4, and we do not know of any contributed modules that provide alternative text editor plugins that use these, it is probably worth deprecating these in Drupal 9 for removal in Drupal 10.

Remaining tasks

TBD

User interface changes

None.

API changes

Deprecate:

  1. \Drupal\editor\Form\EditorLinkDialog
  2. \Drupal\editor\Form\EditorImageDialog
  3. \Drupal\media\Form\EditorMediaDialog

Data model changes

None.

Release notes snippet

TBD

Issue fork drupal-3231341

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers created an issue. See original summary.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm as a volunteer commented
Issue summary: View changes

 

divesh.kumar made their first commit to this issue’s fork.

gaurav.kapoor’s picture

gaurav.kapoor CreditAttribution: gaurav.kapoor at Axelerant for Drupal India Association commented
Assigned: Unassigned » gaurav.kapoor

gaurav.kapoor opened merge request !1192

gaurav.kapoor closed merge request !1192

gaurav.kapoor opened merge request !1193

gaurav.kapoor’s picture

gaurav.kapoor CreditAttribution: gaurav.kapoor at Axelerant for Drupal India Association commented
Status: Active » Needs review

Hello!!. I am still new and trying to understand the new MR workflow introduced for the issues and also to the D10 readiness initiative. Based on my understanding I have added an MR which has code for mentioned deprecations. Looking forward to feedback regarding the same. Thanks.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Title: Deprecate EditorLinkDialog and EditorImageDialog in Drupal 9, remove in Drupal 10 » Deprecate EditorLinkDialog, EditorImageDialog and EditorMediaDialog in Drupal 9, remove in Drupal 10
Spokje’s picture

Spokje
Primary language Dutch
CreditAttribution: Spokje commented
Assigned: gaurav.kapoor » Spokje
Status: Needs review » Needs work
Spokje’s picture

Spokje
Primary language Dutch
CreditAttribution: Spokje commented
Issue summary: View changes
Spokje’s picture

Spokje
Primary language Dutch
CreditAttribution: Spokje commented
Issue summary: View changes
Spokje’s picture

Spokje
Primary language Dutch
CreditAttribution: Spokje commented
Issue summary: View changes

Spokje opened merge request !2414

Spokje’s picture

Spokje
Primary language Dutch
CreditAttribution: Spokje commented
Assigned: Spokje » Unassigned

Hmmm, 198 deprecation notices.
Are we sure that CKEditor5 is not using those dialogs?
I see quite a lot of deprecation notices in the Drupal\Tests\ckeditor5\Functional namespace

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

I think #3270734: Update Editor + CKEditor 5 module to not use CKEditor 4 in tests having landed will make a huge difference. Let's see. Rebasing…

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

All (or at least the vast majority) of the remaining test failures are due to 

editor.media_dialog:
  path: '/editor/dialog/media/{editor}'
  defaults:
    _form: '\Drupal\media\Form\EditorMediaDialog'
    _title: 'Edit media'
  methods: [POST]
  requirements:
    _entity_access: 'editor.use'

pointing to the deprecated class. Why is that triggering so many failures?

Because \Drupal\Core\EventSubscriber\EntityRouteAlterSubscriber::onRoutingRouteAlterSetType() calls \Drupal\Core\Entity\EntityResolverManager::setRouteOptions() which does

if ($controller = $this->getControllerClass($route->getDefaults())) {

… which loads the class and hence triggers the deprecation 😬

It's been a while since I dealt with this kind of thing so … hoping here that @Spokje knows how to solve this 🤓

Spokje’s picture

Spokje
Primary language Dutch
CreditAttribution: Spokje commented

I can't think of another way, but to skip/suppress those deprecations altogether. Route deprecation never landed and (at least) I have never dealt with these EntityRouteAlterSubscriber/EntityResolverManager shenanigans.

Not very nice, since we _might_ be missing actual use of the deprecated classes, but without hacking into EntityRouteAlterSubscriber/EntityResolverManager, (which IMHO is even worse) I'm out of brilliant ideas.

Very happy for Big Brains to come up with a much better approach.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

I don't have a better idea either, nor does it seem worth our time if the very simple solution in the (brilliantly named!) commit from @Spokje solves it 😊

Spokje’s picture

Spokje
Primary language Dutch
CreditAttribution: Spokje commented
Status: Needs work » Needs review

It's green (which, according to a certain amphibian, is not an easy thing to be...)

Putting this on NR to get committers eyes on the chosen "Oh-shut-up-with-your-silly-deprecation-notices" approach.

Spokje opened merge request !2700

Spokje’s picture

Spokje
Primary language Dutch
CreditAttribution: Spokje commented
Title: Deprecate EditorLinkDialog, EditorImageDialog and EditorMediaDialog in Drupal 9, remove in Drupal 10 » Deprecate EditorLinkDialog, EditorImageDialog and EditorMediaDialog in Drupal 9 for removal in Drupal 10
Version: 10.0.x-dev » 9.5.x-dev

Retitling, correct version and added #3306584: [11.x] Remove EditorLinkDialog, EditorImageDialog and EditorMediaDialog in Drupal 11 for the actual removal in 10.0.x.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Related issues: +#3306584: [11.x] Remove EditorLinkDialog, EditorImageDialog and EditorMediaDialog in Drupal 11
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented

Won't this break the ckeditor contrib module if it's committed?

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm as a volunteer commented

Based on #24 I suggest we deprecate this in 10.1 for removal in 11.0.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Version: 9.5.x-dev » 10.1.x-dev

#24 is totally right 🙈

+1.

Whew, one less thing to do before 10.0.0-beta1! 😄

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Title: Deprecate EditorLinkDialog, EditorImageDialog and EditorMediaDialog in Drupal 9 for removal in Drupal 10 » Deprecate EditorLinkDialog, EditorImageDialog and EditorMediaDialog in Drupal 10.1 for removal in Drupal 11
Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

Tangentially related: #2710427-81: Broken "Allowed Tags" updating: after all values for an attribute are allowed, it should not be overridden to allow only certain attribute values.

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ as a volunteer commented
Status: Needs review » Needs work

MR needs updating to 10.1.x

Spokje closed merge request !2414

Spokje opened merge request !2969

Spokje closed merge request !2700

Spokje’s picture

Spokje
Primary language Dutch
CreditAttribution: Spokje commented
Status: Needs work » Needs review

MR!2969 is against 10.1.x

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Needs review » Reviewed & tested by the community

That looks perfect — thank you @Spokje!

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Agileana commented
Related issues: +#2935897: [policy, then docs] Change how we deprecate classes

 

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Agileana commented
Issue tags: +Needs release manager review

Based on #2935897: [policy, then docs] Change how we deprecate classes, I think we should be triggering the deprecations in these classes' constructors rather than at the top of the file. Tagging for RM review to confirm with others.

Spokje’s picture

Spokje
Primary language Dutch
CreditAttribution: Spokje commented

I think the changes proposed in #2935897: [policy, then docs] Change how we deprecate classes make sense, but (at least) I also think that we can't expect people to make patches/MRs that are based on the official policy, which then are being put back to NW because of something that isn't an official policy (yet).

Then again, n=1.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Agileana commented

@Spokje This might be the first issue where we adopt it. I did leave the issue RTBC for a reason though.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Agileana commented
Status: Reviewed & tested by the community » Needs work

#2935897: [policy, then docs] Change how we deprecate classes was indeed adopted today, so let's move the instantiated classes' deprecations to their constructors. (Don't worry, there's another issue that was NW on this too.)

Spokje’s picture

Spokje
Primary language Dutch
CreditAttribution: Spokje commented

Thanks @xjm

#2935897: [policy, then docs] Change how we deprecate classes was indeed adopted today, so let's move the instantiated classes' deprecations to their constructors.

Not sure if I should check my feet for bullet wounds, but at the very least we got an improvement on the deprecation policy. :)

(Don't worry, there's another issue that was NW on this too.)

I gave up worrying about d.o. issues quite a while ago, so we're all good here.

Spokje’s picture

Spokje
Primary language Dutch
CreditAttribution: Spokje commented
Status: Needs work » Needs review

Do we still need a Release Manager review?

Spokje’s picture

Spokje
Primary language Dutch
CreditAttribution: Spokje commented

Updated CR for 10.1.x/11.0.x

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Needs review » Reviewed & tested by the community
Spokje’s picture

Spokje
Primary language Dutch
CreditAttribution: Spokje commented

Thanks @Wim Leers!

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Issue tags: -Needs release manager review

The route and deprecation message skipping is a bit tricky.

Ideally we'd want to deprecate the route, but this depends on #3159210: Support route aliasing (Symfony 5.4) and allow deprecating the route name.

Also even if we deprecate the route, it wouldn't would help us remove the deprecation skipping. The one thing it might mean is that we've got an un-suppressed deprecation for the route, which would be another way to alert contrib/custom code. Either way that should be a follow-up.

The only other thing I can think of would be moving the deprecation out of the constructor and into EditorImageDialog::buildForm() and EditorImageDialog::skipForm(), this would (probably?) mean it could be non-skipped, but that'd be an even further change to policy and not necessarily more useful.

Then after all that I realised controllers/forms are @internal anyway meaning anything we do here is 'best effort', so if the skipping does mean some custom or contrib code doesn't catch this, well we did our best.

So I think we should open a PP-1 follow-up to deprecate the routes once #3159210: Support route aliasing (Symfony 5.4) and allow deprecating the route name is in, but otherwise agreed this is the best we can do here and untagging for RM review.

Spokje’s picture

Spokje
Primary language Dutch
CreditAttribution: Spokje commented
Related issues: +#3337813: Properly deprecate routes editor.image_dialog, editor.link_dialog and editor.media_dialog

Thanks @catch, opened #3337813: Properly deprecate routes editor.image_dialog, editor.link_dialog and editor.media_dialog to address #45.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Status: Reviewed & tested by the community » Fixed

Committed 15cebd32 and pushed to 10.1.x. Thanks!

  • alexpott committed 15cebd32 on 10.1.x 
    Issue #3231341 by Spokje, gaurav.kapoor, Wim Leers, xjm, catch:...

Spokje closed merge request !2969

Status: Fixed » Closed (fixed)

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