Figure out how to prevent data loss during upgrade/migration path

The scary thing is that there is no upgrade/migration path. We physically can't retroactively modify every revision of every processed_text field.

So it's an at-runtime risk.

I think this means that without something else in place first, we can't automatically migrate existing text formats from CKE4 to CKE5

I think this is correct. We need to analyze the filter_html filter's allowed_html setting and cross-reference this against the HTML tags supported by CKEditor 5 plugins.

Whenever we encounter a particular HTML tag, attribute or attribute value that CKEditor 5 does not support, we cannot automatically enable CKEditor 5.

However, when CKEditor 5 supports a superset of those HTML tags, then it is safe to update — automatically or manually.

Proposed auditing/analysis algorithm

1. Iterate over all text formats, and then for each text format, do the following:
2. Check if it has an associated editor config entity; if not: DONE, otherwise: CONTINUE
3. Check if the associated editor config entity uses ckeditor; if not: DONE, otherwise: CONTINUE
4. Check if the \Drupal\filter\FilterFormatInterface::getHtmlRestrictions() return value. Iff it's FALSE any HTML is allowed, this means this format is (similar to) the infamous Full HTML text format and we cannot make CKEditor 5 work for this, which means we're DONE. Otherwise: CONTINUE
5. Do an array_intersect_key($html_restrictions['allowed'], array_flip($cke5_supported_tags)) — if the intersection contains fewer tags than $html_restrictions['allowed'], this means CKEditor 5 does not yet support all these tags. WARN the user, and we're DONE.
6. Repeat the previous step, but now for attributes.
7. Repeat the previous step, but now for attribute values.

IMHO it's crucial we build this auditing logic soon so that we can ensure the CKEditor 5 infrastructure (i.e. this module) has the necessary level of detail in CKEditor 5 Plugin metadata to allow us to do this auditing at all.

EDIT: the cool thing is that because for CKE5 Plugins we have the necessary metadata in PHP (unlike in CKE4!), we can actually write a drush command that automatically analyzes this! And we can provide a Status Report entry too!

(Now: auditing, future: uneventful manual & automatic updates from CKE4 to CKE5.)

I referred to this over at #3200008: Validation and editor settings. See https://www.drupal.org/project/ckeditor5/issues/3200008#mr9-note16328.

So let's start implementing #4 by first ensuring that the manual upgrade path is working correctly: when either all HTML is allowed or not enough HTML is allowed, we should disallow using CKEditor 5.

1. The starting point needs to be \Drupal\ckeditor5\Plugin\CKEditor5PluginManager::getAllowedElements(). It is only inspecting the filter_html filter's HTML restrictions. But there can be any number of filters adding HTML restrictions! That's why this should be using \Drupal\filter\Entity\FilterFormat::getHtmlRestrictions() instead.
   (For example: \Drupal\filter\Plugin\Filter\FilterHtmlEscape allows no tags at all — meaning CKEditor cannot possibly work. The proposed change above would detect this.)
2. The second problem is that \Drupal\ckeditor5\AdminUi::validateAllowedElements() has special logic to take over filter_html. I 100% agree this is the 95% use case, so we should special case it: we should have logic to automatically configure it. But that doesn't mean that's enough. We need to verify that all HTML restrictions originate only from the filter_html filter. If that's not the case, we'll need to provide a form-level (instead of form element-level) validation warning too, which can and should say which filters are the ones that are conflicting.
3. Actually taking over that part of the config UI is out of scope here, for that we already have #3201641: Improve the HTML filter configuration UX.
4. Next, we should ensure that we warn the user before they've spent a lot of time configuring CKEditor 5.
5. Finally, the logic we write should be abstracted/independent enough for both future config validation logic and an automatic upgrade path to be able to use this.

Pushed an initial commit for this. That already implements bullet 4, and part of #4.

When switching Full HTML to CKEditor 5:

When switching Restricted HTML to CKEditor 5:

@zrpnr: I've left three // @todo Test coverage comments and two // @todo Automatically deduce which filter comments. Feel free to tackle those if you have the time — I won't have time to work on CKEditor 5 in the next few days.

