Removing all buttons from the toolbar should not be considered an error

Interesting … that reminds me of #3215897: Toolbar items validation errors not able to target the toolbar.

I think the problem here is that the fix that you made is too broad.

This is the full code:

    $toolbar_items = Json::decode($json);

    // This basic validation must live in the form logic because it can only
    // occur in a form context.
    if (!$toolbar_items) {
      $form_state->setErrorByName('toolbar][items', $this->t('Invalid toolbar value.'));
      return;
    }

You are removing the ::setErrorByName() call. That was intended to get invoked when there is invalid JSON. But you're right that PHP also enters that branch when $toolbar_items === [].

I think we should special case the empty array case. Or … better yet … change the if (!$toolbar_items) to different code that checks whether there were JSON decoding errors.

We should still add a test to verify that A) it's possible to remove all buttons using the admin UI, B) it does not result in errors.

A is already the case, but by testing it using the UI we ensure we don't regress on this front.

Although … it looks like whichever toolbar item was the last one will persist. So this does expose/introduce a new edge case. I think we should solve that here at the same time. This won't be able to land without test coverage anyway.

I just tested it (by forcing it manually), and yes, we can use CKEditor 5 without any toolbar items. But you'll only be able to create <p> and <br>. And while weird … I do think that's valid.

So, to expand what I said: We should still add a test to verify that A) it's possible to remove all buttons using the admin UI, B) it does not result in errors, C) we should be able to save it, D) when re-editing, we should assert that there are still no buttons present.

Thoughts from people besides @marcvangend and I? 🤞

I think you're right actually. It's possible that you want to only allow <p> and <br>. Several tests written since you wrote that do precisely that.

Closed #3372452: When a textformat has no text-editor configured ckeditor crashes as a duplicate of this. Crediting @tim@lammar.be for that!

Clearly, a tiny percentage of people want to configure CKEditor 5 in a way that there's no toolbar at all. @tim@lammar.be, can you please explain what your use case is for:

When you have a text format where you don't want a toolbar and you configure the text-editor field to be none, […]

?

Thanks! 🙏

@Wim Leers,
Sure :-)
Our use case is we have a text format field and 2 form modes for content editing of that specific node type.
In one form mode, the field may be filled with content generated by editor buttons. In the other form mode, the same field may only be filled with text (much like a textarea field). This is mostly because the one form mode is used for users with more permissions, like using advanced editor functionality. The other form mode is used for users with less permissions and actually can not use the editor. So the field needs to be text format, but for some users the editor may not be used. So we had a text format that had no editor active (what was possible in CKEditor 4, but now with the upgrade to Drupal 10, it gave issues)

Thanks for the fast clarification!

So the field needs to be text format, but for some users the editor may not be used. So we had a text format that had no editor active (what was possible in CKEditor 4, but now with the upgrade to Drupal 10, it gave issues)

Even with no buttons, you'll still be able to create <p> and <br> in CKEditor 4/5 — without those tags, it cannot function.

What you're describing indeed sounds like a use case for having NO text editor for that one text format. What problem did you run into when you tried doing that? 🤔

I don't remember exactly what errors occurred, but it had to do with the fact that in the code there are array methods used on toolbar items. These are in this case non existing and so causing php errors (something like expecting array, got NULL).
The patch I provided in the previous ticket fixed the ones I encountered, but it's possible other places need to be fixed as well (ref: https://www.drupal.org/project/drupal/issues/3372452#comment-15136932)

I don't see how any toolbar item method would be getting invoked when no text editor is configured? 🤔

Exactly, but apparently it did 🤷‍♂️

Plain text filter might want this

I added MR for the 11.x, change the condition to check whether the JSON is valid or not. Changes need to be reviewed.