Remove Quick Edit from core

🔥

Here's a list of QuickEdit integration tests in the namespaces of other modules:

core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderQuickEditTest.php
core/modules/layout_builder/tests/src/Functional/LayoutBuilderQuickEditTest.php
core/modules/layout_builder/tests/src/Functional/LayoutBuilderTest.php
core/modules/inline_form_errors/tests/src/FunctionalJavascript/FormErrorHandlerQuickEditTest.php
core/modules/field/tests/modules/field_plugins_test/src/Plugin/Field/FieldFormatter/TestTextTrimmedFormatter.php
core/modules/image/tests/modules/image_module_test/src/Plugin/Field/FieldFormatter/DummyImageFormatter.php
core/modules/image/tests/src/FunctionalJavascript/QuickEditImageEditorTestTrait.php
core/modules/image/tests/src/FunctionalJavascript/QuickEditImageTest.php
core/modules/image/tests/src/Functional/QuickEditImageControllerTest.php
core/modules/system/tests/src/Functional/UpdateSystem/UpdatePathTestBaseFilledTest.php
core/modules/settings_tray/tests/src/FunctionalJavascript/QuickEditIntegrationTest.php
core/modules/settings_tray/tests/src/FunctionalJavascript/SettingsTrayBlockFormTest.php
core/modules/settings_tray/tests/src/FunctionalJavascript/SettingsTrayTestBase.php
core/modules/node/tests/src/Functional/NodeDisplayConfigurableTest.php
core/modules/editor/tests/src/Functional/QuickEditIntegrationLoadingTest.php
core/modules/editor/tests/src/Kernel/QuickEditIntegrationTest.php
core/modules/media/tests/src/Kernel/MediaEmbedFilterDisabledIntegrationsTest.php

Some of them can be removed wholesale; others might needs specific methods removed.

Here are the remaining references in tests after blindly removing those that are explicitly labeled as QuickEdit integration tests:

core/modules/layout_builder/tests/src/Functional/LayoutBuilderTest.php
core/modules/field/tests/modules/field_plugins_test/src/Plugin/Field/FieldFormatter/TestTextTrimmedFormatter.php
core/modules/image/tests/modules/image_module_test/src/Plugin/Field/FieldFormatter/DummyImageFormatter.php
core/modules/system/tests/src/Functional/UpdateSystem/UpdatePathTestBaseFilledTest.php
core/modules/settings_tray/tests/src/FunctionalJavascript/SettingsTrayBlockFormTest.php
core/modules/settings_tray/tests/src/FunctionalJavascript/SettingsTrayTestBase.php
core/modules/node/tests/src/Functional/NodeDisplayConfigurableTest.php
core/modules/media/tests/src/Kernel/MediaEmbedFilterDisabledIntegrationsTest.php

Non-test references remaining:

core/misc/cspell/dictionary.txt
core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/BasicStringFormatter.php
core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/StringFormatter.php
core/modules/layout_builder/layout_builder.module
core/modules/layout_builder/src/Entity/LayoutBuilderEntityViewDisplay.php
core/modules/layout_builder/src/QuickEditIntegration.php
core/modules/comment/src/Plugin/Field/FieldFormatter/CommentDefaultFormatter.php
core/modules/comment/src/Plugin/Field/FieldFormatter/CommentPermalinkFormatter.php
core/modules/ckeditor/css/ckeditor.css
core/modules/image/image.routing.yml
core/modules/image/css/editors/image.css
core/modules/image/css/editors/image.theme.css
core/modules/image/js/editors/image.es6.js
core/modules/image/js/editors/image.js
core/modules/image/js/theme.js
core/modules/image/js/theme.es6.js
core/modules/image/image.libraries.yml
core/modules/image/src/Plugin/InPlaceEditor/Image.php
core/modules/image/src/Plugin/Field/FieldFormatter/ImageFormatter.php
core/modules/image/src/Controller/QuickEditImageController.php
core/modules/settings_tray/js/settings_tray.es6.js
core/modules/settings_tray/js/settings_tray.js
core/modules/text/src/Plugin/Field/FieldFormatter/TextSummaryOrTrimmedFormatter.php
core/modules/text/src/Plugin/Field/FieldFormatter/TextTrimmedFormatter.php
core/modules/responsive_image/src/Plugin/Field/FieldFormatter/ResponsiveImageFormatter.php
core/modules/editor/editor.routing.yml
core/modules/editor/editor.module
core/modules/editor/js/editor.formattedTextEditor.js
core/modules/editor/js/editor.formattedTextEditor.es6.js
core/modules/editor/editor.libraries.yml
core/modules/editor/src/Plugin/InPlaceEditor/Editor.php
core/modules/media/src/Plugin/Filter/MediaEmbed.php
core/themes/stable/css/quickedit/quickedit.module.css
core/themes/stable/css/quickedit/quickedit.theme.css
core/themes/stable/css/quickedit/quickedit.icons.theme.css
core/themes/stable/css/ckeditor/ckeditor.css
core/themes/stable/css/image/editors/image.css
core/themes/stable/css/image/editors/image.theme.css
core/themes/stable/stable.info.yml
core/themes/stable9/css/quickedit/quickedit.module.css
core/themes/stable9/css/quickedit/quickedit.theme.css
core/themes/stable9/css/quickedit/quickedit.icons.theme.css
core/themes/stable9/css/ckeditor/ckeditor.css
core/themes/stable9/css/image/editors/image.css
core/themes/stable9/css/image/editors/image.theme.css
core/themes/stable9/stable9.info.yml
core/themes/claro/css/components/quickedit.css
core/themes/claro/css/components/quickedit.pcss.css
core/themes/claro/claro.info.yml
core/themes/seven/css/components/quickedit.css
core/themes/seven/seven.info.yml

