Add support for Ckeditor 5 (get Video Embed Wysiwyg back)

Is the 8.x.2.5 release is compatible with ckeditor 5 ?

did a quick test with a patch downloaded from the MR above, but get this:

patching file 'modules/video_embed_media/src/Form/VideoEmbedFieldForm.php'
patching file 'modules/video_embed_media/src/Plugin/media/Source/VideoEmbedField.php'
patching file 'modules/video_embed_media/src/Plugin/media/Source/VideoEmbedFieldInterface.php'
patching file 'modules/video_embed_media/video_embed_media.module'
patching file 'modules/video_embed_media/src/Form/VideoEmbedFieldForm.php'
patching file 'modules/video_embed_wysiwyg/src/Plugin/Filter/VideoEmbedWysiwyg.php'
patching file 'modules/video_embed_wysiwyg/video_embed_wysiwyg.module'
1 out of 2 hunks failed--saving rejects to 'modules/video_embed_wysiwyg/video_embed_wysiwyg.module.rej'

➜  video_embed_field git:(feat/ck5) ✗ cat modules/video_embed_wysiwyg/video_embed_wysiwyg.module.rej
@@ -33,6 +33,7 @@
   $filter_enabled = !empty($form_state->getValue(['filters', 'video_embed_wysiwyg', 'status']));
   $button_enabled = FALSE;
   $button_rows = json_decode($form_state->getValue(['editor', 'settings', 'toolbar', 'button_groups']), TRUE);
