Allow using images from external source

Good call!

When we do this, let's make sure we do not make the same mistake that was made for ckeditor.module in core, which still has this issue open: #2771837: drupalimage CKEditor plugin should not require data-entity-uuid and data-entity-type when image upload is disabled.

CKEditor 4 allows using images from external sources when image upload is not enabled.

What does this mean? The HTML is not filtered out? Or a UI is provided? Or something else?

Hm, but those external images would also still work when the image plugin is enabled? Which is not true for CK5 then because it would only allow internal ones?

It does sound like a potential use case though if someone implemented image support differently let's say? Would they be responsible for providing a plugin as they migrate?

Per #10.

#14++ — that's what I recall from the meeting with CKE5 developers too.

IMO how this would ideally work, is we would provide configuration in the editor form that would allow enabling image inserts by an URL even when image upload is enabled. There's a pre-existing UI in CKEditor 5 for this meaning that on CKEditors integration side, this should be straight forward to implement.

Note that this has never worked in Drupal 8|9 + CKEditor 4: #2510394: [drupalImage] Setting to still allow linking to an image by URL even when uploads are enabled. So … are we sure this should be a stable blocker? Pre-CKE5 you could also only do this by editing the source.

Note: #2170275: filter_html_image_secure will replace a (disallowed) external image, this should be visible while in CKEditor is a long pre-existing UX challenge/bug. We should not tackle that here, just linking for awareness/avoid confusion when implementing this.

#16: Totally agreed that it should be pretty straightforward. More so than in CKE4 for sure.

But I think it'd be better to have this as a Major feature request post-stable, just to ensure we meet the D10 deadline, and don't spend time on this now.

#17: Yep, that's my worry too. Especially because filter_html_image_secure is enabled in the default Basic HTML text format. So until that's done, the value of doing this is limited. This is what makes me feel pretty confident that this makes more sense post-stable.

I think the critical piece here is to ensure that the upgrade path works.

+1

But I guess from users perspective there's still the potential regression that existing images cannot be edited when image upload is disabled (including alt texts) since the image plugin wouldn't be enabled.

We should test that. I just did. Editing alt on eternal images works fine. Or perhaps you mean the case where image uploads are disabled for the (CKE5) Text Editor, and hence nothing of the image plugin loads? I bet that is what you mean.

I can get that to work using the attached patch, which tweaks the CKEditor 5 plugin definitions to make this possible. 🤓

Having done the #20 PoC, I'm actually not sure whether this from #14 is true:

I think this is 100% on our side. Image insert is demonstrated in https://ckeditor.com/docs/ckeditor5/latest/features/images/image-upload/.... We simply need to decide what is the behavior we want to provide.

… because that docs page mentions the ImageInsert plugin which adds the insertImage toolbar item. But … that does extends the uploadImage plugin — you cannot configure CKEditor 5 (yet, AFAICT) to only allow external images.

The plugin metadata also indicates this:

		{
			"name": "Image upload",
			"className": "ImageUpload",
			"description": "Allows for pasting images from the clipboard, dragging and dropping of images, selecting them through a file system dialog or from a media management tool. You need to set this plugin up with an official or a custom upload adapter.",
			"docs": "features/images/image-upload/image-upload.html",
			"path": "src/imageupload.js",
			"requires": [
				[
					"ImageBlock",
					"ImageInline"
				]
			],
			"uiComponents": [
				{
					"type": "Button",
					"name": "imageUpload",
					"iconPath": "@ckeditor/ckeditor5-core/theme/icons/image.svg"
				}
			]
		},
		{
			"name": "Image insert",
			"className": "ImageInsert",
			"description": "Allows for inserting images via source URL",
			"docs": "features/images/image-upload/images-inserting.html#inserting-images-via-source-url",
			"path": "src/imageinsert.js",
			"requires": [
				"ImageUpload"
			],
			"uiComponents": [
				{
					"type": "SplitButton",
					"name": "imageInsert",
					"iconPath": "@ckeditor/ckeditor5-core/theme/icons/image.svg"
				}
			]
		}

Yep, but … we could start by allowing external images in addition to uploaded images. We could make that an extra piece of configuration easily.

IOW: do we really need to support the case of not supporting uploaded images and only allowing external images? That hasn't worked historically, and it sets a weird/wrong expectation: "hotlinking images is okay" — it isn't.