One comment from @larowlan on the other issue:

One thing we'll need to consider here, is that formatters define quickedit metadata in their annotation.

We'll need to move that to quickedit module in an alter hook - for formatters we know about in core.

But for other formatters from contrib that integrate with quickedit, they may need additional steps.

It looks like we have a similar thing for themes with the quickedit_stylesheets key.

I am strongly motivated. 🔥

Great job! Looks like #9 could left in core namespace to allow contrib to extend this annotations (panels for example or ctools)

Edit could use @timplunkett review

Updating the remaining tasks.

Maybe better use event to find "annotation extensions" (subscribers) instead of hooks

@dww I'm affecting this test few days ago in #3090187: Mechanism to disable preprocessing of base fields in comment entity type so they can be configured via the field UI

@dww I think we only need a stub quickedit.info.yml marked "obsolete" for some of the tests to pass, so I added that for now.

Re: #20, leaving an obsolete stub in that uninstalls itself is the best practice anyway from moving anything from core; I just wanted to see what would break if the entire module were actually not present to surface other things that will need a fix. (We will need to make updated upgrade path tests again in 10.0.x development for multiple modules and deprecations, not just this one.)

For things like #15, those tests should not be coupled to QuickEdit in the first place. We will need to rewrite them in 9.3.x (and spin off separate issues for them eventually).

@dww, Yep, good idea I think.

Setting a better issue status.

Posted #3228634: Move tests for integrations between QuickEdit and other modules into QuickEdit so that it can more easily be moved into contrib.

Posted #3230928: Remove unused LayoutBuilderTest::testRemovingAllSections()' Quick Edit dependency for that remaining LB test failure.

Oops, my force-push also ate the commits to remove QuickEdit from the test formatter annotation and test image formatter. Those two things should probably get their own issue as well (one issue for both seems fine).

This is an issue to do a trial run of removing it, and see what breaks.

Looking at https://www.drupal.org/pift-ci-job/2165728, me thinks the introduction of a required lifecycle_link for deprecated and obsolete lifecycle states in #3225812: Add lifecycle_link key to info.yml files breaks (almost) every test know to man- and TestBot-kind. ;)

Since this is an [Investigation] issue, I think it's OK to add a not yet existing URL (https://www.drupal.org/about/core/policies/core-change-policies/deprecated-and-obsolete-modules-and-themes#s-quickedit)?

Oops, my force-push also ate the commits to remove QuickEdit from the test formatter annotation and test image formatter. Those two things should probably get their own issue as well (one issue for both seems fine).

Opened #3231071: Remove QuickEdit from the test formatter annotation and test image formatter. for that.

Besides:

• #3227161: Stop using Quick Edit data attributes as selectors in tests that are not testing Quick Edit being RTBC-ed and committed
• #3230928: Remove unused LayoutBuilderTest::testRemovingAllSections()' Quick Edit dependency coming back green, being RTBC-ed and committed
• #3231071: Remove QuickEdit from the test formatter annotation and test image formatter. coming back green, being RTBC-ed and committed
• and creating a valid lifecycle_link for core/modules/quickedit/quickedit.info.yml

I think we're done here for now?

With #3227161: Stop using Quick Edit data attributes as selectors in tests that are not testing Quick Edit and #3238626: [Follow-up] Move tests for integrations between QuickEdit and other modules into QuickEdit done, time to rebase this again… and hopefully we'll see 0 test failures!

With the addition of CKEditor5 in #3231364: Add CKEditor 5 module to Drupal core a new QuickEdit integrationtest was added: \Drupal\Tests\ckeditor5\FunctionalJavascript\QuickEditIntegrationTest.

This has to be moved into QuickEdit's test namespace to facilitate the core removal along the lines of #3228634: Move tests for integrations between QuickEdit and other modules into QuickEdit so that it can more easily be moved into contrib where the other QuickEdit related test classes were moved.

Opened #3252214: Move tests for integrations between QuickEdit and CKEditor5 into QuickEdit so that it can more easily be moved into contrib for that.

Postponing on #3252214: Move tests for integrations between QuickEdit and CKEditor5 into QuickEdit so that it can more easily be moved into contrib

Unpostponing since #3252214: Move tests for integrations between QuickEdit and CKEditor5 into QuickEdit so that it can more easily be moved into contrib was committed.

p.s. Should we open an unpublished CR about this now so the lifecycle_link has a real nid to point to?

Thus spoketh @dww in #39

Looking at the already obsolete Core modules simpletest and entity_reference, the lifecycle_link points to an anchor on https://www.drupal.org/about/core/policies/core-change-policies/deprecated-and-obsolete-modules-and-themes.
I _think_ by the time we launch Drupal 10, an entry should be made there.

Until then I think linking to a CR is indeed the way to go. I've started an unpublished one here: https://www.drupal.org/node/3252839

Maybe a follow-up is needed to change the lifecycle_link from pointing to the CR to https://www.drupal.org/about/core/policies/core-change-policies/deprecat... should be created?

Also a follow-up to move QuickEdit into a Contrib Module should be created, me thinks.

For both follow-ups I'm unsure if they should be children of this issue or of the parent issue #3228986: [Meta] Tasks to deprecate Quick Edit.
Any advice on that would be most welcome.

[Removed double post]

Since we missed the 9.3-boat, here's a patch against 9.4.x

Reroll of patch #47

I think we can use this issue for the actual removal of quickedit from core now.

However seems like we probably need a 9.4.x to mark it deprecated still? Or is there one around somewhere.

However seems like we probably need a 9.4.x to mark it deprecated still? Or is there one around somewhere.

Sense = made.