#5 does not yet handle the case of e.g. starting with Basic HTML, removing <p> <br> from the list of allowed HTML tags, and then switching from CKEditor 4 to CKEditor 5. The code in https://git.drupalcode.org/issue/ckeditor5-3201637/-/commit/87b36391e47f... uses $form_state->getFormObject()->getEntity(), but it should really be using the submitted values — which includes potential filter settings changes.

I run into two problems there WRT Form API:

1. To be able to use that, we need to call \Drupal\ckeditor5\AdminUi::getSubmittedEditorAndFilterFormat(). But calling that triggers a
   

   The subform and parent form must contain the #parents property, which must be an array. Try calling this method from a #process callback instead.
   

   exception from \Drupal\Core\Form\SubformState::getParents(). That is a known core issue: #2798261: Using $form_state->getValue() in BlockBase's blockForm throws "subform and parent form must contain the #parents property" exception.

   I can work around it by doing

       // Work-around for https://www.drupal.org/project/drupal/issues/2798261.
       if ($form_state instanceof SubformState) {
         $form_state = $form_state->getCompleteFormState();
       }
   

   … but that's not ideal of course.

2. Now that I have that work-around, my code can work, except $form_state->values() does not contain the filters top-level key even though it was submitted. This is because editor_form_filter_format_form_alter() sets #limit_validation_errors. We still want validation errors to be limited, we just need the current form values … and so … we can get them using FormState::getUserInput().

Both of these are complex/edge-case-y uses of Form API, and I'm hopeful @tim.plunkett can help out with both! 🤞 See https://git.drupalcode.org/issue/ckeditor5-3201637/-/commit/f8bc5f5ade47....

I also don't think the getUserInput() approach is a hack. It seems like a correct usage of the API. Still always good to heavily document that it's not typically called...

WFM and TIL 😄

One question: as a public method on a service, is there any reason to keep getSubmittedEditorAndFilterFormat() as static?

Yes — it guarantees that this method does not depend on any other inputs than the parameters it is given.

Thanks to #9, both blockers in #7 have been addressed 🥳 That means

#5 does not yet handle the case of e.g. starting with Basic HTML, removing <p> <br> from the list of allowed HTML tags, and then switching from CKEditor 4 to CKEditor 5. The code in https://git.drupalcode.org/issue/ckeditor5-3201637/-/commit/87b36391e47f... uses $form_state->getFormObject()->getEntity(), but it should really be using the submitted values — which includes potential filter settings changes.

is fixed 👍

I've also implemented the logic to identify which filter is the culprit:

Before

After

With the changes I pushed today:

1. #5.1 has been addressed:
   

   The starting point needs to be \Drupal\ckeditor5\Plugin\CKEditor5PluginManager::getAllowedElements(). It is only inspecting the filter_html filter's HTML restrictions. But there can be any number of filters adding HTML restrictions! That's why this should be using \Drupal\filter\Entity\FilterFormat::getHtmlRestrictions() instead.
   (For example: \Drupal\filter\Plugin\Filter\FilterHtmlEscape allows no tags at all — meaning CKEditor cannot possibly work. The proposed change above would detect this.)

2. Even if the initial compatibility check did not run, it now runs upon validation, always. (This also paves the path for config-level validation in the future.)
3. It now is explicitly checking the TYPE_MARKUP_LANGUAGE filters edge case:
   
4. A lot of infrastructure preparations already are in there!

My next steps are:

1. generalize \Drupal\ckeditor5\AdminUi::validateAllowedElements() to no longer be tied to filter_html, which will address #5.2:
   

   The second problem is that \Drupal\ckeditor5\AdminUi::validateAllowedElements() has special logic to take over filter_html. I 100% agree this is the 95% use case, so we should special case it: we should have logic to automatically configure it. But that doesn't mean that's enough. We need to verify that all HTML restrictions originate only from the filter_html filter. If that's not the case, we'll need to provide a form-level (instead of form element-level) validation warning too, which can and should say which filters are the ones that are conflicting.

