CKEditor 5 Support

kinda weird to have the embed dependency at all which has the multiple instances thing built-in and then not actually use that, so maybe, maybe removing that might make sense, but that's a fairly big change too.

Having reflected on this architectural question, I would like to make the recommendation that we remove the "middleware" dependency on drupal/embed, and instead, directly call logic from embed/embed.

This change is already performed in the patch in #7, "3316376_cke5-support_7.no-drupal-embed.patch", with passing tests, so the "fairly big change" part of the statement above is, at least, mostly behind us :)

Looking forward to hearing what the maintainers think about this!

As discussed, we need to decide what to do with the plugin, possibly flag as deprecated, with a label/explanation in the UI and remove in an upcoming major version.

That said, the removal of the embed module dependency seems barely related to CkEditor 5 support (it's 90% copy pasting of the code from the embed module that we *do* need), so I'd suggest we split that up into two issues either way, get this in first and then the dependency change becomes easier to review and manage?

I'm not sure when I'll have time to really test this myself, so what would help a lot is people reporting back on whether or not this works for them.

the removal of the embed module dependency seems barely related to CkEditor 5 support (it's 90% copy pasting of the code from the embed module that we *do* need), so I'd suggest we split that up into two issues either way

Great point. I've created 📌 Consider removing dependency on "embed" contrib module Active to capture the thoughts on the removal of the embed module dependency, and agree that CKEditor 5 readiness should not involve that scope.

For folks coming here to test: use the merge request (https://git.drupalcode.org/project/url_embed/-/merge_requests/10), which only includes the CKEditor 5 support and does not remove the dependency on the embed module.

I've added test coverage, which completes the requirements spelled out in the issue description. Per request from maintainers, what would help is for others to test & report whether this is working for them.

Suggested testing

The staged code is at https://git.drupalcode.org/project/url_embed/-/merge_requests/10

For testing steps, this issue's description, specifically: https://www.drupal.org/project/url_embed/issues/3316376#testing ✨ CKEditor 5 Support Fixed

I applied your patch to 2.x-dev running Drupal 9.5.5, with CKEditor 5. And it works, except for Facebook and Instagram, they are throwing the error, below. Which may or may not be related to this issue: https://www.drupal.org/project/url_embed/issues/3272702 📌 Support for Facebook/Instagram API (2022), switch to embed/embed v4 Fixed

(I tested it with Vimeo, Youtube, Twitter, Facebook and Insta.)

Warning: Attempt to read property "html" on null in Drupal\url_embed\Plugin\Filter\UrlEmbedFilter->process() (line 78 of modules/contrib/url_embed/src/Plugin/Filter/UrlEmbedFilter.php).
Drupal\url_embed\Plugin\Filter\UrlEmbedFilter->process('
', 'en') (Line: 118)
Drupal\filter\Element\ProcessedText::preRenderText(Array)
call_user_func_array(Array, Array) (Line: 101)
Drupal\Core\Render\Renderer->doTrustedCallback(Array, Array, 'Render #pre_render callbacks must be methods of a class that implements \Drupal\Core\Security\TrustedCallbackInterface or be an anonymous function. The callback was %s. See https://www.drupal.org/node/2966725 → ', 'exception', 'Drupal\Core\Render\Element\RenderCallbackInterface') (Line: 788)
Drupal\Core\Render\Renderer->doCallback('#pre_render', Array, Array) (Line: 374)
Drupal\Core\Render\Renderer->doRender(Array, 1) (Line: 204)
Drupal\Core\Render\Renderer->render(Array, 1) (Line: 160)
Drupal\Core\Render\Renderer->Drupal\Core\Render\{closure}() (Line: 580)
Drupal\Core\Render\Renderer->executeInRenderContext(Object, Object) (Line: 161)
Drupal\Core\Render\Renderer->renderPlain(Array) (Line: 91)
Drupal\embed\Controller\EmbedController->preview(Object, Object)
call_user_func_array(Array, Array) (Line: 123)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 580)
Drupal\Core\Render\Renderer->executeInRenderContext(Object, Object) (Line: 124)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->wrapControllerExecutionInRenderContext(Array, Array) (Line: 97)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 169)
Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object, 1) (Line: 81)
Symfony\Component\HttpKernel\HttpKernel->handle(Object, 1, 1) (Line: 58)
Drupal\Core\StackMiddleware\Session->handle(Object, 1, 1) (Line: 48)
Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object, 1, 1) (Line: 106)
Drupal\page_cache\StackMiddleware\PageCache->pass(Object, 1, 1) (Line: 85)
Drupal\page_cache\StackMiddleware\PageCache->handle(Object, 1, 1) (Line: 50)
Drupal\ban\BanMiddleware->handle(Object, 1, 1) (Line: 48)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object, 1, 1) (Line: 51)
Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object, 1, 1) (Line: 23)
Stack\StackedHttpKernel->handle(Object, 1, 1) (Line: 718)
Drupal\Core\DrupalKernel->handle(Object) (Line: 19)

re: #19

I'm a goof, I did not know you needed an API key for Facebook/Instagram. I did not have one when I tested, and I am fairly certain that is probably why it failed.

In a perfect world, an exception might be helpful there.

(Sorry we just don't use them, I just thought I'd try them. I need it for Youtube and Vimeo.)

So someone who has those API credentials should test those. Otherwise it worked great for me.

Thanks.

Thanks for testing! Regarding the Facebook API requirement, in a large sense, that is out of scope of this issue. The URL Embed module consists of a CKEditor plugin that provides a UI for inserting URLs and a Drupal text filter for rendering them. This issue's scope only regards the former (the UI for inserting links); nothing in the latter (the rendering of the links) has changed here.

My organization has been using the latest patch on multiple sites in production, without incident, for over a week, so I think this patch is good to go.