So here's a patch for the removal and making the module obsolete in 10.0.x

And a deprecation patch for 9.4.x.

Both CRs could do with some people with Big Brains looking at it.

Also we need to have the Contributed Module quickedit in place so we can link to it in both CRs, I presume?

Yes I think the order is:

1. Contrib module quickedit
2. Deprecation in 9
3. Removal in 10

Steps 2 and 3 can be combined if both bits are ready.

So, for the ignorant amongst us *raises hand* how does one create a contrib module?

- I imagine naming it "quickedit" would clash with the 9.4.x release which still has a "quickedit" module in core (although deprecated by then)?
- If one creates a Contrib Module, I suppose you're immediately the maintainer, which in this case should be @gaurav.kapoor (See #3228986-4: [Meta] Tasks to deprecate Quick Edit?

Patch #52 reveals we need at least to solve one more issue: Having a deprecated module with non-deprecated dependency in core leads to a test failure in InstallUninstallTest.

Created issue #3259850: Having a deprecated module with non deprecated dependency leads to test failure in InstallUninstallTest to deal with this.

Postponing on #3259850: Having a deprecated module with non deprecated dependency leads to test failure in InstallUninstallTest .

Pu tth espac ei nth ewron gplac e :/

@Spokje

I imagine naming it "quickedit" would clash with the 9.4.x release which still has a "quickedit" module in core (although deprecated by then)?

So this is actually fine given the module will be identical, with a slight wrinkle from #3188544: [policy discussion] Address Composer namespacing issues when extensions move between core and contrib but that is resolvable.

If you have a Drupal 9.4 site, you'll get deprecation warnings, and if you go and download the contrib project, then it'll override your core module and they'll go away (because modules in modules/* or sites/* override those in core/*

If you have a Drupal 10 site, your only option is the contrib module.

I think that we should add a test that when the module gets installed that the module is deprecated.

Besides the point which @daffie makes above, there's Yet Another Issue with InstallUninstallTest: #3259888: InstallUninstallTest fails on enabling both deprecated and experimental modules at the same time

I think that we should add a test that when the module gets installed that the module is deprecated.

We have generic tests for the lifecycle key, I don't think we need one for each individual module that we deprecate.

I think that we should add a test that when the module gets installed that the module is deprecated.

We have generic tests for the lifecycle key, I don't think we need one for each individual module that we deprecate.

Looks like an attempt at this is being made at #3257127: Trigger a deprecation message when a deprecated module or theme is enabled.

Removing "needs tests" tag since:

1) Test is being made in #3257127: Trigger a deprecation message when a deprecated module or theme is enabled
2) Test seemed to be unneeded according to @catch

Postponing on #3259888: InstallUninstallTest fails on enabling both deprecated and experimental modules at the same time

#3259888: InstallUninstallTest fails on enabling both deprecated and experimental modules at the same time was committed, so unpostponing.

Starting a retest on the deprecation D9.4 patch

#3259888: InstallUninstallTest fails on enabling both deprecated and experimental modules at the same time has landed.

Looks good to me.

So now we have a TestBot-green deprecating patch for 9.4 and a TestBot-green removal/obsolete patch for D10.
According to catch in #53:

Yes I think the order is:

1. Contrib module quickedit
2. Deprecation in 9
3. Removal in 10

Steps 2 and 3 can be combined if both bits are ready.

we're still missing step 1.: A contib quickedit module.

Which leads us (or at least me) back to point #2 in #55:

- If one creates a Contrib Module, I suppose you're immediately the maintainer, which in this case should be @gaurav.kapoor (See #3228986-4: [plan] Move QuickEdit into contrib and remove it from core?

dww for president!

Besides the above, I think that realistically, no other new maintainer will stand up any time soon. I also don't expect a lot of issues being created on the new Contrib incarnation of QuickEdit.
Searching for "quickedit" in the issue queue doesn't reveal a lot of activity on the actual module, so it might fit in with @dww's "minimally maintained" approach.

To keep this issue moving forward I would love to see @dww's "shot-in-own-foot" #73 come into fruition and a Contrib version of quickedit to emerge.

Thanks @dww!

So I _think_ we're all good for deprecation now.

However there's one thing missing IMHO: We need to transfer all QuickEdit issue in the Core queue to the QuickEdit Contrib queue.
Is that something that deserves it's own issue? (I have no idea how this can be done and who to ping on this).

I think we could just transfer issues from this list manually. There's just one page of issues so it seems easier to move them manually compared to trying to figure out who can do it automatically.

Thank @lauriii, moved those in your search query and (weirdly) found (and moved) about 30 more issue on component ''quickedit" by just searching for quickedit on core.

All are now transferred to https://www.drupal.org/project/issues/quickedit?categories=All

@dww: Since I think you're only (minimally) supporting the D9 version (1.x) you might want to close all 7.x issues and update the Module page which still has some D7 information.

The 10.0.x patch should do a complete removal rather than marking obsolete. We haven't done many of these recently, so I'm not sure we have a clear precedent, but reasoning below:

The obsolete status is for modules like simpletest or entity_reference where it is either a development dependency we want to ensure is uninstalled, or the module itself has been completely gutted, and the only course of action is to uninstall before we remove it. In this case we don't need to give site admins a choice, the module is genuinely obsolete and just needs to be removed cleanly.

For non-development modules where we're offering a contrib version, we don't want to uninstall the module on sites' behalf, they need to take an active decision to install the contrib version or uninstall themselves. If someone updates to 10.0.x without doing that, then the big error that a module is missing from the filesystem is correct.

Also looks like the 10.0.x patch needs a re-roll too.

New D10 removal patch

