Add support for Ckeditor 5

It seems to me that for this module to be CKE5-compatible, the video_embed_wysiwyg submodule should declare a CKE5 plugin, which it does not.

See https://www.drupal.org/docs/core-modules-and-themes/core-modules/ckedito... →

I'm on it.

I'm done for today, will continue later if no one does in the meanwhile.

Can not be applied on 8.x-2.5 version.

Hi vuil and thank you for your feedback.

Yes, the patch is against 8.x-2.x-dev. Feel free to submit a patch for the last stable version for composer use, but if I'm right, to be accepted/committed/merged, a patch has to be against the dev version.

Tested patch and, unfortunately, it gives an JS error when switching between filter formats and/or when CKEditor enabled field is added via AJAX

Setting it to NW

While testing this patch, I could not save the text editor and add video_embed_wysiwyg because it would error out saying height and weight was required and had an empty value. Please review.

Tested and patch is working. Moving to needs review.

There is a checkbox under "Enabled filters" called "Video Embed WYSIWYG". The Video Embed button appears in the "Available Buttons" list even if you don't have this checked. Is this on purpose?

Hmm. This appears to be a thing that happens with all of the embedded elements, not just the video. Very well. This seems to be working within that particular constraint of the editor in general.

+1 to this MR. Allowed me to upgrade to ckeditor5.

Would love to see this get committed as technically if you are using the video_embed_wysiwyg module, you can't migrate to ckeditor5 due to the dependency on ckeditor.

Correct @ThomWilhelm. The only way is to use the issue fork which is a bit of a security risk.

https://www.drupal.org/docs/develop/git/using-gitlab-to-contribute-to-dr... →

Hi.

Sorry but just wanted to ask, as I am in need of this fix as I need to upgrade upgrade my website to CKEditor5 and can't as we use this module.
Can I ask what the next step here is please ?
BTW you guys rock and I appreciate all you do here.
I have been following this issue for about 4 months in the hope.

@jacqui1811 you can use this version for video_embed_field in composer.json:

"drupal/video_embed_field": "dev-3311063-add-support-for",

But you need to do this in your repositories section:

    "repositories": {
        "drupal": {
            "type": "composer",
            "url": "https://packages.drupal.org/8",
            "exclude": [
                "drupal/video_embed_field"
            ]
        },
        "drupal/video_embed_field": {
            "type": "git",
            "url": "https://git.drupalcode.org/issue/video_embed_field-3311063.git"
        }
    },

Just know that there are some inherent security risks when using WIP dev branches.

Thanks for that,
Will try that tomorrow and report back by editing this comment.

My latest commit removes the composer requirement for ckeditor and adds ckeditor5 as a dependency on the wysiwyg module.

Do note that if you are using blt drupal:sync:default:db during your remote ci run, you will actually need to keep drupal/ckeditor required in the codebase until the next release. I recommend just doing one release with the dependency, then a quck bugfix release with the dependency removed after that is deployed. Ref: https://github.com/acquia/blt/issues/4687

We have noticed that, if you have multiple video embeds in a WYSIWYG, it does not render the videos; it just displays the CK5 markdown instead.

One video:

Two videos:

I was hoping this issue would solve the problem, but I applied the patch and it does not: https://www.drupal.org/project/video_embed_field/issues/3371353 🐛 VideoEmbedWysiwyg::getValidMatches() does not detect two videos next to each other Needs review

