Remove Quick Edit support from editor.module

Good find, thanks for opening this. More tentacles! 😉

Changing parent to the main tracking issue. Might get some more exposure as well.

Drupal.quickedit.editors.editor._getUntransformedText() uses this:

      /**
       * Loads untransformed text for this field.
       *
       * More accurately: it re-filters formatted text to exclude transformation
       * filters used by the text format.
       *
       * @param {function} callback
       *   A callback function that will receive the untransformed text.
       *
       * @see \Drupal\editor\Ajax\GetUntransformedTextCommand
       */
       _getUntransformedText(callback) {

The solution is simple IMHO:

1. move \Drupal\editor\Plugin\InPlaceEditor\Editor (and its route + its JS) out of core/modules/editor and into core/modules/quickedit
2. make sure both that plugin and the associated route are only available if the editor module is installed

@Wim Leers: Thanks! Very helpful. Here's an untested first attempt. Let's see how badly this blows up the bot. 😅

Meanwhile, how does one deprecate a route?

sh ./core/scripts/dev/commit-code-check.sh is hanging on my local dev box, sorry about that. This should now at least get past the code check phase...

Meanwhile, how does one deprecate a route?

We can't yet, see #3159210: Support route aliasing (Symfony 5.4) and allow deprecating the route name. But in this case we don't need to - we don't consider route names part of the API (even though we might not randomly move ones that are referenced a lot).

1. +++ b/core/modules/quickedit/js/editors/formattedTextEditor.es6.js
   @@ -219,7 +219,7 @@
   -              'editor/!entity_type/!id/!field_name/!langcode/!view_mode',
   +              'quickedit/!entity_type/!id/!field_name/!langcode/!view_mode',
   
   +++ b/core/modules/quickedit/quickedit.routing.yml
   @@ -35,3 +35,16 @@ quickedit.entity_save:
   +  path: '/quickedit/{entity_type}/{entity}/{field_name}/{langcode}/{view_mode_id}'
   

   🤓 Note that you could even keep the URL the same, for less disruption — specifically when the JS is still cached on the client, it'll continue to make requests against the old URL.

   So keeping the old URL would reduce disruption.

2. +++ b/core/modules/quickedit/src/QuickEditController.php
   @@ -356,4 +357,31 @@ public function entitySave(EntityInterface $entity) {
   +    $response->addCommand(new GetUntransformedTextCommand($editable_text));
   

   🤓 This is the only use in Drupal core of GetUntransformedTextCommand. If we move the Quick Edit module to contrib, I think we should A) deprecate the PHP class in core, B) switch the Quick Edit module to use its own copy of this command.

This is not going to be a big success, but for now I don't have any more time left.
Let's see if we can bring the number of failures down.

Haven't done _anything with #11 and #12

Mo' sleep, mo' green?

Since the patch in #14 removes skipping of test in core/modules/quickedit/tests/src/FunctionalJavascript/QuickEditIntegrationTest.php it might fail because of the NewChromeDriverMageddon.

I've got a pretty decent, if I say so myself, fix up for review over in #3268010: Restore/Robustify \Drupal\Tests\quickedit\FunctionalJavascript\QuickEditIntegrationTest::testCustomBlock(), so folks might want to head over there for some reviewing.

So #14 was indeed hit by Chromageddon.

Still putting it on NR for eyes on it and #3268010: Restore/Robustify \Drupal\Tests\quickedit\FunctionalJavascript\QuickEditIntegrationTest::testCustomBlock().

We have to also look at #12.

1. I don't have enough knowledge to have a solid opinion on _not_ removing the old route, but it feels a bit dirty to me.
At some point it _will_ become disruptive, when D10 lands without quickedit. I rather have stuff "breaking" as soon as possible, to create awareness that quickedit is leaving Core and because brains here are still filled with the knowledge about changes we made.

2. I like it and it makes sense to deprecate the current class and create a new one in (contrib) quickedit.
The class doesn't seem to be "heavily" used in Contrib anyway: http://grep.xnddx.ru/search?text=GetUntransformedTextCommand&filename=
My question is: Should it be done here or in (Yet Another) follow-up issue.

My question is: Should it be done here or in (Yet Another) follow-up issue.

I think whatever's easiest to manage is fine - the only thing that complicates it is 10.x removal and 9.4.x deprecation, otherwise it's straightforward.

Thanks @mstrelan and @catch, let's do #12.2 here and make 2 patches, one for 9.4.x-dev and 10.0.x-dev.

Added draft CR.

Unsure if Release note is needed?

Besides #3268010: Restore/Robustify \Drupal\Tests\quickedit\FunctionalJavascript\QuickEditIntegrationTest::testCustomBlock() this seems ready for review again.

Used 3267258-d94-25.patch as base for the MR for 9.4.x-dev.

Used 3267258-d10-23.patch as base for the new 10.0.x-dev MR.

That looks like a real fail of that test, not a random.

This looks very close! 👍

Addressed @Wim Leers comments.

We've identified (and fixed) the root cause of random failures in \Drupal\Tests\quickedit\FunctionalJavascript\QuickEditIntegrationTest::testCustomBlock(), but haven't looked at ::testArticleNode() yet.

Let's see if there's a fix for that one, we have some QuickEdit test failure trickery in our bag by now, maybe one of those can help.

This needs #3268244: [random test failure] Un-skip and fix QuickEditIntegrationTest::testArticleNode() to land first (it currently incorporates that change directly into the MR — but #3268244 should just land first, that'll simplify this MR!).

#3268244: [random test failure] Un-skip and fix QuickEditIntegrationTest::testArticleNode() is in.

#3268244: [random test failure] Un-skip and fix QuickEditIntegrationTest::testArticleNode() landed, let's do this!

EDIT: dammit, @catch beat me to it 🤣

Only a single question left! 😄

Last question resolved, 2x green TestBot.

Per #3268244-49: [random test failure] Un-skip and fix QuickEditIntegrationTest::testArticleNode(), that fix was never pushed to 9.5.x. We need that commit pushed, then to rebase this MR, so the MR isn't duplicating the effort from 3268244.

Nope, I misread. It should be there. I think this MR just needs a real rebase.

Nothing to "proper rebase" here IMHO. The threads you opened were never part of #3268244: [random test failure] Un-skip and fix QuickEditIntegrationTest::testArticleNode().
That one is about ::testArticleNode(), the changes you marked are for ::testCustomBlock().

Yeah, sorry. I've been deep in so many other swamps the last few weeks, I'm struggling to get my head back into the QE quagmire. 😉 I think this is fine. I don't really have anything else to complain about. If it works, ship it!

I think it's OK to unskip the test here - there's lots of other test changes, we won't have the test in 10.0.x once this is in. Absolute worst case we'd have to start skipping it again in 9.5.x.

Committed/pushed the respective MR diffs to 10.0.x and 9.5.x, thanks!

Published CR.

Yep, agreed that fixing/unskipping the test as part of this issue was fine.

Was hoping to RTBC this morning, but instead I got the notification that it was in already: better still! 👏

Thanks for all the epic work on this and related issues!

Was hoping to RTBC this morning, but instead I got the notification that it was in already: better still!

Well, the module is named Quick Edit...

Me and my buddy Sisyphus are here all week. Try the veal!

🤣

The attached plain diff from MR2020 applies cleanly to 9.4.x-dev.

Follow-up to change the deprecation errors mentioning 9.5.0 to 9.4.2 here: #3292525: [PP-1] Update deprecation errors made in [#3267258] from 9.5.0 to 9.4.2

We also need to update the CR when this gets committed.

Not backporting this any more.