If it is okay to add the image insert via URL like here, then attached patch let me test it out.

Screenshot of the active toolbar when editing the text format (so this would need a new static SVG icon I think given the dropdown arrow is separate):

Screenshot of the toolbar when editing content:

PR to allow insertImage without requiring uploadImage for CKE5 created here: https://github.com/ckeditor/ckeditor5/pull/11571

Missed patch on previous comment, but probably needs a different approach anyways if the CKE5 PR gets in.

WOW! 🤩 Epic work, @scott_euser!

Assigning to @lauriii for review.

Ah thanks for updating the issue; wasn't sure if we were moving to the PR to have the back and forth there or keeping these two in sync.

#25 + #27: Would it be possible to not add a new toolbar item? 🤔 The screenshot in #25 shows that there's now two visually indistinguishable buttons. That's bad for UX — both for the site builder and the content creator.

I think ideally there would be a single button. When the Enable image uploads checkbox would be checked, it'd only allow uploads (basically: uploadImage). When the Enable image uploads checkbox would be unchecked, it'd only allow URLs (basically: imageInsert).

Thoughts? 😊

HI,
Applied patch #27 and it's working fine.
Patch added a new icon that can add an external image as well.
But there were two 'insert image ' buttons since the old one also enabled, both seem identical icons.
(There was an option to enable both. screenshot attached).

thankyou

I think we need to wait for the upstream ckeditor side first right? For now we can decide a plan forward, but the patch I wrote was before any ckeditor work so it's not fit for purpose.

I imagine it's a single configurable Plugin where the user decides whether to allow image upload, image insert (via url), or both, and default to image upload only? What do you think?

We definitely are blocked on upstream! 😅

I was just trying to get the patch through a first review round already, I didn’t realize it predated your CKEditor 5 PR! 🙈

Waiting for it to land upstream is probably the sensible thing to do. Thanks *so* much for already getting us so far!

Discussed during today's CKEditor 5 meeting. The PR that @scott_euser created is likely too much of a BC break for them to go in as-is.

They're aware of the problem. They're awaiting a write-up from either @lauriii or I. I'm going to ask @lauriii to create that insertImage vs uploadImage upstream issue that explains the capabilities we need.

In a nutshell:

• Drupal needs (to not break BC): A) only uploads, B) only URLs.
• CKEditor 5 has: A) only uploads, C) both uploads + URLs

We need CKEditor 5 to make option B possible. But Drupal is happy to also add C on the Drupal side, but we definitely need to continue to support A + B too — C has a different set of security implications.

The actual code for the insert via url template is independent of the plugin. The plugin is essentially just creating the 'dropdown' as they call it, and rendering the insert via url template into that.

So it should not be too hard for us I think to create a plugin of our own within Drupal for an independent insert via url which uses the CKE5 core template.

So A and C are supported by CKE5 core and B within Drupal. Is that in line with what you are thinking?

Thanks!

Great news: @Reinmar told us this is now officially in their sprint that started this Monday, with @kuba working on it! 🥳

https://github.com/ckeditor/ckeditor5/pull/11980 was merged 16 days ago, just after the last tagged release, so should be in the next one.

Should be unblocked.

Unblocked thanks to #3301495: Update CKEditor 5 to 35.0.1 having landed.

Today:

ckeditor5_image:
  ckeditor5:
    plugins:
      - image.Image
      - image.ImageToolbar
      - drupalImage.DrupalImage
    config:
      image:
        toolbar: [drupalImageAlternativeText]
  drupal:
    label: Image
    library: ckeditor5/drupal.ckeditor5.image
    elements:
      - <img>
      - <img src alt data-entity-uuid data-entity-type height width>
    conditions:
      toolbarItem: uploadImage
      imageUploadStatus: true

Problems:

1. This requires uploads to be enabled
2. This is tightly coupled to the uploadImage toolbar item, which does not allow image insertion via URL
3. This always enables the data-entity-type and data-entity-uuid functionality

How to solve?

Discussed how to tackle this in detail with @lauriii and @nod_ on a lengthy call today.

The challenge is that the https://github.com/ckeditor/ckeditor5/pull/11980 PR made possible what we need … with one important exception: there's no way to have a single toolbar item that allows either image uploads or URL-based image insertions. Image uploads are still always enabled.