------ --------------------------------------------------------------------- 
  Line   core/modules/editor/src/Plugin/InPlaceEditor/Editor.php              
 ------ --------------------------------------------------------------------- 
         Class Drupal\quickedit\Plugin\InPlaceEditorInterface not found.      
         💡 Learn more at https://phpstan.org/user-guide/discovering-symbols  
  18     Class Drupal\quickedit\Plugin\InPlaceEditorInterface not found.      
         💡 Learn more at https://phpstan.org/user-guide/discovering-symbols  
 ------ --------------------------------------------------------------------- 

 ------ --------------------------------------------------------------------- 
  Line   core/modules/image/src/Plugin/InPlaceEditor/Image.php                
 ------ --------------------------------------------------------------------- 
         Class Drupal\quickedit\Plugin\InPlaceEditorBase not found.           
         💡 Learn more at https://phpstan.org/user-guide/discovering-symbols  
  15     Class Drupal\quickedit\Plugin\InPlaceEditorBase not found.           
         💡 Learn more at https://phpstan.org/user-guide/discovering-symbols  
 ------ --------------------------------------------------------------------- 

If we didn't have the quickedit module in contrib, we'd need a core patch to move them to quickedit prior to removal. Now that it's there, I opened #3261096: Include quickedit plugins from editor and image modules, but that'll need to block this issue so that we're not knowingly introducing a regression.