2. move the (many) static methods I added to the CKEditor5 class out of that class, into a separate one, perhaps called CKEditor5CompatibilityChecker?

#5.3 is out of scope here, #5.4 is already the case since #7 and is now even more solidly the case, #5.5 is specifically being taken into account as you can see in my report. :)

Expect things to really come together in the next few days 🤓

#11 said addressing #5.2 was next. That happened in 3832a57b128a7274144c73386bbe5bf8824a640a.

I have not yet moved the many static methods out of the CKEditor5 class into a separate class/service.

But … I do have unified logic: I am now reusing the same logic for both validating CKEditor 5 within Drupal, and within a newly introduced drush ckeditor5:audit command:

I wrote this in #4, nearly two months ago:

Proposed auditing/analysis algorithm

1. Iterate over all text formats, and then for each text format, do the following:
2. Check if it has an associated editor config entity; if not: DONE, otherwise: CONTINUE
3. Check if the associated editor config entity uses ckeditor; if not: DONE, otherwise: CONTINUE
4. Check if the \Drupal\filter\FilterFormatInterface::getHtmlRestrictions() return value. Iff it's FALSE any HTML is allowed, this means this format is (similar to) the infamous Full HTML text format and we cannot make CKEditor 5 work for this, which means we're DONE. Otherwise: CONTINUE
5. Do an array_intersect_key($html_restrictions['allowed'], array_flip($cke5_supported_tags)) — if the intersection contains fewer tags than $html_restrictions['allowed'], this means CKEditor 5 does not yet support all these tags. WARN the user, and we're DONE.
6. Repeat the previous step, but now for attributes.
7. Repeat the previous step, but now for attribute values.

IMHO it's crucial we build this auditing logic soon so that we can ensure the CKEditor 5 infrastructure (i.e. this module) has the necessary level of detail in CKEditor 5 Plugin metadata to allow us to do this auditing at all.

EDIT: the cool thing is that because for CKE5 Plugins we have the necessary metadata in PHP (unlike in CKE4!), we can actually write a drush command that automatically analyzes this! And we can provide a Status Report entry too!

