new subtree split of core Quick Edit into contrib (v2)

For now Quick Edit in the state it is in commit 1c5c378e02a692dc59952c3904dd5131bbe6971e on the 10.0.x branch is the closest this module has been to be removed.

As stated in #3227033-181: Remove Quick Edit from core there's still one bit of CSS and one bit of JS in the CKEditor5 module that has to be decided on. Could be deleted altogether or could be moved to the Quick Edit module, which in turn would mean moving it to this Contrib incarnation.

Yeah, definitely needs another subtree split. I put this as a step in one of the metas, but I didn’t open an issue here for it. Thanks! I’ve been waiting for the dust to settle in core before working on this. But I’ll make some time to do it today if we’re ready.

Hopefully this goes smoothly, but it might be a bit messy since I had already pushed some contrib-specific commits on top of the core history in the 1.0.x branch to get the 1.0.0 (and 1.0.1) releases out. Perhaps I can work with Drumm to try to get an exception to allow a force push to do this with minimum hassle and maximum historical accuracy.

I’ve been waiting for the dust to settle in core before working on this.

.

Well, technically there are still changes "up in the air" (see the mentioned CKEditor5 CSS/JS in #2), but seeing that Contrib Quick Edit is currently broken, it _might_ be better to do a "forced subtree split deluxe 2022 special" now and (maybe) one more later?

I pushed the commits to an issue fork here and opened MR 7. Anyone care to review before I cut 1.0.2 with this? Thanks!

For posterity, here's the Slack convo that lead to this approach:

dww
:question: What's the best way to deal with trying to re-subtree split Quick Edit from 9.5.x core into contrib, given that I pushed a few contrib-only commits to to 1.0.x contrib branch for both 1.0.0 and 1.0.1 releases. More in :thread: Thanks! :pray:

dww
Since the history diverged, this was failing since it wasn't a fast-forward push:

git remote add qe-contrib /path/to/contrib/quickedit
git fetch qe-contrib
git subtree push --prefix=core/modules/quickedit qe-contrib 1.0.x

dww
I pushed the new subtree split into a clean 1.0.x-resplit branch in my local repo, which worked.

dww
Now I've got:

• 1.0.x is what d.o has and wants only fast-forward pushes to.
• 1.0.x-resplit diverges and can't be cleanly pushed on its own, and doesn't have the contrib-ification I already performed.

catch
@dww I haven't really dealt with this, but how about merging or cherry picking from that branch? This might be a problem for the next one though (although maybe not if you merge or cherry-pick again)

dww
Right, so I did an experiment branch to start clean from 1.0.x, then merge 1.0.x-resplit. There were some conflicts, but not too bad, and now resolved.

dww
But if we have to do another subtree split, I don't believe it'll understand where it left off.

catch
couldn't you do it all again with another new clean branch?

dww
So we'll have to do more git gymnastics. Hopefully this is the last time we need to do this! :stuck_out_tongue_winking_eye: :crossed_fingers:

dww
Yeah, I could probably do something similar again if needed...

catch
It doesn't seem like the right way, but the right way probably involves force pushes.
:100:

catch
And it doesn't seem too bad either.

dww
Not terrible, no. I'm ready to push the 25 commits to 1.0.x.

dww
Heh, it'll be 26. The merge conflicts included a duplicate quickedit.inPlaceEditor.formattedText entry in quickedit.libraries.yml.

Confirming tests before I put this up for review.

Ugh, even wrapping the function like this:

/**
 * Implements hook_quickedit_render_field().
 */
if (!function_exists('layout_builder_quickedit_render_field')) {
  function layout_builder_quickedit_render_field(EntityInterface $entity, $field_name, $view_mode_id, $langcode) {
    /** @var \Drupal\quickedit\LayoutBuilderIntegration $layout_builder_integration */
    $layout_builder_integration = \Drupal::classResolver(LayoutBuilderIntegration::class);
    return $layout_builder_integration->quickEditRenderField($entity, $field_name, $view_mode_id, $langcode);
  }
}

the 9.4 tests are still failing like this:

Fatal error: Cannot redeclare layout_builder_quickedit_render_field() (previously declared in /var/www/html/modules/contrib/quickedit/quickedit.module:229) in /var/www/html/core/modules/layout_builder/layout_builder.module on line 357

Is this because we intentionally try to load contrib before core, so when we load quickedit.module we don't see a copy, but the layout_builder.module in core doesn't know it shouldn't define it itself? Do we have to do specific version checks on layout_builder or something? Is that even possible that early in the game to be conditionally defining this function?

I think the main issue here is that #3264633: Remove \Drupal\layout_builder\QuickEditIntegration and refactor it so that quickedit contrib provides the integration with layout builder has been committed to 9.5.x, but _not_ to 9.4.x.

This means that the "moving around" of layout_builder_quickedit_render_field() didn't happen in 9.4.x, leaving you and Contrib Quick Edit with double declarations when testing against9.4.x.

I guess that if we want Contrib Quick Edit to work on 9.4.x a backport is needed.
This is probably also the case for #3291047: Move Quick Edit-specific styling of CKEditor 4 & 5 into Quick Edit module and #3291018: Move \Drupal\Tests\ckeditor5\Functional\CKEditor5QuickEditLibraryTest to the quickedit namespace/directory.

Besides the above, this also strikes me as odd:

/var/lib/drupalci/workspace/jenkins-drupal8_contrib_patches-111653/source/css/editors/formattedText.ckeditor.css ✗ 1 more
line 0	Could not read file data. Is the file empty?
/var/lib/drupalci/workspace/jenkins-drupal8_contrib_patches-111653/source/css/editors/formattedText.ckeditor5.css ✗ 1 more
0	Could not read file data. Is the file empty?
/var/lib/drupalci/workspace/jenkins-drupal8_contrib_patches-111653/source/css/editors/formattedText/ckeditor5.workaround.css ✗ 1 more
0	Could not read file data. Is the file empty?
/var/lib/drupalci/workspace/jenkins-drupal8_contrib_patches-111653/source/css/editors/image.css ✗ 1 more
0	Could not read file data. Is the file empty?
/var/lib/drupalci/workspace/jenkins-drupal8_contrib_patches-111653/source/css/editors/image.theme.css ✗ 1 more
0	Could not read file data. Is the file empty?
quickedit 1.0.x-dev branch result

(Bottom the page under "5 coding standards messages" on https://www.drupal.org/pift-ci-job/2409103)

When I look in the MR there seems to be code in the above files?

C/p from Slack: (https://drupal.slack.com/archives/C014CT1CN1M/p1655973880916309?thread_t...)

I think we (at least) have a problem with the actual git split.

Compare:
https://git.drupalcode.org/project/quickedit/-/tree/1.0.x/css/editors
https://git.drupalcode.org/project/drupal/-/tree/9.5.x/core/modules/quic...

and:

https://git.drupalcode.org/project/quickedit/-/tree/1.0.x/js
https://git.drupalcode.org/project/drupal/-/tree/9.5.x/core/modules/quic...

Especially the js folder, I don't remember we moved the sub-dirs modules nor views, editors could have an issue somewhere where we moved things in Core.

Right, think before you type...

The remaining test fail in #12 for 9.4.x is because #3291047: Move Quick Edit-specific styling of CKEditor 4 & 5 into Quick Edit module hasn't been backported to 9.4.x. So our choices are:

1. Backport #3291047
2. Add some conditional hacks to this test to assert different things depending on which version of core we're running on.

#2 feels a bit gross, but I'm willing to do it if backporting #3291047 is considered too disruptive / risky in a patch release.

Tagging this for a release manager to tell me what to do.

Thanks,
-Derek

This is now working for 9.4.x and 9.5.x. In Slack, @catch proposed merging this as-is, and opening another issue to get QE contrib working in 10.0.x.

I would advice against committing this.

1) There's still one blocking code issue left #3292780: Move Quick Edit related Javascript from core/modules/ckeditor5/js/ckeditor5.es6.js::detach() to the Quick Edit module, so there would be a need for Yet Another Subtree Split.
2) Since we will IMHO not be supporting 9.4.x, because we'll (hopefully) be deprecating in 9.5.0, I would revert the last "hacky" commit. This is already present in 9.5.x.
3) Currently the presence of the replace: "drupal/quickedit": "self.version", in /core/composer.json means no one can install the Contrib module anyway.