The plan

That's why we reached the conclusion that we need to create a new plugin that provides a new toolbar item that does not come with this hardcoded behavior. (Ideally this would be implemented upstream, but given the tight deadline we're on, we cannot wait for that. If they implement that at some point, we can transparently switch over!)

The great news is that they made this composable in that same PR! So it's become a lot easier to implement our alternative to uploadImage and imageInsert, that provides the functionality we need — see their ImageInsert for the composition they provide today. Thanks to the work they did on ImageInsertViaUrl to make upload functionality load conditionally, we can now pretty easily build an alternative DrupalImageInsert plugin that composes their functionality slightly differently to get the end result we need!

That should result in CKEditor 5 plugin definitions like this:

ckeditor5_image:
  ckeditor5:
    plugins:
      - image.Image
      - image.ImageToolbar
      - drupalImage.DrupalImage
      - drupalImage.DrupalImageInsert
    config:
      image:
        toolbar: [drupalImageAlternativeText]
  drupal:
    label: Image
    library: ckeditor5/drupal.ckeditor5.image
    elements:
      - <img>
      - <img src alt height width>
    conditions:
      toolbarItem: DrupalImageInsert

ckeditor5_image_url:
  ckeditor5:
    config:
      DrupalImage:
        upload: false
  drupal:
    label: Image URL
    conditions:
      imageUploadStatus: false
      plugins:
        - ckeditor5_image

ckeditor5_image_upload:
  ckeditor5:
    config:
      DrupalImage:
        upload: true
  drupal:
    label: Image Upload
    elements:
      - <img data-entity-uuid data-entity-type>
    conditions:
      imageUploadStatus: true
      plugins:
        - ckeditor5_image

or presented in diff form:

diff --git a/core/modules/ckeditor5/ckeditor5.ckeditor5.yml b/core/modules/ckeditor5/ckeditor5.ckeditor5.yml
index a8115170d7..c5402a9913 100644
--- a/core/modules/ckeditor5/ckeditor5.ckeditor5.yml
+++ b/core/modules/ckeditor5/ckeditor5.ckeditor5.yml
@@ -496,6 +496,7 @@ ckeditor5_image:
       - image.Image
       - image.ImageToolbar
       - drupalImage.DrupalImage
+      - drupalImage.DrupalImageInsert
     config:
       image:
         toolbar: [drupalImageAlternativeText]
@@ -504,10 +505,35 @@ ckeditor5_image:
     library: ckeditor5/drupal.ckeditor5.image
     elements:
       - <img>
-      - <img src alt data-entity-uuid data-entity-type height width>
+      - <img src alt height width>
+    conditions:
+      toolbarItem: DrupalImageInsert
+
+ckeditor5_image_url:
+  ckeditor5:
+    config:
+      DrupalImage:
+        upload: false
+  drupal:
+    label: Image URL
+    conditions:
+      imageUploadStatus: false
+      plugins:
+        - ckeditor5_image
+
+ckeditor5_image_upload:
+  ckeditor5:
+    config:
+      DrupalImage:
+        upload: true
+  drupal:
+    label: Image Upload
+    elements:
+      - <img data-entity-uuid data-entity-type>
     conditions:
-      toolbarItem: uploadImage
       imageUploadStatus: true
+      plugins:
+        - ckeditor5_image
 
 ckeditor5_imageResize:
   ckeditor5:

the config part, still some problems with it. JS part soon