Basically, the code necessary to do this now exists, except for the two last steps (for attributes & attribute values), plus one step I had not yet thought about yet back then: generating a CKEditor 5 toolbar configuration equivalent to the pre-existing CKEditor 4 toolbar configuration. (And the cool thing about that is that we'll also be able to use it in the UI! 😄)

Too silent too long here. Sorry. Expect progress Monday & Tuesday.

My last commit broke the drush output:

  filtered_html     ckeditor      ❌             CKEditor 5 needs at least the  and                                                      
                                                                                                                                        
                                                 tags to be allowed to be able to function. They are not allowed by the "Limit allowed  
                                                HTML tags and correct faulty HTML" (filter_html) filter.         

obviously a HTML escaping problem.

Fixed in 0308b62.

7. Repeat the previous step, but now for attribute values.

This can only happen once #3215466: Attribute values not accounted for in CKEditor5PluginManager::getProvidedElements is done.

Repeat the previous step, but now for attributes.

And this is technically already supported, although it is currently not very helpful: it does not yet tell you which specific attributes are a problem. For example:

This is nowhere near specific enough yet. But improving that is out of scope here (otherwise the scope becomes wayy too big).

So … moving on to writing the necessary test coverage. Back when I started working on this, I was hoping others would write that but obviously that is not happening 🤓😅

I do not think that making the actual migration work already makes sense here. For now, the drush command proves that we have the necessary foundations. We still need at a minimum:

• knowing how to map CKEditor 4 plugin configuration to CKEditor 5 plugin configuration — must be hardcoded
• knowing how to map CKEditor 4 toolbar configuration (buttons) to CKEditor 5 toolbar configuration (buttons) — must be hardcoded
• figuring out which CKEditor 5 plugins need to be enabled to match the same set of HTML — #3215008: Ensure the "elements" metadata for each CKE5 plugin is accurate will help with that

Once the test coverage is written here, I'll create follow-up issues for those key aspects (and possibly more).

Created separate issue + MR for creating validation constraint infrastructure including tests, which this should be rebased on top of: #3216013: Move validation logic out of form into validation constraints ⇒ use config validation for form, audit command, upgrade path, et cetera.

#3216013: Move validation logic out of form into validation constraints ⇒ use config validation for form, audit command, upgrade path, et cetera landed! I can continue here. Hopefully I can push this forward again tomorrow. If not tomorrow, then by Tuesday at the latest.

The Gitlab UX is really bad/confusing. And deleting stale branches is disallowed. Ah well. 😬

Closed https://git.drupalcode.org/project/ckeditor5/-/merge_requests/17 and opened https://git.drupalcode.org/project/ckeditor5/-/merge_requests/54.

It's rebased on a much more recent version. But more importantly, it takes advantage of the infrastructure that #3216013: Move validation logic out of form into validation constraints ⇒ use config validation for form, audit command, upgrade path, et cetera introduced and takes it further.

Whereas MR17 used to do everything in a way where it was adding more special snowflake methods to \Drupal\ckeditor5\Plugin\Editor\CKEditor5 and \Drupal\ckeditor5\Plugin\CKEditor5PluginManager, we now do the exact opposite: we make them simpler.

I kept the entire MR 17 commit history. The first new commit is https://git.drupalcode.org/project/ckeditor5/-/merge_requests/54/diffs?c..., where I lay the foundations.

Key concepts:

1. We use validation constraints.
2. But validation constraints cannot cover multiple entities at the same time.
3. To solve this, we construct a special config schema for validating a text format+editor pair 🤓 — see ckeditor5.pair.schema.yml
4. \Drupal\ckeditor5\Plugin\Editor\CKEditor5::validatePair() and \Drupal\ckeditor5\Plugin\Validation\Constraint\FundamentalCompatibilityConstraintValidator are the beating heart

Happy to walk anybody through this — Ben perhaps?

There's still lots of clean-up potential, but the architecture is solid, so I'm posting it now.

Total diffstat:

 12 files changed, 882 insertions(+), 93 deletions(-)

… of which 324 insertions(+), 2 deletions(-) in tests, 45 additions for the config schema, 107 additions for the ckeditor5:audit drush command, and a lot of documentation.

After this lands, we can be 100% confident that if CKEditor5::validatePair() returns no validation constraints, that there are no problems.

Once this lands, #3216015: Generate CKEditor 5 configuration based on pre-existing text format configuration will make sense to work on, and we'll be able to use that both in the UI and in the drush command.

I've posted initial code at #3216015: Generate CKEditor 5 configuration based on pre-existing text format configuration — cool to see all this come together 😊

Rebased on top of #3201641: Improve the HTML filter configuration UX. That was hard 😬 New branch for that, because a complete rebase is A) impossible, B) nonsense — since the code it was changing has been completely changed by #3201641. See https://git.drupalcode.org/project/ckeditor5/-/merge_requests/58

A mostly straight rebase caused the instantaneous warning message about fundamental incompatibilities to not show up 😞 See the screenshots in #10 and #11 and not pictured there, but equally important, this one:

Simply doing adding an early return to ckeditor5_form_filter_format_form_alter() is enough to make it work again. This is because of the AJAX magic that was added in #3201641: Improve the HTML filter configuration UX. I had to explicitly render messages because of this.

(I also managed to add a condition to not make it override the Allowed HTML tags form item but even then it will refuse to render the message.)

On the bright side: the whole crazy work-around in #7 is no longer necessary now :)

Found the root cause, but not happy with the solution I pushed in 413bf6c2.

This is definitely related to the very complex "dynamically enable AJAX-updating everything in the form in an alter, depending on the value in a dropdown — oh and also change #limit_validation_errors" 😬.

This is also why we still have #3218985: CKEditor 5 admin UI's AJAX logic and form alterations are too complex and brittle and #3218900: Ensure only CKE5 gets the AJAXy live syncing of settings.

I think it'll really pay off to try to sort this out once and for all, to make this easier to reason about. Insofar that is possible, that is.

The problem at the moment occurs when switching to CKEditor 5 from a new or existing text format when the allowed tags filter is on (likely additional scenarios, but definitely this one). When switching to CKEditor 5, the settings of the prior format do not pass validation (which is essentially a good thing!) but the validation pushes the form into a state that makes it impossible to implement the changes needed to fix the validation errors. This is partially due to the ckeditor 5 admin library not loading (i think that one is relatively addressable).

Another issue that I'm not yet sure how to address is the "allowed tags" field is updated the field itself on non-CK5 formats, while on CK5 it is updated dynamically based on plugins and toolbar buttons. The user could possibly manually update "allowed tags" to pass CK5 validation, but that would require changing it to what CK5 requires when it is first initialized - not the full set of tags a format might require. The mastermind behind this - @Wim Leers is away for a bit, but I have some ideas I'll try out and lets see how it goes...

After spending more time with this I think there are two primary issues that need to be addressed:

• The validation in the form UI is currently taking place in \Drupal\ckeditor5\Plugin\Editor\CKEditor5::buildConfigurationForm, which only fires if the form is well into the process of switching to CKEditor 5. $form_state->get('editor' is already CKEditor 5 for example, and the form is already in the process of being rebuilt is for CKEDitor 5. Aside from a few potential exceptions (discussed in the next point), the validator needs to happen earlier to prevent any CKEditor 5 switches from happening.
• The violation error that may need to be treated differently is when checking for "allowed tags", since updating allowed tags is performed differently in CKEditor 5 vs other editors. I think the ideal approach may scope-bloat this issue, so I'm going to aim for a solution that will get things working, but may not be ideal until some followups are completed.

I added the 3201637-ben-ui-atop-wim-backend branch. As the branch name suggests, it builds off of what's been provided as of #26, but as a separate branch so it's easier to cherry pick the parts that work as this nears completion.

It's now possible to switch to CKEditor 5 in the UI once the form is in a state that is compatible with it. However, the text format will not save. There are no visible or logged errors, and my attempts to find the cause in XDebug have not yet surfaced anything.

Steps to reproduce what is happening:

1. use the MR branch
2. Go to admin/config/content/formats/add
3. Make the settings CKEditor 5 compatible by:
   • Enabling the "Limit allowed HTML tags and correct faulty HTML" filter.
   • Make this the value of "Allowed HTML tags" in the filter config: <p> <br> <h1> <h2> <h3> <h4> <h5> <h6> <strong> <i>
4. Change the value of "Text Editor" to CKEditor 5. The UI should update and the "Allowed HTML tags" field now read only
5. Click "Save configuration" to save, but it will not save.

Validation on a new editor , works great! https://youtu.be/_1shIf_M808

Validation when switching to CK5 on an existing editor has an issue, as seen in this video: https://youtu.be/d9cJgIJh9XA

The validator inside CKEditor5::buildConfigurationForm is what identifies the error. It seems to be checking against an editor that isn't representative of the form state, despite debugging confirming that $form_state has filter_html enabled. I did some debugging of AdminUi::getSubmittedEditorAndFilterFormat (which is what the validator checks against)and it does seem to be returning a $submitted_editor without the current filter values. These are the final lines:

 $reflector = new \ReflectionObject($submitted_editor);
    $property = $reflector->getProperty('filterFormat');
    $property->setAccessible(TRUE);
    $property->setValue($submitted_editor, $submitted_format);

    return $submitted_editor;

That $submitted_format seems to have the accurate filter config, but the $submitted_editor; doesn't. I'm not 100% sure this is the origin of the problem, but it's the area I'll investigate further when I resume working on this.

#30

Validation on a new editor , works great! https://youtu.be/_1shIf_M808

1. HOLY SHIT YOUR VIDEO EVEN HAS MUSIC!!!!!! 😄
2. Wow, that is an epic step forward!
3. But wouldn't it be simpler to reset the value to None automatically if CKEditor 5 is incompatible?

Validation when switching to CK5 on an existing editor has an issue, as seen in this video: https://youtu.be/d9cJgIJh9XA

1. HOLY SHIT AGAIN MUSIC!!!!!! 😄
2. But wouldn't it be simpler to reset the value to the previous text editor (if any) if CKEditor 5 is incompatible?
3. I cannot reproduce the behavior in the screencast: if I try to switch Full HTML from CKEditor 4 to 5, and then enable filter_html (like you do in the screencast around 0:12) I do not get the Apply changes to allowed tags button on the filter settings. EDIT: aha! This only works in Seven right now, in Claro it fails. I found the root cause: the AJAX request is still sending the old value for filters[filter_html][settings][allowed_html] … but then again it appears the Apply changes to allowed tags button does not actually do anything? I'm on commit 79ac82bb30c2b42bc1b4b8358347265630b4febf, which is the latest pushed commit.
4. D'oh, I failed to notice that one is supposed to copy/paste the allowed tags and then click the button 🙈😅😂 — I got spoiled by the luxury of #3201641: Improve the HTML filter configuration UX! When I do that, I can reproduce the problem. ckeditor5_validate_switching_from_different_editor() now indeed no longer has any validation errors — this is what you expect. But then when \Drupal\ckeditor5\Plugin\Editor\CKEditor5::buildConfigurationForm() finally runs (now that the fundamental compatibility validation is passing). Turns out the constructed text format is again the original text format, because the submitted filter values are ignored … because $switching_to_ck5 is incorrectly FALSE. Trying to work on a work-around…

P.S.: the tags that the "Apply changes to allowed tags" button is adding/setting is not the minimum, but it is the set of tags that will be generated for \Drupal\ckeditor5\Plugin\Editor\CKEditor5::getDefaultSettings(). #3216015: Generate CKEditor 5 configuration based on pre-existing text format configuration will improve that 😊 Therefore, I'm fine keeping this as-is. EDIT: oh and I even see this is just hard-coded for now — perfect!

But wouldn't it be simpler to reset the value to the previous text editor (if any) if CKEditor 5 is incompatible?

. I attempted to do this without success. Unfortunately, I already don't recall the specifics of how I was attempting to do it, and what the problems I ran into were (long week!). I think this would provide clearer UX if it's possible.

So this is once again caused by the super tricky $form_state->getUserInput() stuff 😬 (See #7, #9, #10.)

I haven't been able to figure out a work-around yet 😔

@bnjmnm, wanna pair tomorrow? 🤓

@Wim Leers and I reviewed each others changes and all threads that came out of that co-review are resolved. I'm going to hold off on committing until I've at least had a moment away from this. I'd be comfortable committing Wim's part of the work, but some of the changes I just made are substantial enough (the mutation observer and changes to getSubmittedEditorAndFilterFormat in particular) that I don't want to casually commit immediately after writing it.

#34: I checked it in detail today. I can't find any problems with any of the changes you made. I went ahead and merged it 🤓

(I did some minor refactoring of the "root violation" checking: rather than duplicating logic, I added a new optional parameter to \Drupal\ckeditor5\Plugin\Editor\CKEditor5::validatePair(). This reduces brittleness. In doing so I first caused a subtle regression and then spotted it — and in stumbling there I've accidentally proven that the tests are really solid 😊)

diffstat: 1350 additions, 109 removals. 354 are in the validation constraint, 24 for the schema, 77 are providing infrastructure in the CKEditor5 plugin to use that validation constraint and 413 are in the test coverage for that validation constraint. That adds up to 868 of the 1350 additions. Another 107 for the drush command. Together that adds up to 72% of the new LoC. In doing so, @bnjmnm found half a dozen or so bugs in the pre-existing UI+test coverage that the validation constraints helped uncover. Overall, we agreed on Friday that this issue/merge request was more than worth it: it already helped stabilize the code base! (I admit I was pleasantly surprised that so many problems + improvements in the pre-existing test coverage were found! 😄)