Confirming also that the latest patch was used on multiple sites in production without issues, so I also agree that this patch is good to go.

Yet another confirmation that the patch in the merge request works as intended. Thanks!

Hi,

I wanted to provide an update regarding the patch I received from the merge request and my attempt to apply it. Unfortunately, the patch did not produce the expected results. Here is what I did:

1. Updated the contrib module to version 2.x.
2. Added the patch to the composer.json file.
3. Made an attempt to apply the patch.

Also, I found an error in the output:
"Could not apply patch! Skipping. The error was: Cannot apply patch."

I would appreciate any help in this regard

Made an attempt to apply the patch.

Can you confirm that your syntax in the composer.json file was the following?

      "drupal/url_embed": {
        "CKEditor 5 Support": "https://git.drupalcode.org/project/url_embed/-/merge_requests/10.diff",
      },

If so, are you trying to apply any *other* patches to url_embed simultaneously?

Note: in practice, patches should not be directly referenced from Gitlab, as shown in the example above, since they could potentially be changed by anyone with a drupal.org account. Ideally, you should download the contents of a patchfile and host it directly in your own code.

No, I didn't use that syntax. Instead, I used the following syntax:

            "drupal/url_embed": {
                "CKEditor 5 Support": "./patches/10.patch"
            }

Hrm. Nothing has been committed to the 2.x branch since that patch was created, so that's not the problem. My guess is that there's something problematic in the contents of the patchfile that you have locally. Maybe you could test the syntax shown in #26, above, and if that works, that would help confirm that the issue is with the local patch file?

I used the syntax in #26 and it worked properly, but I'm curious as to why the patch file is not functioning as expected. Just to clarify, I downloaded the patch file from the merge request and copied it into my project. Is there anything else that needs to be done in order to apply the patch file successfully?

@mark_fullmer Great news! I successfully applied the patch, and I'm sharing it here just in case anyone else might find it useful

I applied the patch from #30. Patch applies cleanly but when i set text format as CK5 i still get: The CKEditor 4 button url does not have a known upgrade path.

I am pretty sure that "url" button is this module; is it not?

@liquidcms

I am indeed also experiencing the same issue. Not only would you get that, but also adding your own embed buttons through admin/config/content/embed on the type url is not possible, as they do not show in the toolbar.

adding your own embed buttons through admin/config/content/embed on the type url is not possible, as they do not show in the toolbar.

I see in the topic started thread both have been chosen on purpose.

That's correct. For others coming here, the section "Architecture change callout" in this issue's description explains the rationale for removing the ability to add additional embed buttons.

I am just coming here as I have used this plugin with wysiwyg module and ckeditor 4.

Actually I have migrated to ckeditor 5, however where I had previously added via url_embed module are not viewable in ckeditor.

My guess is that these will be stripped out if I try to save these nodes?

I have tried this patch url_embed_ckeditor5_support.patch and could not notice any change here? (btw this is one of the biggest patches I have ever applied)

I propose simplifying the implementation to be designed around a single button.

Probably should be a separate issue to this one.

All I am really interested in is how to get this working with ckeditor 5. What is the change here required to achieve that?

Ok, I have discovered the iframe and iframe_media module. Works with CK editor like any other media type entity.

https://www.drupal.org/project/media_iframe →

Recently we are updating to Drupal CKEditor 5, drupal core media uses oembed, but only has Vimeo and youtube.

But there is a contrib module that allows you to choose more providers https://www.drupal.org/project/oembed_providers →

Should we keep the URL Embed if the Media Core already provides it?

Maybe we should provide a migrator.

As of patch https://www.drupal.org/project/embed/issues/3310328 📌 When CKEditor5 is installed, only allow SVGs for button icons Fixed in Embed module all buttons require and SVG icon to be displayed.

I have amended the patch to use CKEditor5 icon.

I humbly submit this svg icon I made for the button.
Note: I had to wrap it in a zip file to upload.. just extract and review.

Helps if I upload the zip too.

Finally got around to properly test this on Drupal 10 with CKEditor5.

This seems to work well enough and I'm going ahead and committing it, we can make further adjustments later on if necessary.

@Spurlos: Can you create a new issue for that? If you use ckeditor 5 then you no longer need an embed button at all, that would only be when still using CKEditor4 and therefore unrelated to this issue.

@emptyvoid: I think I prefer the current svg icon in the project, feel free to create a new issue and propose that, if others like it too then I'm open to changing it.

@mark_fullmer: Do I understand correctly that if we remove the ckeditor4 integration, this module wouldn't depend on embed anymore at all and we could do a 3.x release (maybe, in the future) that removes both that plugin and the embed.module dependency?

Do I understand correctly that if we remove the ckeditor4 integration, this module wouldn't depend on embed anymore at all and we could do a 3.x release (maybe, in the future) that removes both that plugin and the embed.module dependency

That's correct, with the clarification that it's not *just* removal: we would also need to do some copy-paste of some helper methods in the embed module into url_embed, as mentioned in the issue description in 📌 Consider removing dependency on "embed" contrib module Active .

Removing CKEditor 4 support and the embed module dependency in a new major version (3.x) sounds like the right way to handle this transition seamlessly.

@mark_fullmer please note that MR10 does not contain the svg fix that #37 has. Not sure if that is intentional, but I was assured by frontend team that it is vital. Which means this issue isn't completely fixed yet. We're currently applying this patch on alpha2.

It is my understanding that this is only the CKEditor 4 integration and unused for CKEditor5 and that's why I suggested doing that as it's own issue. If you migrated to CKEditor5 you can remove that embed configuration AFAIK.

Even if not, at this point it should be filled in a new issue.

Automatically closed - issue fixed for 2 weeks with no activity.