Which brings me to: How is it possible that test can even run in this issue? When I try something similar in #3293311: [ignore] Spokjes Subtree Split Shindig I get shot down by (emphasis mine)

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].
    - drupal/quickedit 1.0.x-dev is an alias of drupal/quickedit dev-1.0.x and thus requires it to be installed too.
<strong>    - Only one of these can be installed: drupal/quickedit[dev-1.0.x], drupal/core[9.5.x-dev]. drupal/core replaces drupal/quickedit and thus cannot coexist with it.</strong>
    - Root composer.json requires drupal/quickedit 1.0.x-dev -> satisfiable by drupal/quickedit[1.0.x-dev (alias of dev-1.0.x)].

Is some kind of magic elven dust sprinkled on this issue?

Thanks for the reviews, testing and continued efforts!

Re: #18

1. Yes. But wouldn't it be worth fixing the fatal errors you get now in 9.4.x and/or 9.5.x by merging this, cutting another release, and moving on? Now that I'm getting the flow of them, I think I'll be able to handle the subtree splits well in future issues/MRs.
2. But what about early adopters that want to start migrating to contrib ASAP? Doesn't really hurt to leave this test defensive enough to work in both branches, now that it's already working, right? It's only test code, I'd potentially care a little more about introducing "hacks" into the actual code paths for QE itself.
3. Yes, and there are other ways to ensure you get the code you want. 😉 Including the work around from #19.