Just a note that the other issue mentioned above ( https://www.drupal.org/project/video_embed_field/issues/3371353 🐛 VideoEmbedWysiwyg::getValidMatches() does not detect two videos next to each other Needs review ) actually does fix the rendering of multiple videos in one field, at least for me. I did not apply the patch, I just manually replaced that one line after applying the work in this issue.

For those that prefer a patch for the time being: 3311063-35.patch →
Applies against current stable version 2.x.
Might enable easier test feedback.

Works for me.

Applied patch and it allows me to move to CK Editor five and embed videos into the WYSIWYG.

Patch #36 also worked for me. Drupal 9.5.11, CKEditor 5, PHP 8.1, Video Embed Field 8.x-2.5.
Test video embed also worked.

Regarding #3311063-34: Add support for Ckeditor 5 → : I created a patch over at #3371353-5: VideoEmbedWysiwyg::getValidMatches() does not detect two videos next to each other → .
@loopy1492: Can you please try to apply both patches and report whether that fixes it for you? It's not really related to this issue at hand, but I'd like to get rid of this negative feedback here so we can push this (and the other ticket) to RTBC.

#38 confirmed patch from #36 does work
Let's push it to release

+1 RTBC
Tested in Drupal 10.0, YMCA Website Services Distribution https://github.com/YCloudYUSA/yusaopeny/pull/127

Let's merge and release

I get the following error when switching from CKEditor 4 to 5:

Drupal\Component\Plugin\Exception\InvalidPluginDefinitionException: The "video_embed_wysiwyg_video_embed" CKEditor 5 plugin definition is configurable, has non-empty default configuration but has no config schema. Config schema is required for validation. in Drupal\ckeditor5\Plugin\CKEditor5PluginDefinition->validateDrupalAspects()

This is because video_embed_wysiwyg.schema.yml has incorrect indenting for the CKEditor 5 plugin part. I fixed this in the MR.

I can confirm that 8.x-2.5 patched (by composer) with patch made from #42 allowed me to switch to CKEditor 5 smoothly without any problems (Drupal 9.5.11).

Patch for #42 attached if anyone needs it before this got release.

I can also confirm that 3311063-42.patch worked.

I'm using patch 42, but something is off:

In /admin/config/content/formats/manage/full_html I have the Autoplay - Autoplay the videos for users without the "never autoplay videos" permission. Roles with this permission will bypass this setting. set to Off.

When I edit a ndoe and add a video it has Autoplay marked as On, ant it should be off:

I can confirm @gwvoigt → 's observation in comment #46 ✨ Add support for Ckeditor 5 Needs review . I tested patch 42 on a vanilla Drupal 9.5.11 site (PHP 8.1). I observed the same, the difference in the text format config versus when you edit a node and attempt to add a video.

I believe the issue raised by @gwvoigt is not actually related with the CKEDITOR5 compatibility, rather it should be open in a separate issue queue for the logical issue.
Please share the issue link if it has opened already.

Thanks

#44 (i.e. the patch made from #42) works for me in 8.x-2.5 in D10.2.1.

Re #46 & #47, in the setting Autoplay the videos for users without the "never autoplay videos" permission. Roles with this permission will bypass this setting, I think users refers to users viewing the page, and not to users creating or editing the page. As I understand it, it is not a setting that does something like Allow permitted users to enable Autoplay on videos they embed. So I don't see a problem.

Dylan Donkersgoed → made their first commit to this issue’s fork.

Looking at the code it does appear to be intended that the autoplay setting from the WYSIWYG settings should be applied in the modal. It's trying to load default settings from there but they always turn out blank because it has the wrong plugin key ("video_embed", probably corresponding to the CKE4 plugin, instead of "video_embed_wysiwyg_video_embed"):

        $plugin_settings = NestedArray::getValue($editor_settings, [
          'plugins',
          // This should be video_embed_wysiwyg_video_embed for CKE5.
          'video_embed',
          'defaults',
          'children',
        ]);
      $settings = $plugin_settings ? $plugin_settings : [];

I've updated VideoEmbedDialog.php to check for both in the MR and attached the corresponding patch.

@dylan-donkersgoed Re #51, please update "$this->render" to "$this->renderer" in modules/video_embed_wysiwyg/src/Form/VideoEmbedDialog.php.

This is error what I got:
Deprecated function: Creation of dynamic property Drupal\video_embed_wysiwyg\Form\VideoEmbedDialog::$render is deprecated in Drupal\video_embed_wysiwyg\Form\VideoEmbedDialog->__construct() (line 51 of /var/www/html/docroot/modules/contrib/video_embed_field/modules/video_embed_wysiwyg/src/Form/VideoEmbedDialog.php)

❯ interdiff 6.diff 3311063-50.patch
❯ echo $?
0

Patch 6 is indeed equivalent with the current state of the MR, hiding the patch as (IMO) it creates additional noise in the issue.

@dylan-donkersgoed Re #51, please update "$this->render" to "$this->renderer" in modules/video_embed_wysiwyg/src/Form/VideoEmbedDialog.php.

Fixed in MR#6 by using the WebIDE, however my eyes and CTRL+F did not find any usage of $this->renderer... maybe that is a dad code?

I've noticed an issue related to the one described in https://www.drupal.org/project/video_embed_field/issues/3311063#comment-... ✨ Add support for Ckeditor 5 Needs review where videos will render raw JSON. This is also related to the regex, but in this case seems to happen when there is an earlier { character before the preview_thumbnail property. In my case this was due to data-entity-embed-display-settings="{ in a <drupal-entity> tag but it probably could happen in other situations as well. Could be the cause of the issues a couple other users reported.

To fix this I pushed up a small change to the regex to make the opening

tag required. From what I can tell in the code for both the CKE4 and CKE5 plugins videos should always be wrapped in a paragraph so I believe this should be safe, but it would be nice to have some additional review/confirmation.

Updated patch is attached.

I tested 3311063-56.patch on 10.2.4 - working for me.

I need this! Please add this to the next version. Thank you!

Also tested patch #56 and works well with ckeditor5

Why a new patch was uploaded instead of contributing changes to the already open MR#6? It is one click requesting access and adding changes to it...

patch #56 works fine with ckeditor5 (D10.2.6)

rokkun88 → changed the visibility of the branch 3311063-add-support-for to hidden.

mxr576
Why a new patch was uploaded instead of contributing changes to the already open MR#6? It is one click requesting access and adding changes to it...

rokkun88 changed the visibility of the branch 3311063-add-support-for to hidden.

Well, I would have rather incorporated new changes to MR that makes code reviews and rebasing much easier then a patch... but maybe that is only my opinion.