+
   if (!empty($button_rows)) {
     foreach ($button_rows as $button_row) {
       foreach ($button_row as $button_group) {
➜  video_embed_field git:(feat/ck5) ✗ 

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 add the video_embed_wysiwyg to the text editor, it would error out saying height and weight were required fields so I've added a default value to the patch.
Please review.

Tested and patch is working. Moving to needs review.

When I check "Autoplay", or "Responsive Video", then go back to the content item, click the video, then click to edit the existing field, those elements are not checked.

When testing this, do note that Autoplay is disabled by default for Administrators.

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 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 drupal/ckeditor and adds drupal: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

I have switched this from RBTC to in progress.

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

NOTE

This happens on a field-by-field basis. If you have two WYSIWYG fields on the same content item, and each field only has one embedded video, they render just fine. But when you try to embed more than one video per field, they stop rendering.

Just a note that the other issue mentioned above (https://www.drupal.org/project/video_embed_field/issues/3371353) 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.0.
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 (get Video Embed Wysiwyg back): 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. 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.

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-... 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)

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.

Drupal: 10.2.6
PHP: 8.2.2
MariaDB: 10.5.18
Web Server: nginx
ckeditor5: 10.2.6
video_embed_wysiwyg: 8.x-2.5

@dylan-donkersgoed thank you very much.
Patch #56 works fine.

This patch worked to relieve the ajax error when switching the dropdown selection to ckeditor5.
But upon saving, there is a set of save-blocking errors, ie, 'image_style_large' is not a supported key.
(Related: https://www.drupal.org/project/inline_responsive_images/issues/3460644)

Can we get this merged as #3465381: Drupal 11 compatibility for Video Embed Field is blocked by this. For Drupla 11 support this is critical.

Tests also needs to be updated as per CKEditor5, will leave it up to maintainers.

will work on test failure with CKE5.

Can we get this merged, as it's blocker for #3465381: Drupal 11 compatibility for Video Embed Field.

Hi everyone. Can you please tell me if this is going to be available for D10 / 11 ?

Hi, could this be rebased on the 3.0.x Drupal 11 compatible branch which is currently missing the video_embed_wysiwyg module? Thanks.

Patch #56 still applies cleanly to 2.x, but doesn't apply to 3.x.

Hi everyone, both the patch from #56 and the latest merge diff fail against 2.xdev on Drupal 10.3.9

Patch from #56:

sudo -u www-data patch -p1 < 3311063-56.patch
patching file composer.json
patching file modules/video_embed_media/src/Form/VideoEmbedFieldForm.php
patching file modules/video_embed_media/src/Plugin/media/Source/VideoEmbedField.php
Hunk #1 FAILED at 5.
Hunk #2 succeeded at 18 (offset -5 lines).
Hunk #3 succeeded at 47 (offset -37 lines).
Hunk #4 succeeded at 105 (offset -37 lines).
Hunk #5 succeeded at 144 (offset -37 lines).
Hunk #6 FAILED at 191.
2 out of 6 hunks FAILED -- saving rejects to file modules/video_embed_media/src/Plugin/media/Source/VideoEmbedField.php.rej
patching file modules/video_embed_media/src/Plugin/media/Source/VideoEmbedFieldInterface.php
patching file modules/video_embed_media/video_embed_media.module
patching file modules/video_embed_wysiwyg/.gitignore
patching file modules/video_embed_wysiwyg/config/schema/video_embed_wysiwyg.schema.yml
patching file modules/video_embed_wysiwyg/css/video_embed.admin.css
patching file modules/video_embed_wysiwyg/icons/film.svg
patching file modules/video_embed_wysiwyg/icons/play-circle.svg
patching file modules/video_embed_wysiwyg/icons/video.svg
patching file modules/video_embed_wysiwyg/js/build/README.txt
patching file modules/video_embed_wysiwyg/js/build/videoEmbed.js
patching file modules/video_embed_wysiwyg/js/ckeditor5_plugins/videoEmbed/src/index.js
patching file modules/video_embed_wysiwyg/js/ckeditor5_plugins/videoEmbed/src/insertvideoembedcommand.js
patching file modules/video_embed_wysiwyg/js/ckeditor5_plugins/videoEmbed/src/videoembed.js
patching file modules/video_embed_wysiwyg/js/ckeditor5_plugins/videoEmbed/src/videoembedediting.js
patching file modules/video_embed_wysiwyg/js/ckeditor5_plugins/videoEmbed/src/videoembedui.js
patching file modules/video_embed_wysiwyg/package.json
patching file modules/video_embed_wysiwyg/plugin/README.md
patching file modules/video_embed_wysiwyg/src/Form/VideoEmbedDialog.php
Hunk #1 FAILED at 48.
Hunk #2 succeeded at 85 (offset 3 lines).
1 out of 2 hunks FAILED -- saving rejects to file modules/video_embed_wysiwyg/src/Form/VideoEmbedDialog.php.rej
patching file modules/video_embed_wysiwyg/src/Plugin/CKEditor4To5Upgrade/VideoEmbed.php
patching file modules/video_embed_wysiwyg/src/Plugin/CKEditor5Plugin/VideoEmbedWysiwyg.php
patching file modules/video_embed_wysiwyg/src/Plugin/CKEditorPlugin/VideoEmbedWysiwyg.php
Hunk #1 succeeded at 14 with fuzz 1 (offset 3 lines).
patching file modules/video_embed_wysiwyg/src/Plugin/Filter/VideoEmbedWysiwyg.php
patching file modules/video_embed_wysiwyg/video_embed_wysiwyg.ckeditor5.yml
patching file modules/video_embed_wysiwyg/video_embed_wysiwyg.info.yml
patching file modules/video_embed_wysiwyg/video_embed_wysiwyg.libraries.yml
patching file modules/video_embed_wysiwyg/video_embed_wysiwyg.module
Hunk #1 succeeded at 13 (offset -2 lines).
Hunk #2 succeeded at 27 (offset -2 lines).
Hunk #3 FAILED at 50.
1 out of 3 hunks FAILED -- saving rejects to file modules/video_embed_wysiwyg/video_embed_wysiwyg.module.rej
patching file modules/video_embed_wysiwyg/webpack.config.js
patching file modules/video_embed_wysiwyg/yarn.lock

Latest merge request plain diff:

sudo -u www-da            ta patch -p1 < 6.diff
The next patch would create the file .gitlab-ci.yml,
which already exists!  Assume -R? [n] n
Apply anyway? [n] y
patching file .gitlab-ci.yml
Hunk #1 FAILED at 1.
1 out of 1 hunk FAILED -- saving rejects to file .gitlab-ci.yml.rej
patching file composer.json
patching file modules/video_embed_media/modules/vem_migrate_oembed/tests/src/Fun            ctional/oEmbedUpdateTest.php
Reversed (or previously applied) patch detected!  Assume -R? [n] n
Apply anyway? [n] n
Skipping patch.
2 out of 2 hunks ignored -- saving rejects to file modules/video_embed_media/mod            ules/vem_migrate_oembed/tests/src/Functional/oEmbedUpdateTest.php.rej
patching file modules/video_embed_media/src/Form/VideoEmbedFieldForm.php
patching file modules/video_embed_media/src/Plugin/media/Source/VideoEmbedField.            php
Hunk #1 FAILED at 5.
Hunk #2 succeeded at 18 (offset -5 lines).
Hunk #3 succeeded at 47 (offset -37 lines).
Hunk #4 succeeded at 105 (offset -37 lines).
Hunk #5 succeeded at 144 (offset -37 lines).
Hunk #6 FAILED at 191.
2 out of 6 hunks FAILED -- saving rejects to file modules/video_embed_media/src/            Plugin/media/Source/VideoEmbedField.php.rej
patching file modules/video_embed_media/src/Plugin/media/Source/VideoEmbedFieldI            nterface.php
patching file modules/video_embed_media/video_embed_media.module
patching file modules/video_embed_wysiwyg/.gitignore
patching file modules/video_embed_wysiwyg/config/schema/video_embed_wysiwyg.sche            ma.yml
patching file modules/video_embed_wysiwyg/css/video_embed.admin.css
patching file modules/video_embed_wysiwyg/icons/film.svg
patching file modules/video_embed_wysiwyg/icons/play-circle.svg
patching file modules/video_embed_wysiwyg/icons/video.svg
patching file modules/video_embed_wysiwyg/js/build/README.txt
patching file modules/video_embed_wysiwyg/js/build/videoEmbed.js
patching file modules/video_embed_wysiwyg/js/ckeditor5_plugins/videoEmbed/src/in            dex.js
patching file modules/video_embed_wysiwyg/js/ckeditor5_plugins/videoEmbed/src/in            sertvideoembedcommand.js
patching file modules/video_embed_wysiwyg/js/ckeditor5_plugins/videoEmbed/src/vi            deoembed.js
patching file modules/video_embed_wysiwyg/js/ckeditor5_plugins/videoEmbed/src/vi            deoembedediting.js
patching file modules/video_embed_wysiwyg/js/ckeditor5_plugins/videoEmbed/src/vi            deoembedui.js
patching file modules/video_embed_wysiwyg/package.json
patching file modules/video_embed_wysiwyg/plugin/README.md
patching file modules/video_embed_wysiwyg/src/Form/VideoEmbedDialog.php
Hunk #1 FAILED at 48.
Hunk #2 succeeded at 85 (offset 3 lines).
1 out of 2 hunks FAILED -- saving rejects to file modules/video_embed_wysiwyg/sr            c/Form/VideoEmbedDialog.php.rej
patching file modules/video_embed_wysiwyg/src/Plugin/CKEditor4To5Upgrade/VideoEm            bed.php
patching file modules/video_embed_wysiwyg/src/Plugin/CKEditor5Plugin/VideoEmbedW            ysiwyg.php
patching file modules/video_embed_wysiwyg/src/Plugin/CKEditorPlugin/VideoEmbedWy            siwyg.php
Hunk #1 succeeded at 14 with fuzz 1 (offset 3 lines).
patching file modules/video_embed_wysiwyg/src/Plugin/Filter/VideoEmbedWysiwyg.ph            p
patching file modules/video_embed_wysiwyg/tests/src/Functional/TextFormatConfigu            rationTest.php
Hunk #4 FAILED at 55.
Hunk #5 FAILED at 69.
2 out of 5 hunks FAILED -- saving rejects to file modules/video_embed_wysiwyg/te            sts/src/Functional/TextFormatConfigurationTest.php.rej
patching file modules/video_embed_wysiwyg/tests/src/FunctionalJavascript/EmbedDi            alogTest.php
Hunk #3 FAILED at 43.
1 out of 6 hunks FAILED -- saving rejects to file modules/video_embed_wysiwyg/te            sts/src/FunctionalJavascript/EmbedDialogTest.php.rej
patching file modules/video_embed_wysiwyg/tests/src/Kernel/FilterTest.php
patching file modules/video_embed_wysiwyg/video_embed_wysiwyg.ckeditor5.yml
patching file modules/video_embed_wysiwyg/video_embed_wysiwyg.info.yml
patching file modules/video_embed_wysiwyg/video_embed_wysiwyg.libraries.yml
patching file modules/video_embed_wysiwyg/video_embed_wysiwyg.module
Hunk #1 succeeded at 13 (offset -2 lines).
Hunk #2 FAILED at 29.
1 out of 2 hunks FAILED -- saving rejects to file modules/video_embed_wysiwyg/vi            deo_embed_wysiwyg.module.rej
patching file modules/video_embed_wysiwyg/webpack.config.js
patching file modules/video_embed_wysiwyg/yarn.lock
patching file src/Plugin/Field/FieldType/VideoEmbedField.php
patching file tests/src/Functional/FieldConfigurationTest.php
Reversed (or previously applied) patch detected!  Assume -R? [n] n
Apply anyway? [n] n
Skipping patch.
2 out of 2 hunks ignored -- saving rejects to file tests/src/Functional/FieldCon            figurationTest.php.rej
patching file video_embed_field.info.yml

Hi all! Please, please, hoping someone can get this working for D10 . / CKEditor 5

Code will have to be updated to fix the following security issue before merging: #3503383: Potential XSS vulnerability in colorbox and lazy load formatters.

#3503383: Potential XSS vulnerability in colorbox and lazy load formatters and #3517880: Broken thumbnail access when the default download method is set to "Private local files served by Drupal." have to be handled before this can land.

Actually #3503383: Potential XSS vulnerability in colorbox and lazy load formatters is unrelated to this issue.

And #3517880: Broken thumbnail access when the default download method is set to "Private local files served by Drupal." is only necessary for this issue to work when default download method is set to private, but it's a separate bug which can be handled separately.

The only thing is making sure the latest one to be merged still works when the previous one has been merged, but it's the case for any issue.

Convert MR to a patch .

Rebased MR to apply it to the new 3.x default branch.

Locally tested successfully.

Could we have another RTBC before I merge it? Thanks.

Works fine here..

It's merged. Thanks, everyone, for your contribution!

New version released: 3.0.0-beta2.

Thanks @mably!

Rebased MR6 for 8.x-2.x version

This is a closed fixed issue, it might be worth opening a new targetted issue for 2.x @alexanderpatricio if you need a backport as sometimes closed fixed get ignored by maintainers.