Remove \Drupal\layout_builder\QuickEditIntegration and refactor it so that quickedit contrib provides the integration with layout builder

Here's a start. Haven't tried to refactor \Drupal\layout_builder\Entity\LayoutBuilderEntityViewDisplay::getQuickEditSectionComponent and haven't tested it at all.

Thanks, y'all! Standing by to commit whatever's supposed to go into QE contrib into place. ;) I'll try to review the core patches as they arrive...

The test passed, but I don't really know what it is testing, nor what the integration provides. In manual testing before the patch I could quickedit the body field of a node that was rendered with layout builder but with the patch I couldn't.

Update on manual testing. Previously I still had the contrib module in place, so I was not actually testing the patch. I have tested again today with the steps below and the integration appears to be working:

1. Enable layout builder for article content type
2. Create an article node with some text in the body
3. Quickedit the node. If the integration works you can update the body field. Without the integration you can only change the title.

That just leaves LayoutBuilderEntityViewDisplay::getQuickEditSectionComponent to be refactored in to quickedit module.

There are actually at least three tests that cover the layout builder / quickedit integration:

• \Drupal\Tests\quickedit\Functional\LayoutBuilderQuickEditTest
• \Drupal\Tests\quickedit\FunctionalJavascript\LayoutBuilderIntegrationTest
• \Drupal\Tests\quickedit\FunctionalJavascript\LayoutBuilderQuickEditTest

I have updated the MR to move all remaining quickedit references in layout_builder. Let's see what the testbot thinks.

The latest commit 18f61cf5 should make the remaining tests pass, but they really don't provide any value. I believe #3043245: Allow marking a config schema types to be marked as "internal" (to avoid exposing them via HTTP APIs) would mean we could remove them. They also shouldn't need to exist in contrib because the test coverage tests won't be looking for them.

From a Layout Builder perspective, this looks great. I didn't have a chance to review it from a BC perspective, but untagging for subsystem maintainer review for now.

Don't know why that's failing now when it was passing before and is passing locally. I merged in 9.4.x and made some minor adjustments, let's try again.

This makes a lot more sense now that I see #3263654: Move all HAL tests to the module in preparation of removal in D10. So the question is should the hal tests for quickedit live in the hal module since both modules are moving to contrib?

So the question is should the hal tests for quickedit live in the hal module since both modules are moving to contrib?

If we move the hal tests for quickedit to hal, then it means hal needs to get removed from core before quickedit, otherwise its tests would start to break. Since they're both very close, it shouldn't matter.

The way the coverage checker test is written means the tests have to live in hal for now as per the last commit in this MR.

I just did a subtree split for hal (awww xD), but can always just force push a new split since there was no real work done yet.

@bbrala I don't think hal in contrib would need to include these tests. They can possibly exist in quickedit contrib but they really only exist right now to make core tests happy.

Hmm, ok, we'll see. Feels like i rather split the exact state it is at the point it is removed. So if there is extra tests added, i'd split with those, then remove in contrib if needed.

But that might be extra work that is not needed, IF the tests live in a separate file to other HAL tests.

Great, thanks! And thanks for confirming my suspicions @larowlan, good to be able to point to the actual place for that :)

I suspect this is RTBC, but I’d like to take a look before this happens in core, not just when this is all hitting QE contrib. I botched this on help topics, and only noticed some problems late. Trying to be more proactive this time.

The CR looks good (reviewed it while checking links on deprecation messages), although don't we usually put the exact release something was introduced in? Should it not be 9.4.0-alpha1, not just 9.4.0?

Meanwhile, I opened about a dozen new MR threads. Mostly nits and probably-out-of-scope stuff, but a couple of points that are legit reasons this needs work before commit.

Thanks,
-Derek

Updated the test groups and added the moduleExists check.

The CR looks good (reviewed it while checking links on deprecation messages), although don't we usually put the exact release something was introduced in? Should it not be 9.4.0-alpha1, not just 9.4.0?

In the codebase I searched core for other deprecations and there are plenty of 9.4.0 deprecations and zero for 9.4.0-alpha1. There are also plenty of CRs for 9.4.0 and only one for 9.4.0-alpha1.

In the codebase I searched core for other deprecations and there are plenty of 9.4.0 deprecations and zero for 9.4.0-alpha1

Yes this is correct, we always do the next stable release.

Thanks! Looks like this is ready for re-review. Can’t respond to specifics now, but I hope to look later today...

Changing parent to the issue tracking the steps to deprecate quickedit.

This is going to need a 10.x version without the HAL changes, since HAL module is already removed.

Merged 9.5.x in to the existing MR and cherry-picked the commits in to a new 10.0.x MR. Best to review the 9.5.x MR as it has existing comments.

Needs work as per latest MR feedback

Updated deprecation message in 9.x PR and removed the class in 10.x PR. Updated CR.

This looks good to me, reviewed both PRs and can't find anything that's worth holding this up on.

There's a few unresolved threads in the 9x PR but I think they're technically resolved.

Setting back to NR since I added another commit

@mstrelan and I discussed whether using the layout builder class constant in quickedit was safe in the case where layout builder was disabled and decided to go with the string to be safe. Back to RTBC

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

I've backported this to 9.4.x, because not having it there is causing issues with contrib quickedit + 9.4.x

While this introduced a couple of deprecations, it's not API surface we expect anyone to be using, so I think we should treat it similar to a constructor deprecation to fix a bug. If it turns out there's a problem, there's time to revert before the next patch release of core, but we'd then need do diverge contrib quickedit further from the core version which further increases the chance of more bugs.

Tagging for 9.4.2 release notes.

Adding CR link to the release note. Thanks!

Release note was already added in #51 👍