1. +++ b/core/modules/ckeditor5/config/schema/ckeditor5.schema.yml
   @@ -73,6 +73,17 @@ ckeditor5.plugin.ckeditor5_imageResize:
   +# Plugin \Drupal\ckeditor5\Plugin\CKEditor5Plugin\Image
   +ckeditor5.plugin.ckeditor5_image:
   +  type: mapping
   +  label: Image
   +  mapping:
   +    allow_upload:
   +      type: boolean
   +      label: 'Allow upload'
   +      constraints:
   +        NotNull: []
   
   +++ b/core/modules/ckeditor5/src/Plugin/CKEditor5Plugin/Image.php
   @@ -92,6 +68,7 @@ public function validateConfigurationForm(array &$form, FormStateInterface $form
   +    $this->configuration['allow_upload'] = $form_state->getValue('status');
   
   @@ -100,7 +77,7 @@ public function submitConfigurationForm(array &$form, FormStateInterface $form_s
   -    return [];
   +    return ['allow_upload' => FALSE];
   

   🤔 We should not need this; we're using \Drupal\editor\EditorInterface::getImageUploadSettings() in HEAD and should continue to do so.

   See \Drupal\ckeditor5\Plugin\CKEditor5Plugin\ImageUpload::buildConfigurationForm() and \Drupal\ckeditor5\Plugin\CKEditor5Plugin\ImageUpload::submitConfigurationForm().

   Or … why do we actually need this?

   I think this is just confusing the "Drupal plugin" configuration (which would indeed need a config schema!) with the "CKEditor 5 plugin" configuration. 😅

2. +++ b/core/modules/ckeditor5/src/Plugin/CKEditor5Plugin/Image.php
   @@ -54,16 +26,20 @@ class ImageUpload extends CKEditor5PluginDefault implements CKEditor5PluginConfi
   +    if ($editor->getImageUploadSettings()['status'] === TRUE) {
   

   … because here it is correct!

3. +++ b/core/modules/ckeditor5/src/Plugin/Validation/Constraint/ToolbarItemConditionsMetConstraint.php
   @@ -18,13 +18,6 @@
   -  /**
   -   * The violation message when the required image upload status is not set.
   -   *
   -   * @var string
   -   */
   -  public $imageUploadStatusRequiredMessage = 'The %toolbar_item toolbar item requires image uploads to be enabled.';
   -
      /**
       * The violation message when a required filter is missing.
       *
   diff --git a/core/modules/ckeditor5/src/Plugin/Validation/Constraint/ToolbarItemConditionsMetConstraintValidator.php b/core/modules/ckeditor5/src/Plugin/Validation/Constraint/ToolbarItemConditionsMetConstraintValidator.php
   
   diff --git a/core/modules/ckeditor5/src/Plugin/Validation/Constraint/ToolbarItemConditionsMetConstraintValidator.php b/core/modules/ckeditor5/src/Plugin/Validation/Constraint/ToolbarItemConditionsMetConstraintValidator.php
   index a45c427235..fb95187c07 100644
   
   index a45c427235..fb95187c07 100644
   --- a/core/modules/ckeditor5/src/Plugin/Validation/Constraint/ToolbarItemConditionsMetConstraintValidator.php
   
   --- a/core/modules/ckeditor5/src/Plugin/Validation/Constraint/ToolbarItemConditionsMetConstraintValidator.php
   +++ b/core/modules/ckeditor5/src/Plugin/Validation/Constraint/ToolbarItemConditionsMetConstraintValidator.php
   
   +++ b/core/modules/ckeditor5/src/Plugin/Validation/Constraint/ToolbarItemConditionsMetConstraintValidator.php
   +++ b/core/modules/ckeditor5/src/Plugin/Validation/Constraint/ToolbarItemConditionsMetConstraintValidator.php
   @@ -58,13 +58,7 @@ public function validate($toolbar_item, Constraint $constraint) {
   
   @@ -58,13 +58,7 @@ public function validate($toolbar_item, Constraint $constraint) {
              break;
    
            case 'imageUploadStatus':
   -          $image_upload_settings = $text_editor->getImageUploadSettings();
   -          if (!isset($image_upload_settings['status']) || (bool) $image_upload_settings['status'] !== TRUE) {
   -            $this->context->buildViolation($constraint->imageUploadStatusRequiredMessage)
   -              ->setParameter('%toolbar_item', (string) $toolbar_item_label)
   -              ->setInvalidValue($toolbar_item)
   -              ->addViolation();
   -          }
   +          // Nothing to validate.
   

   🤔 I agree nothing in Drupal core would be using this anymore, but shouldn't we keep this around just for contrib/custom modules that may add CKEditor 5 plugins that would require this to be enabled? 😊

triggering bot

Since 100% of the test failures are in the CKEditor 5 module (as they should be!), I'm going to temporarily modify drupalci.yml to only run those. That'll speed up this issue considerably.

Did that in https://git.drupalcode.org/project/drupal/-/merge_requests/2669/diffs?co....

I observed the update test live on DrupalCI — it took way longer than all other Functional tests (including the other update path test!). I just realized we could make the update path tests equally reliable and a hell of a lot faster… by using the bare instead of the filled DB.

It has no effect at all on what we actually test, it just requires far fewer DB operations to set up the test fixture.

On my local machine:

1. CKEditor5UpdateAlignmentTest → from 1 min 12 seconds to 33 seconds (pre-existing update path test in HEAD)
2. CKEditor5UpdateImageToolbarItemTest → from ~10 minutes to 4.3 minutes(the new update path test being added here)

Pushed in https://git.drupalcode.org/project/drupal/-/merge_requests/2669/diffs?co...

Perfect, thanks to @laurii's latest push, the only failing test is now the new update path test! 🥳 Time to push the update path implementation!

Update path test results:

Testing Drupal\Tests\ckeditor5\Functional\Update\CKEditor5UpdateImageToolbarItemTest
.F.F.F..                                                            8 / 8 (100%)

Time: 00:57.988, Memory: 6.00 MB

There were 3 failures:

All 3 failures are due to <img src> no longer being allowed only if image uploads are disabled, which also means the update path no longer needs to add <img src> to the Source Editing configuration (and enable Source Editing if it is not already enabled).

That was originally the intent/plan of this MR, but @lauriii changed that in https://git.drupalcode.org/project/drupal/-/merge_requests/2669/diffs?co... once he realized that this is actually wrong.

It seemed like a great idea this morning between @nod_, @lauriii and I, but then @lauriii discovered it was wrong. That's why @lauriii reverted that change in https://git.drupalcode.org/project/drupal/-/merge_requests/2669/diffs?co....

Why is this wrong? Because when image uploads are enabled, data-entity-type and data-entity-uuid are used by \Drupal\editor\Plugin\Filter\EditorFileReference to generate the correct src. But … src must already be set while editing in CKEditor 5 itself because A) otherwise no image would be visible!, B) a src-less image is invalid anyway, and C) EditorFileReference will only update the src … if it is actually set!

The good news is that this simplifies the update path a fair bit 👍 — but we still need to explicitly enable <img data-entity-uuid data-entity-type> to be creatable & editable using Source editing to avoid breaking BC, so it doesn't simplify things that much.

#57

I think we should still add additional test coverage for the upgrade path of the case where image insert is enabled without image uploads.

That's already taken care of AFAICT?

What permutation is missing from \Drupal\Tests\ckeditor5\Functional\Update\CKEditor5UpdateImageToolbarItemTest::provider()?

Reviewed all non-update path aspects of this MR, marking Needs work for that.

still some things that could be cleaned up in the test classes, but should be good enough. Trying to switch to stark for imageTest.

Addressed @lauriii's feedback from #57/#58/#59 👍

One failure remains:

1) Drupal\Tests\ckeditor5\FunctionalJavascript\ImageTest::testImageCaption
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-''
+''

/var/www/html/vendor/phpunit/phpunit/src/Framework/Constraint/Equality/IsEqual.php:96
/var/www/html/core/modules/ckeditor5/tests/src/FunctionalJavascript/ImageTestBase.php:561

@nod_, do you think you can tackle that one too? 🤓

@lauriii I’m not sure I get the changes in https://git.drupalcode.org/project/drupal/-/merge_requests/2669/diffs?co... — the commit message does not give me enough context to understand the JS changes. Furthermore, it’s adding hundreds of lines to ImageUrlTest, that seems a mistake?

AFAICT @lauriii reviewed my update path contributions here. I addressed his feedback.

I just reviewed the entire MR. No remarks.

I also tested the UX. I disabled image uploads on Basic HTML, and got this UX:

99% of the test coverage is shared thanks to \Drupal\Tests\ckeditor5\FunctionalJavascript\ImageTestBase having been extracted out of \Drupal\Tests\ckeditor5\FunctionalJavascript\ImageTest (which uses/tests uploaded images) — we now just have a new \Drupal\Tests\ckeditor5\FunctionalJavascript\ImageUrlTest (which uses/tests external images).

This is ready! 😊🥳

Down to 1 failure that looks like a simple order expectation mismatch! 👍

I figured out what's going on: why there was a sudden failure and why 22378616 was necessary to make it green again…

… it's because #3271097: Replace CKEditor 4 with CKEditor 5 in the Standard profile and StandardTest landed and changed core/profiles/standard/config/install/editor.editor.basic_html.yml: it now uses CKEditor 5 instead of 4. That same issue copied the previous CKEditor 4-using Text Editor config entities in Standard into core/modules/ckeditor5/tests/fixtures/ckeditor4_config/. We should just have changed that 🤓.

I'll prove this in the sequence of commits I'm about to push.

🚀

Obviously the test failure due to the latest reroll needs to be addressed but I imagine that's going to be simple.
I spent some quality time with this today and haven't found much I'd like different outside of small docs tweaks. Due to the number of files changed, I'd like to have another look Monday before I fully commit/NW the issue, and I assume the test will be addressed by then too.

The fix to that failure is simple: #3270734: Update Editor + CKEditor 5 module to not use CKEditor 4 in tests changed \Drupal\Tests\ckeditor5\FunctionalJavascript\CKEditor5AllowedTagsTest::$defaultElementsWhenUpdatingNotCkeditor5 and \Drupal\Tests\ckeditor5\FunctionalJavascript\CKEditor5AllowedTagsTest::$defaultElementsAfterUpdatingToCkeditor5. In the merge commit, only the first of those two upstream changes was retained, the second was not. So it was an incorrect (tricky!) merge conflict resolution.

Pushed single line fix, back to RTBC 🥳

Unfortunately the 10x test failure isn't addressable by simply updating the fixture to drupal-9.4.0.bare.standard.php.gz. 9.5.x is all set, but I've never heard anyone mention frontporting so probably best to wait for 10x to work.

D10 simply does not have the 9.3 fixtures IIRC. The solution here is indeed to use the 9.4 fixture instead 👍

Maybe somebody wens to do that while I sleep? 😴 😄

Figured out the root cause for #82, after a hunch from @lauriii and initially both of us being completely flabbergasted. We need to do here what core/modules/layout_builder/tests/fixtures/update/layout-builder.php does: we need to mark the existing post-updates as already having been executed.

Thanks for showing the way, #3115503: Support context aware layout plugins! 😊

🤔 Rolling a new 10.0.x patch right now and wondering what we should do: #3290810: Remove updates added prior to 9.4.0 (9.4.4 for ckeditor) and add 9.4.0 database dumps removed the other CKE5 post-update from D10. Shouldn’t we do the same here then?

It kind of makes sense that we don’t add this post-update (nor its test) to 10.0.x actually I think: nobody can have installed a supported version of D10 yet! So there’s literally no point.

@lauriii is checking this with @catch, who was very active on #3290810 👍

In Slack:

catch: @lauriii @wimleers if we remove it, we'll need to regenerate the 9.4 database dump and require updating from the 9.4 patch release that has that update in it. I would commit the post update to both branches then file a follow up to remove it again.
catch: Mainly to avoid reroll hell with module removals, circular dependencies with a 9.4 patch release etc.

Commit dcb2e7fb had two failures:

1. a random one in a Layout Builder functional JS test
2. a non-random one in CKEditor5UpdateImageToolbarItemTest relating to config schema (Schema key olivero.settings:base_primary_color failed with: missing schema).

The first one should disappear.

The second one is for something that was added in #3257274: Implement color changing theme settings for Olivero, months ago. But … that was committed only to Drupal 10. A diff today proves this:

$ git diff 9.5.x 10.0.x -- core/themes/olivero/config/schema
diff --git a/core/themes/olivero/config/schema/olivero.schema.yml b/core/themes/olivero/config/schema/olivero.schema.yml
index 9ae8bbaa62..7723ba3a46 100644
--- a/core/themes/olivero/config/schema/olivero.schema.yml
+++ b/core/themes/olivero/config/schema/olivero.schema.yml
@@ -17,7 +17,10 @@ olivero.settings:
               label: 'Module Link'
     mobile_menu_all_widths:
       type: integer
-      label: "Mobile menu all widths"
+      label: 'Mobile menu all widths'
     site_branding_bg_color:
       type: string
-      label: "Site branding background color"
+      label: 'Site branding background color'
+    base_primary_color:
+      type: color_hex
+      label: 'Base Primary Color'

How is it possible that the 9.4.0 DB fixture contains data that can only be created by code that is specific to 10.0.x?

Specifically, this line in the 9.4 dumps is wrong:

->values(array(
  'collection' => '',
  'name' => 'olivero.settings',
  'data' => 'a:8:{s:5:"_core";a:1:{s:19:"default_config_hash";s:43:"1TswGK46jyu77aIM7Z-0JVQs5bxHmo-gtgrvrQGMXxc";}s:7:"favicon";a:1:{s:11:"use_default";b:1;}s:8:"features";a:4:{s:20:"comment_user_picture";b:1;s:25:"comment_user_verification";b:1;s:7:"favicon";b:1;s:17:"node_user_picture";b:0;}s:4:"logo";a:1:{s:11:"use_default";b:0;}s:20:"third_party_settings";a:1:{s:8:"shortcut";a:1:{s:11:"module_link";b:1;}}s:22:"mobile_menu_all_widths";i:0;s:22:"site_branding_bg_color";s:7:"default";s:18:"base_primary_color";s:7:"#1b9ae4";}',
))

If my understanding is correct, then this patch (which is the plain diff for the MR up to and including dcb2e7fb, with for D10 only the trivial conflict resolved and a new yarn build) should pass on 10.0 but fail on 9.5.

If #86 is correct, then we technically have to update the DB dumps anyway, because they're wrong. So we have two choices AFAICT:

1. Fix the 9.4 dumps in 9.5.x and 10.0.x, which means we might as well omit the post-update hook in D10, contrary to what @catch said in #85. 🙈
2. Create a 9.5.x patch that uses the 9.3 DB dump for the update test, and the 10.0.x patch could use the 9.4 DB dump for the update test.

The latter seems less work.

Thoughts?

I'm going to assume we go with the approach in #87.2, because that's the only way to do what core committer/release manager @catch asked: to not update the 9.4 DB dump.

So:

1. #86's 9.5 patch attached, with one-line interdiff to switch to the 9.3 DB dump
2. #86's 10.0 patch attached, unchanged (just renamed the file), still using the 9.4 DB dump (the 9.3 DB dump does not exist in 10.0.x)

To convince yourself these are 99% identical, I suggest diffing the patches.

The analysis I posted on #86 WRT 9.4 DB dumps being broken seems to be confirmed by @smustgrave over in #3272969: Remove unique constraint on block content info field — see https://drupal.slack.com/archives/C1BMUQ9U6/p1661873272270379.

I agree option #87.2 is fine. In addition to @catch reasonably wanting to not update the dump, this minor change between 10x and 9x both use available fixtures to successfully test what needs to be tested. The difference between the fixtures does not compromise the validity of the test. Plus,the onus of updating those fixtures should not be in this issue's scope.

Committed t0 10.1.x, that got cherry picked to 10.0.x, and a dedicated commit to 9.5.x.

We should still get this committed to 9.4.x.

Will provide a patch. Currently not cleanly cherry-pickable. So determining what the optimal cherry-pick order is …

I agree 9.4.x should get this. Now one of my websites is stuck with CKE4 and CKE5 on other formats, depending if we use images or not

Thanks the backports/cherry-picks that have already happened to 9.4.x in the past 6 days, the 9.5.x patch from #89 now applies cleanly, except for the changes to core/profiles/standard that this made. That makes sense: CKEditor 5 was added to the Standard install profile in 9.5.x, the same won't happen in 9.4.x.

So this patch is the result of:

git apply -3v 3222756-89-9.5.x.patch
git restore --staged core/profiles/standard/
git checkout -- core/profiles/standard
git diff --staged > 3222756-97-9.4.x.patch

Symfony\Component\Yaml\Exception\ParseException: File "core/modules/ckeditor5/tests/fixtures/ckeditor4_config/editor.editor.basic_html.yml" does not exist.

I need to update the 9.5.x tests to point to the Standard install profile config instead of the copies of that config that 9.5.x has. Will do 👍

Well that was apparently just a single line change! 👍

This patch has a huge impact on getting 9.4.x in sync with 9.5.x:

$ git d 9.4.x 9.5.x -- core/modules/ckeditor5 | grep insertions
 58 files changed, 1249 insertions(+), 1258 deletions(-)
$ git apply -3v 3222756-101-9.4.x.patch
$ git d 9.5.x -- core/modules/ckeditor5 | grep insertions
 33 files changed, 376 insertions(+), 423 deletions(-)

25 more files in sync, ~1000 fewer insertions & deletions!

I tested this locally, I'm confident this will be green.

Eh … CI job missing is something I've never seen before :O https://dispatcher.drupalci.org/job/drupal_patches/150532 indeed 404s.

Re-testing…

Yay! Restoring prior state. 👍

Awesome work. When I run #101 in Drupal 9.4.3 throw an error applying the patch.
Patch: https://www.drupal.org/files/issues/2022-09-28/3222756-101-9.4.x.patch

"patches": {
            "drupal/core": {
                "Allow using images from external source": "https://www.drupal.org/files/issues/2022-09-28/3222756-101-9.4.x.patch"
            }

Getting the following message:
"Could not apply patch! Skipping. The error was: Cannot apply patch https://www.drupal.org/files/issues/2022-09-28/3222756-101-9.4.x.patch"
Has anyone else had the same problem?
Or any ideas about it? Thanks
here composer install -vvv, The command couldn't find some files according to that:

    https://www.drupal.org/files/issues/2022-09-28/3222756-101-9.4.x.patch (Allow using images from external source)
Downloading https://www.drupal.org/files/issues/2022-09-28/3222756-101-9.4.x.patch
patch '-p1' --no-backup-if-mismatch -d 'docroot/core' < '/tmp/6356ac34dc674.patch'
Executing command (CWD): patch '-p1' --no-backup-if-mismatch -d 'docroot/core' < '/tmp/6356ac34dc674.patch'
can't find file to patch at input line 5
Perhaps you used the wrong -p or --strip option?
The text leading up to this was:
--------------------------
|diff --git a/core/modules/ckeditor5/ckeditor5.ckeditor5.yml b/core/modules/ckeditor5/ckeditor5.ckeditor5.yml
|index b3ef9da583..afcdbd7a7d 100644
|--- a/core/modules/ckeditor5/ckeditor5.ckeditor5.yml
|+++ b/core/modules/ckeditor5/ckeditor5.ckeditor5.yml
--------------------------
File to patch: 
Skip this patch? [y] 
Skipping patch.

3 out of 3 hunks ignored
can't find file to patch at input line 85
Perhaps you used the wrong -p or --strip option?
The text leading up to this was:
--------------------------
|diff --git a/core/modules/ckeditor5/ckeditor5.libraries.yml b/core/modules/ckeditor5/ckeditor5.libraries.yml
|index 27d2fa3ca3..12f5d7d8ad 100644
|--- a/core/modules/ckeditor5/ckeditor5.libraries.yml
|+++ b/core/modules/ckeditor5/ckeditor5.libraries.yml
--------------------------
File to patch: 
Skip this patch? [y] 
Skipping patch.
3 out of 3 hunks ignored
can't find file to patch at input line 116
Perhaps you used the wrong -p or --strip option?
The text leading up to this was:
--------------------------
|diff --git a/core/modules/ckeditor5/ckeditor5.post_update.php b/core/modules/ckeditor5/ckeditor5.post_update.php
|index bb82debc76..1e56e029c1 100644
|--- a/core/modules/ckeditor5/ckeditor5.post_update.php
|+++ b/core/modules/ckeditor5/ckeditor5.post_update.php
--------------------------
File to patch: 
Skip this patch? [y] 
Skipping patch.
2 out of 2 hunks ignored

patching file core/modules/ckeditor5/css/image.admin.css

The next patch would delete the file core/modules/ckeditor5/css/imageupload.admin.css,
which does not exist!  Assume -R? [n] 
Apply anyway? [n] 
Skipping patch.

1 out of 1 hunk ignored

Cannot rename file without two valid file names

can't find file to patch at input line 203
Perhaps you used the wrong -p or --strip option?
The text leading up to this was:
--------------------------
|diff --git a/core/modules/ckeditor5/js/build/drupalImage.js b/core/modules/ckeditor5/js/build/drupalImage.js
|index e1efb4cf08..63b5661f61 100644
|--- a/core/modules/ckeditor5/js/build/drupalImage.js
|+++ b/core/modules/ckeditor5/js/build/drupalImage.js
--------------------------
File to patch: 
Skip this patch? [y] 
Skipping patch.
...

It's a large list of errors, then I just put some of them here.
@win-leers