I'm inclined to proceed... any objections?

I'm OK with the maintainer of this module doing extra work ;)

But what about early adopters that want to start migrating to contrib ASAP?

They have no fighting chance until #3292303: Remove replace section in core/composer.json for several deprecated modules lands, or unless they use the (probably to-be-removed at some point in time) composer require drupal/quickedit-quickedit.

But as said: @dww, You're the maintainer of this Contrib Module (and I personally applaud you for stepping up!), so I'm gonna quote the committing committee on this: "If you wanna commit: Commit to committing!"

I've manually cherry-picked four commits from 9.5.x to this branch, which should bring it up to date:

#3292780: Move Quick Edit related Javascript from core/modules/ckeditor5/js/ckeditor5.es6.js::detach() to the Quick Edit module
#3281535: Fix 'Access to an undefined property' PHPStan L0 errors in test code
#1988968: Drupal.ajax does not guarantee that "add new JS file to page" commands have finished before calling said JS
#2935999: Remove Layout Builder's hard dependency on Field UI

I couldn't figure out how to update the subtree split so I did this manually. I used git log core/modules/quickedit to find the individual commits, dumped them each to a file with e.g. git show dc1441d3392ebbda30d217ae3ed0d678d933632d > modules/quickedit/2935999.patch, applied that patch to the module with patch -f -p4 < 2935999.patch, then committed the changes, copying the commit message from the patch file.

I did some manual testing for the 9.4.x to 10.0.x upgrade path, which I will shortly document in another issue. While doing this I found the root cause of Symfony\Component\HttpKernel\Exception\BadRequestHttpException: Input value "fields" contains a non-scalar value. which is a missing fix from this issue:

#3254250: [Symfony 6] Retrieving a non-string value from "Symfony\Component\HttpFoundation\InputBag::get()" is deprecated

Quick Edit contrib version works for me in 10.0.x now, so I committed this to the branch as well.

I’m planning to do a final round of testing on this, merge it, and tag a new release tomorrow. Thanks everyone!

@longwave: Thanks so much for diving into this! I rebased your 5 commits to update the --author to match the core commits. Probably should have added 'cherry picked from commit zyx' lines, too, but I didn't think of that until after I merged it to 1.0.x. 😉

Split off #3304020: Tests failing on 10.0.x.
Also want to get #3303241: Bump core requirement to ^9.4 || ^10 in before I tag 1.0.2, but I think we're now very close.

Yay, and thanks everyone! 🎉🙏