(Also, well done phpstan / I coulda gotten away with it if it weren't for that meddling phpstan).

#3261096: Include quickedit plugins from editor and image modules is now PP-2, unsure if this would mean this is now PP-3?

To quote myself:

...unsure if this would mean this is now PP-3?

Not that it matters, because we have (at least) a PP+1 here.
The attached rerolled D10 removal patch will show that it fails on the fixture core/modules/system/tests/fixtures/update/drupal-9.0.0.filled.standard.php.gz having quickedit installed.

I think we need a new drupal-10.0.0.filled.standard.php.gz fixture, without quickedit.
Whilst we're at it, it probably should already have all D9.x updates sucessfully ran, except for the installation a the appropriate DB module.

That might mean we need to go ahead with #2942096: [policy, no patch] Remove old update hooks prior to each major version (Drupal 10 and later) first.

Well, as a bare minimum this issue for the removing D10.x patch needs a fixture that has no quickedit installed.

Since it's about D10 I think we indeed need to postpone it on the issue that @catch mentioned above (#2942096: [policy, no patch] Remove old update hooks prior to each major version (Drupal 10 and later)), since that fixture would be a base for the one needed here.
Maybe in fact more a child-issue that actually commits such a fixture into the 10.0.x-dev branch.

Maybe in fact more a child-issue that actually commits such a fixture into the 10.0.x-dev branch.

#3261486: Remove core updates added prior to 9.3.0 and adjust test coverage seems to be just that issue.

#3261486: Remove core updates added prior to 9.3.0 and adjust test coverage was just committed, now just postponed on #3261096: Include quickedit plugins from editor and image modules

Reroll of the 10.0.x removal patch to see where we stand now that #3261486: Remove core updates added prior to 9.3.0 and adjust test coverage is in.

Updated the D10.0 removal patch with a drupal-9.3.0.filled.standard.php.gz fixture without the quickedit module being installed.

Interdiff doesn't contain the changes in that fixture, because it's gzipped and would be non-human-readable.

Also needed to create a drupal-9.3.0.bare.standard.php.gz fixture without the quickedit module being installed.

No interdiff, because only that .gz file changed and would be non-human-readable.

#3261096: Include quickedit plugins from editor and image modules is resolved so this should be ready again.

Wait, sorry, I didn't see that #3260995: Split the core 9.4.x quickedit history into a new 1.0.x branch and cut a 1.0.0 release was a child issue of this, and it's noted in that issue summary as blocking.

https://www.drupal.org/project/quickedit/releases/1.0.0 is tagged and out. 🎉 I believe that was the only remaining blocker to this moving forward.

Thanks @dww for cutting the release and @mradcliffe for unpostponing.

In #3263618-11: Deprecate HAL module and #3263618-10: Deprecate HAL module it was decided to link to https://www.drupal.org/node/3223395 in the lifecycle_link of the deprecated extension.
I've added a new section for quickedit to that page.

Attached patch should comply with the above.

Reroll of patch in #93

Both the patch for D9.4 and D10 look good to me.
The patch for D9.4 deprecates the module and add a deprecation link.
The patch for D10 removes the module.
For me it is RTBC.

All of the 9.4 quickedit tests need to be tagged with @legacy after #3257127: Trigger a deprecation message when a deprecated module or theme is enabled.

Help topics and REST are both enabling all modules, need to filter deprecated ones out, opened #3264435: Help topics and rest don't filter out deprecated modules when testing.

This patch should take care of the 16 errors that are caused by running @group quickedit tests, the remaining 2 failures should be fixed when #3264435: Help topics and rest don't filter out deprecated modules when testing lands.

Ready to go again once the other issue has landed.

As expected (and predicted by the almighty @catch) 2 failures remain, soon to be solved when #3264435: Help topics and rest don't filter out deprecated modules when testing lands.

#3264435: Help topics and rest don't filter out deprecated modules when testing was just committed, starting D9.4 deprecation patch retest and expecting it to come back green

Reroll of D10 removal patch

Aaaaand back to Postponed, this time on #3264061: Remove deprecated functions from image module

waves at guy pushing boulder uphill whilst running after his own boulder rolling downhill

Hi Sisyphus!

That should hopefully have been fixed by #3255887: MediaThumbnailFormatter => Calling ImageFormatter::__construct() without the $file_url_generator argument is deprecated in drupal:9.3.0. I've sent the patch here for retesting now that's in. Tentatively unpostponing again.

00:03:39.301 ----------------------------------------------------------------------------------------------------
00:03:39.301 
00:03:39.301 Running PHPStan on *all* files.
00:04:52.735 
00:04:52.741  [OK] No errors       

Sees rolling stone suddenly changing direction from downwards to upwards
Well, that was unexpected...

Thanks @catch and @Big Brains behind fixing #3255887: MediaThumbnailFormatter => Calling ImageFormatter::__construct() without the $file_url_generator argument is deprecated in drupal:9.3.0.

Chases upwards rolling stone and passes guy running after boulder going downwards
Hi Sisyphus!

Module and theme removals should always be tagged for the release notes. Thanks!

Thus spoketh @xjm in [#2966859-32]

We still need to remove \Drupal\layout_builder\QuickEditIntegration and refactor it so that quickedit contrib provides the integration with layout builder.

We still need to remove \Drupal\layout_builder\QuickEditIntegration and refactor it so that quickedit contrib provides the integration with layout builder.

Ouch, so \Drupal\layout_builder\QuickEditIntegration has:

• No testcoverage
• No usage of any classes in the quickedit module
• Usage of a couple of hooks in the layout_builder.module file
• Nothing moved to the Contrib quickedit module yet

I'm quite happy for Bigger Brains to step in on this one, I actually think I've never user quickedit nor layout_builder, so even visual testing to see if before and after matches would not work.

The class is marked as @internal so I guess it doesn't need to be deprecated in 9.4.

Ouch, so \Drupal\layout_builder\QuickEditIntegration has:
No testcoverage

I'm guessing that's what is covered by \Drupal\Tests\quickedit\Functional\LayoutBuilderQuickEditTest which is now in contrib.

Usage of a couple of hooks in the layout_builder.module file

I believe the class and those hooks should be relatively easy to move to quickedit, however there is also \Drupal\layout_builder\Entity\LayoutBuilderEntityViewDisplay::getQuickEditSectionComponent which is called from ::getComponent in the same class. Quickedit will need to implement the relevant hook to provide this behaviour.

Right, whilst walking away from the boulder at the bottom of the hill to look for my old buddy Sisyphus to check if he has worked with layout_builder and/or quickedit lately, I realized that no matter what or who's going to do this, (Yet Another) new issue will probably come in handy. The patch or plain diff from the MR there could probably be applied on the Contrib quickedit.

So: Once more unto the breach, dear friends, once more...where the breach this time is located at #3264633: Remove \Drupal\layout_builder\QuickEditIntegration and refactor it so that quickedit contrib provides the integration with layout builder

And of course: Nice catch @mstrelan.

Postponing on #3264633: Remove \Drupal\layout_builder\QuickEditIntegration and refactor it so that quickedit contrib provides the integration with layout builder

\Drupal\image\Controller\QuickEditImageController is another integration in core that would need to move.

The PHPStan failure is due to Symfony 6:

 * Rename `RequestStack::getMasterRequest()` to `getMainRequest()`

https://github.com/symfony/symfony/search?q=getMasterRequest

Smart thing were said (certainly not by me) in this Slack thread about the PHPStan failure on the D10 patch: https://drupal.slack.com/archives/C014CT1CN1M/p1645198628262459

This issue is moving backwards faster than a moonwalking MJ...

$PP++ on #3265121: Remove Symfony 4 RequestStack BC shim in 11.0.x which breaks all tests when core/phpstan-baseline.neon is changed in a patch/MR.

MJ doing a side-step in the remove-quickedit-moonwalk: Still PP-4, but #3265121: Remove Symfony 4 RequestStack BC shim in 11.0.x is being swapped for #3265356: Deprecate Symfony 4 RequestStack BC shim for removal in D11.

One less PP after #3265356: Deprecate Symfony 4 RequestStack BC shim for removal in D11 landed.

#3264945: Move quickedit help topics to quickedit module was committed.

@Spokje: thank you for your hilarious sisyphean commentary along the way — been there, done that, I wasn't as funny :P

Added another blocker: #3267258: Remove Quick Edit support from editor.module

I move #3267258: Remove Quick Edit support from editor.module to a child of the Meta for tracking the steps to remove Quickedit in one place, #3228986: [Meta] Tasks to deprecate Quick Edit.

Thanks for working on removing QuickEdit, I am not going to miss it. I never used it, but it often gives false positives in the tests, such as these two WebDriver\Exception\UnknownError: element click intercepted: Element ... is not clickable at point recently:

• https://www.drupal.org/pift-ci-job/2372095
• https://www.drupal.org/pift-ci-job/2370974

Trying to get this to follow the policy that we've been developing. The parent issue for the removal of a module is to be a Meta that tracks all the removal work. For quickedit that is #3274155: [Meta] Tasks to remove Quick Edit from core and move to contrib, changing parent.

#3265140: Move QuickEditImageController from image to quickedit is in.

We still have #3267258: Remove Quick Edit support from editor.module and #3264633: Remove \Drupal\layout_builder\QuickEditIntegration and refactor it so that quickedit contrib provides the integration with layout builder.

This is unblocked now. Needs work / needs IS update because it's not immediately clear what is still outstanding here.

All blockers (see #147) are in.

As I start pushing the boulder uphill yet again, a guy chasing a boulder thundering downhill shouts at me: "Famous Last Words!"

Plenty of references outside of quickedit.module that we can remove in 10.0.x

> Yay, thanks for getting this one ready! But we still have to mark it deprecated before we can remove it. ;)

Yes, but both issues have to be ready at about the same time. Starting with this one gives (at least me) a better overview of what gets in the way of both issues being RTBC. Deleting stuff always shows errors earlier IMHO.

And to prove my case, we have a new postponing issue: #3291018: Move \Drupal\Tests\ckeditor5\Functional\CKEditor5QuickEditLibraryTest to the quickedit namespace/directory

And one more postponing issue discovered while reviewing the one @Spokje just opened: #3291047: Move Quick Edit-specific styling of CKEditor 4 & 5 into Quick Edit module.

And one more postponing issue discovered

Erm...thanks?

There's really _nothing_ easy and/or straightforward on the deprecation/removal of this specific module.

I think it proves that it's a good decision to remove it at least, so much supporting code and test coverage scattered around core for something that's an admin feature.

There's really _nothing_ easy and/or straightforward on the deprecation/removal of this specific module.

💯

I think it proves that it's a good decision to remove it at least, so much supporting code and test coverage scattered around core for something that's an admin feature.

I don't disagree with what you say here, but … I don't agree this is an "admin" feature. It's a content creator/editor feature. That's why it's scattered all over: because it blurs the lines between "back end" and "front end". That blurring requires code and integration tests.

Don't get me wrong: I'm happy to see this removed! The UX was never good enough. (I can say that, because I'm the person who probably worked on this the most. 😅)

It's a content creator/editor feature

I kind of see these as a subset of admin, although yes it's more of a venn diagram depending on the site.

Last, last, last last, last, last blockers are in (again, again, again again).

🥳 Hopefully the very last round on this one, @Spokje! 🤞

Thank @catch and @Win Leers.

I'll circle back to this one (and the deprecation one) tomorrow.
Off for some beers in the "Rolling Stone" bar with me ol' pal Sisyphus tonight.

Added release note snippet.

FIxtures were changes, so adding tests for different DBs.

Big MR, let's try to break it down:

- Removal of directory core/modules/quickedit.
- Changes in composer.lock and core/composer.json to remove the module.
- Rebuild core/misc/cspell/dictionary.txt since we deleted a lot of files.
- Uninstalled Quick Edit from core/modules/system/tests/fixtures/update/drupal-9.3.0.bare.standard.php.gz
- Uninstalled Quick Edit from core/modules/system/tests/fixtures/update/drupal-9.3.0.filled.standard.php.gz
- Removed Quick Edit from $expected_enabled_modules in core/modules/system/tests/src/Functional/UpdateSystem/UpdatePathTestBaseFilledTest.php
- Removed (P)CSS files from themes that will stay in core (Claro, Olivero, Starterkit, Umami), leave them in the others (Bartik, Classy, Seven, Stable, Stark). Discussed this briefly with @catch, we're doing the same thing here as we decided to do with templates. (See https://www.drupal.org/project/drupal/issues/3264120#comment-14456294)
- Removed Quick Edit from core/MAINTAINERS.txt
- Removed annotations in the plugins \Drupal\Core\Field\Plugin\Field\FieldFormatter\BasicStringFormatter, \Drupal\Core\Field\Plugin\Field\FieldFormatter\StringFormatter, \Drupal\comment\Plugin\Field\FieldFormatter\CommentDefaultFormatter, \Drupal\comment\Plugin\Field\FieldFormatter\CommentPermalinkFormatter, \Drupal\image\Plugin\Field\FieldFormatter\ImageFormatter, \Drupal\responsive_image\Plugin\Field\FieldFormatter\ResponsiveImageFormatter, \Drupal\text\Plugin\Field\FieldFormatter\TextSummaryOrTrimmedFormatter and \Drupal\text\Plugin\Field\FieldFormatter\TextTrimmedFormatter.

So by now the remaining references to Quick Edit are:

- CSS and JS in ckeditor5. I have no clue if that should be removed?
- Help topic for Settings Tray. This is OK, the functionality "Quick Edit" is a contained in the Settings Tray module and is different from the Quick Edit module.
- CSS for themes Seven, Stable, Stable9. This is OK, these are all themes that will be deprecated.
- Annotations in the plugins \Drupal\Core\Field\Plugin\Field\FieldFormatter\BasicStringFormatter, \Drupal\Core\Field\Plugin\Field\FieldFormatter\StringFormatter, \Drupal\comment\Plugin\Field\FieldFormatter\CommentDefaultFormatter, \Drupal\comment\Plugin\Field\FieldFormatter\CommentPermalinkFormatter, \Drupal\image\Plugin\Field\FieldFormatter\ImageFormatter, \Drupal\responsive_image\Plugin\Field\FieldFormatter\ResponsiveImageFormatter, \Drupal\text\Plugin\Field\FieldFormatter\TextSummaryOrTrimmedFormatter and \Drupal\text\Plugin\Field\FieldFormatter\TextTrimmedFormatter. I know Core doesn't support Contrib, but if we remove these I think we'll basically cripple Contrib Quick Edit massively.

I thought we had just moved all CSS and JS referencing Quick Edit out of the CKEditor 5 module in the issue I started yesterday? (#3291047: Move Quick Edit-specific styling of CKEditor 4 & 5 into Quick Edit module) :O

Regarding for matters having Quick Edit-related annotations: those can all be moved to the contrib module, the contrib module can then just *alter* those annotations 😊

Thanks @Wim Leers:

I thought we had just moved all CSS and JS referencing Quick Edit out of the CKEditor 5 module in the issue I started yesterday?

Me too, but still:
#3292626: Remove core/modules/ckeditor5/css/quickedit.css https://git.drupalcode.org/project/drupal/-/blob/10.0.x/core/modules/cke...
https://git.drupalcode.org/project/drupal/-/blob/10.0.x/core/modules/cke...

Regarding for matters having Quick Edit-related annotations: those can all be moved to the contrib module, the contrib module can then just *alter* those annotations

So you're saying: Nuke them in this issue?

Regarding for matters having Quick Edit-related annotations: those can all be moved to the contrib module, the contrib module can then just *alter* those annotations

So you're saying: Nuke them in this issue?

Reread and decided it is indeed an OK for removal of the annotations.

Postponed on #3266476: Ensure that quickedit does not get special core treatment per Slack discussion here: https://drupal.slack.com/archives/C014CT1CN1M/p1655750184381089
Also needs manual testing when that blocker lands (see also Slack discussion)

The main thing we need to do before removing them from 10.0.x is ensure that the composer façade naming for them is fixed, and have someone manually test EACH to confirm downloading the contrib module in D9 and installing it works SEAMLESSLY, and that it then does again if the rm -rf core/modules/foo

According to @Mixologic #3266476: Ensure that quickedit does not get special core treatment is actually done for quickedit, just the issue was not updated.

So.. I tested the upgrade path from 9.5.x to 10.0.x

First, in 9.5.x, I tried to install quickedit via composer, this does not work:

Your requirements could not be resolved to an installable set of packages.

  Problem 1
    - Root composer.json requires drupal/core 9.5.x-dev -> satisfiable by drupal/core[9.5.x-dev].
    - Only one of these can be installed: drupal/quickedit[1.0.0-rc1, 1.0.0, 1.0.1, 1.0.x-dev], drupal/core[9.5.x-dev]. drupal/core replaces drupal/quickedit and thus cannot coexist with it.
    - Root composer.json requires drupal/quickedit ^1.0 -> satisfiable by drupal/quickedit[1.0.0-rc1, 1.0.0, 1.0.1, 1.0.x-dev].

You can also try re-running composer require with an explicit version constraint, e.g. "composer require drupal/quickedit:*" to figure out if any version is installable, or "composer require drupal/quickedit:^2.1" if you know which you need.

Installation failed, reverting ./composer.json and ./composer.lock to their original content.

This is because core/composer.json in 9.5.x still specifies quickedit as replaced. We might need to just tell people to wait until updating to 10.0.x to install the contrib module. #3188544: [policy discussion] Address Composer namespacing issues when extensions move between core and contrib will eventually resolve this in all directions.

Then I checkout out 10.0.x and ran composer update.

Because this issue hasn't landed yet, you also can't composer require drupal/quickedit in 10.0.x either for the same package replacement issue, so I applied this MR to arrive in the proper 10.0.x state with no quickedit references in core.

After that composer require drupal/quickedit runs fine:

composer require drupal/quickedit
Using version ^1.0 for drupal/quickedit
./composer.json has been updated
Running composer update drupal/quickedit
> Drupal\Composer\Composer::ensureComposerVersion
Loading composer repositories with package information
Updating dependencies
Lock file operations: 1 install, 0 updates, 0 removals
  - Locking drupal/quickedit (1.0.1)
Writing lock file
Installing dependencies from lock file (including require-dev)
Package operations: 1 install, 0 updates, 0 removals
  - Installing drupal/quickedit (1.0.1): Extracting archive

I hadn't run update.php yet (I forgot, but also this is the correct order to do things anyway). Got a whitescreen hitting the front page, update.php loads fine, ran the one olivero update that was pending, no weird updates to run for quickedit or anything.

I did find #3291700: new subtree split of core Quick Edit into contrib (v2), which looks to me like it might just be quickedit needing a new subtree split update from core since some commits are missing given the contrib project was created so long ago before we knew there'd be another 15 pending issues blocking its removal.

Leaving PP-1 on the update to the contrib module.

I think removing [PP-1] from the issue title might make things a bit more clear.

Leaving PP-1 on the update to the contrib module.

Ah, missed that, reverting title and putting this back on postponed, since it is...erm...postponed on #3291700: new subtree split of core Quick Edit into contrib (v2). Adding that to the IS as well.

On a more cheery note:

We might need to just tell people to wait until updating to 10.0.x to install the contrib module. #3188544: [policy discussion] Address Composer namespacing issues when extensions move between core and contrib will eventually resolve this in all directions.

Discussed this briefly with catch on Slack and as a (valid?) workaround we propose:

composer require drupal/quickedit for 10.0.x
composer require drupal/quickedit-quickedit for 9.4.x/9.5.x

#3292626: Remove core/modules/ckeditor5/css/quickedit.css is just sloppy, but not affecting this issue at all.

It seems like this is the final code-blocker: #3292780: Move Quick Edit related Javascript from core/modules/ckeditor5/js/ckeditor5.es6.js::detach() to the Quick Edit module.
#FamousLastWordsThatMostLikelyWillNotBeTrueYetAgain

Since this is a JavaScript issue, I myself tried yesterday to do this myself, and failed in various, spectacular ways.
Somebody with an actual (JavaScript-minded) brain is needed to get that one over the line.

Sisyphus approves this message.

#3292780: Move Quick Edit related Javascript from core/modules/ckeditor5/js/ckeditor5.es6.js::detach() to the Quick Edit module was committed, this issue now needs #3291700: new subtree split of core Quick Edit into contrib (v2) to land and manual testing after that (https://drupal.slack.com/archives/C014CT1CN1M/p1658772338310209)

Note
This MR (against 10.0.x) needs:
- the fixture changes updated
- a separate patch against 10.1.x.

Since fixture changes are tedious work and there are a lot of those floating around in several other issues that are close to being committed, I (=Spokje) have given up on updating the MR until the dust settles a bit and I can do the update in one go.

By the time this issue is actually not being postponed any more it should be quick and easy enough (#FamousLastWords) to create the 10.1.x-patch by me or basically anybody.
The fixture update is a bit more elaborate, but I strongly believe that I since I've figured it out, so can anybody.

If anybody feels inclined to keep the MR up-to-date whilst this issue is still postponed: Please be my guest :)

I'm going to be bold and say we can continue moving this forward. I wrote manual test steps in #3303780: Manually test QuickEdit module removal that when combined with the patch in #3291700: new subtree split of core Quick Edit into contrib (v2) mean that I can successfully upgrade from a Quick Edit-enabled Drupal 9.4 to 10.0.x.

@Spokje as the king of fixture rerolling if you want to give this another pass I am happy to review :)

Is #3303780: Manually test QuickEdit module removal supposed to include applying the latest MR patch from here? It doesn't currently mention that step

No it's fine for manual testing to just rm -rf or similar.

Similar to my comment over in #3270899: Remove Color module from core I think the remaining thing to do here is leave behind a stub quickedit.install that cleanly uninstalls Quick Edit for anyone who upgrades to Drupal 10 without installing the contrib module.

Removed a bunch of Quick Edit CSS and images from stable and stable9 that is provided by the module already.

Some things remain that I'm not sure what to do with:

1. Seven provides extra stylesheets for Quick Edit. As Seven is being removed from core anyway, do we leave them intact for users who are already using Seven and Quick Edit and want to continue that in contrib in Drupal 10?
2. Settings Tray has some JS to handle Quick Edit. Do we port this to the contrib module (which might be tricky), or just remove it?
3. Settings Tray also has tests that apparently rely on Quick Edit selectors, yet they aren't failing once Quick Edit has been removed!

Re #208.3 the tests triggers a mouseleave event on Quick Edit selectors, but if the selectors don't exist then the code is a no-op so this appears safe to remove.

Out of scope here but once Quick Edit is gone we can (hopefully) remove the skipped test in SettingsTrayBlockFormTest as it's possible the random fail was Quick Edit related.

Seven provides extra stylesheets for Quick Edit. As Seven is being removed from core anyway, do we leave them intact for users who are already using Seven and Quick Edit and want to continue that in contrib in Drupal 10?

IMO we should just leave that in so that it'll be there in the contrib version.

Settings Tray has some JS to handle Quick Edit. Do we port this to the contrib module (which might be tricky), or just remove it?

I found #2944810: [PP-1] Add tests edit various fields with QuickEdit module when Settings tray is enabled. and #2944145: [PP-1] Settings Tray prevents editing some fields with QuickEdit module which suggest not only is the current logic broken and untested, but that fixing it is dependent on fixing other test coverage. This is extremely tangled, and part of the maintenance burden of supporting conflicting UX patterns between contextual links, quickedit, and settings tray, so I don't think it should block removal. I do think we should open an issue against the contrib module and maybe manually test the behaviour (10.x quickedit and HEAD, 10.x quickedit + this MR) to see whether if and what the regression is. A possible quick fix for the contrib module would be to fork current HEAD's settings_tray.js entirely, and then replace the core version with that, then that would provide parity until we change settings tray in core. I've opened #3304909: Skip contrib Quick Edit's Settings Tray integration test until Settings Tray provides an API to improve integration in #3308867.

Rerolled with updated fixtures after #3270899: Remove Color module from core landed.

Unsure what to do with the thread in the MR about the generalisation of the comment. Are we OK after we remove the term "Quick Edit" and mention something like "Some modules require the current data to update EditorModel."?

Tried to answer the thread on the removal of stable/images/image/(error|upload).svg from stable.

"Some modules require the current data to update EditorModel."

Maybe 'Allow modules to update EditorModel by providing the current data' but tbh I don't know what's going on there so I'm just trying to rephrase a sentence.

don't know what's going on there so I'm just trying to rephrase a sentence.

In the same boat as you, but at the very least it removes a mention of Quick Edit, so I ran with your suggestion.
Real JS boffins can correct it later, if needed.

Put a patch up on #3304893: Add Quick Edit metadata to core field formatters. Can't verify the patch on DrupalCI until this issue is committed to core but it's pretty trivial.

Not sure what's going on with the DrupalCI errors here.

Not sure what's going on with the DrupalCI errors here.

Rerolled, retested, all OK now.

Added the Stable images back again, they won't harm anything by being there, and there is a tiny chance someone is using them. Let's remove them when we get rid of Stable itself.

With the MR applied, the string "quickedit" still appears in the following files:

core/misc/cspell/dictionary.txt
core/modules/settings_tray/js/settings_tray.es6.js
core/modules/settings_tray/js/settings_tray.js
core/themes/seven/css/components/quickedit.css
core/themes/seven/seven.info.yml

We agreed to leave the Seven integration there, but I think the ST thing probably needs to be addressed? We discussed it with the random test fail above, but it's still there in the MR.

Meanwhile, "quick edit" appears in:

core/modules/help_topics/help_topics/core.settings_tray.html.twig
core/modules/settings_tray/js/settings_tray.es6.js
core/modules/settings_tray/tests/src/FunctionalJavascript/SettingsTrayBlockFormTest.php
core/modules/settings_tray/tests/src/FunctionalJavascript/OverriddenConfigurationTest.php
core/modules/settings_tray/settings_tray.links.contextual.yml
core/modules/settings_tray/settings_tray.module
core/themes/seven/css/components/quickedit.css

The Settings Tray help topic (mentioned previously) is still kicking around. So I guess this is blocked on #3264949: Move Quick Edit help topics to contrib Quick Edit module also. But it looks like there's some more mentions to get rid of in ST code especially.

#3227033: Remove Quick Edit from core is open regarding the settings tray integration and the contrib module, I think we just need to remove that code from the MR here.

@catch Is there a different issue you meant to link?

Sorry, this one: #3304893: Add Quick Edit metadata to core field formatters.