Automated Drupal 11 compatibility fixes for embed

This is an automated patch generated using Upgrade Status and Drupal Rector. Please see the issue summary for more details. A merge request is also openend and updated.

It is important that any automated tests available are run and that you manually test the changes.

Drupal 11 Compatibility

According to the Upgrade Status module → these changes make this module compatible with Drupal 11! 🎉
Therefore these changes update the info.yml file for Drupal 11 compatibility.

Leaving this issue open, even after committing the current patch, will allow the Project Update Bot → to post additional Drupal 11 compatibility fixes as they become available in Drupal Rector.

Debug info

This patch was created using these packages:

1. drupal/upgrade_status: 4.1.0
2. mglaman/phpstan-drupal: 1.2.7
3. palantirnet/drupal-rector: 0.20.1

This is an automated patch generated using Upgrade Status and Drupal Rector. Please see the issue summary for more details. A merge request (MR) is also openend and updated.

It is important that any automated tests available are run and that you manually test the changes.

Drupal 11 Compatibility

According to the Upgrade Status module → , even with these changes, this module is not yet compatible with Drupal 11.

Currently Drupal Rector, version 0.20.1, cannot fix all Drupal 11 compatibility problems.

Therefore, these changes did not update the info.yml file for Drupal 11 compatibility.

The compatibility issues that Upgrade Status found after the Drupal Rector fixes were applied are attached to help you resolve them manually.

Leaving this issue open, even after committing the current patch or merging the MR, will allow the Project Update Bot → to post additional Drupal 11 compatibility fixes as they become available in Drupal Rector.

Debug information

These packages were used to generate the fixes:

1. drupal/upgrade_status: 4.3.1
2. mglaman/phpstan-drupal: 1.2.11
3. palantirnet/drupal-rector: 0.20.1

Hello @dave-reid - I have a suggestion on the new release for embed module and was thinking if there is any plan to drop Drupal 9 support as it has ckeditor4 dependency which will not be supported in Drupal 11. Would it be ok if i create an MR with dropped support for Drupal 9?

This is an automated patch generated using Upgrade Status and Drupal Rector. Please see the issue summary for more details. A merge request (MR) is also openend and updated.

It is important that any automated tests available are run and that you manually test the changes.

Drupal 11 Compatibility

According to the Upgrade Status module → these changes make this module compatible with Drupal 11! 🎉
Therefore these changes update the info.yml file for Drupal 11 compatibility.

Leaving this issue open, even after committing the current patch or merging the MR, will allow the Project Update Bot → to post additional Drupal 11 compatibility fixes as they become available in Drupal Rector.

Debug information

These packages were used to generate the fixes:

1. drupal/upgrade_status: 4.3.1
2. mglaman/phpstan-drupal: 1.2.11
3. palantirnet/drupal-rector: 0.20.1

This is an automated patch generated using Upgrade Status and Drupal Rector. Please see the issue summary for more details. A merge request (MR) is also openend and updated.

It is important that any automated tests available are run and that you manually test the changes.

Drupal 11 Compatibility

According to the Upgrade Status module → these changes make this module compatible with Drupal 11! 🎉
Therefore these changes update the info.yml file for Drupal 11 compatibility.

Leaving this issue open, even after committing the current patch or merging the MR, will allow the Project Update Bot → to post additional Drupal 11 compatibility fixes as they become available in Drupal Rector.

Debug information

These packages were used to generate the fixes:

1. drupal/upgrade_status: 4.3.2
2. mglaman/phpstan-drupal: 1.2.11
3. palantirnet/drupal-rector: 0.20.2

This is an automated patch generated using Upgrade Status and Drupal Rector. Please see the issue summary for more details. A merge request (MR) is also openend and updated.

It is important that any automated tests available are run and that you manually test the changes.

Drupal 11 Compatibility

According to the Upgrade Status module → these changes make this module compatible with Drupal 11! 🎉
Therefore these changes update the info.yml file for Drupal 11 compatibility.

Leaving this issue open, even after committing the current patch or merging the MR, will allow the Project Update Bot → to post additional Drupal 11 compatibility fixes as they become available in Drupal Rector.

Debug information

These packages were used to generate the fixes:

1. drupal/upgrade_status: 4.3.2
2. mglaman/phpstan-drupal: 1.2.11
3. palantirnet/drupal-rector: 0.20.2

@deepakkm - I verified MR 16 and can see following deprecations, hence needs work.

================================================================================
Embed, --
Scanned on Wed, 06/05/2024 - 06:23

FILE: modules/contrib/embed/src/Form/EmbedButtonForm.php

STATUS         LINE                           MESSAGE                           
--------------------------------------------------------------------------------
Check manually 117  Call to deprecated function watchdog_exception(). Deprecated
                    in drupal:10.1.0 and is removed from drupal:11.0.0. Use Use 
                    Drupal\Core\Utility\Error::logException() instead.          
--------------------------------------------------------------------------------

FILE: modules/contrib/embed/tests/src/Unit/DomHelperTraitTest.php

STATUS         LINE                           MESSAGE                           
--------------------------------------------------------------------------------
Check manually 35   Missing call to parent::setUp() method.                     
--------------------------------------------------------------------------------

I am working on this issue

Remaining phpcs and phpstan issue should be fixed as part of separate ticket. I think MR 16 is enough for drupal 11 comptability

Requested some changes in MR. Please see. Hence, moving it back.

Verified changes with Drupal 11 rc version and working fine.

ankitv18 → changed the visibility of the branch project-update-bot-only to hidden.

Changes done in the MR!16 looks fine by me, hence marking this RTBC.
Also I'm linking the other issues.

This is an automated patch generated using Upgrade Status and Drupal Rector. Please see the issue summary for more details. A merge request (MR) is also openend and updated.

It is important that any automated tests available are run and that you manually test the changes.

Drupal 11 Compatibility

According to the Upgrade Status module → these changes make this module compatible with Drupal 11! 🎉
Therefore these changes update the info.yml file for Drupal 11 compatibility.

Leaving this issue open, even after committing the current patch or merging the MR, will allow the Project Update Bot → to post additional Drupal 11 compatibility fixes as they become available in Drupal Rector.

Debug information

These packages were used to generate the fixes:

1. drupal/upgrade_status: 4.3.2
2. mglaman/phpstan-drupal: 1.2.11
3. palantirnet/drupal-rector: 0.20.3

MR !16 is D11 compatible

Left some feedback.

As per discussion we are going to refactor functionaljavascript test for ckeditor and change it to ckeditor5.

I have reviewed the changes, and everything looks good to me, also validated on local.

A couple of questions, and I see that most of the "validate" stage of the build pipeline is still failing: https://git.drupalcode.org/project/embed/-/pipelines/245832

Any chance we can fix those things here? Or at least open a follow-up issue to fix them?

Fixed phpstan and phpcs issue, hence requesting review.

All pipelines are green now ~~ hence marking this RTBC!!

Nice. I think we need a few more things here, though - mostly uses of DeprecationHelper, and a follow-up.

❤️

The changes in this commit have caused significant compatibility issues. The changes to EmbedTypeBase::__construct() caused 🐛 Entity Embed fails to install after Embed module update Active . Deleting EmbedCKEditorPluginBase cause problems with any module that extends this, such as in entity_embed.

These changes are big enough that they should be done in a new minor or major version of the module. I suggest reverting this commit, making a 8.x-1.9 release, and creating a 2.0.x branch from version 8.x-1.8.

Updating to embed 1.8 has broken our ability to use entity_embed in ckeditor5. We get a spinner, then nothing. This essentially breaks any site building for us.

In the logs we get:

ArgumentCountError: Too few arguments to function Drupal\embed\EmbedType\EmbedTypeBase::__construct(), 3 passed in /code/web/modules/contrib/entity_embed/src/Plugin/EmbedType/Entity.php on line 56 and exactly 4 expected in Drupal\embed\EmbedType\EmbedTypeBase->__construct() (line 29 of /code/web/modules/contrib/embed/src/EmbedType/EmbedTypeBase.php).

What is the solution? Where is the embed module even coming from, it's not in our composer.json. Is it in core? We need to lock it down to 1.7 because 1.8 has broken our sites.

To constrain it, in your project, run: composer require 'drupal/embed:1.7'

Rather than running composer require you can add a conflict on 1.8.0, or if you prefer you can use ~1.8.0 or >= 1.8.0

"conflict": {
  "drupal/embed": "1.8.0"
}

This will allow it to upgrade to 8.x-1.9 when it's out, assuming that will fix the issue.

I reverted this commit and released 1.9. Whoops! I suspect this is a CKEditor 4 compatibility issue, so we'll do whatever work is needed in the 2.0.x branch.

Reopening this then.

ckeditor might cause issues too but the main problem is the EmbedTypeBase constructur.

It's a good example why I always recommend to not mix coding standards with compatibility patches. There's no need to rush that in. With that removed/made optional it shouldn't be necessary to add a 2.x branch, maybe it makes sense for the ckeditor stuff, not sure.

I suggest making a release node for the 2.0.x branch. That would allow setting the version of this issue to 2.0.x. Tests are passing. 8.x-1.x could be maintained for only Drupal 10, so no need for any D11 compatibility patches.

And my suggestion is to try and avoid a 2.x release. This project is barely maintained as it is, a new release branch is not going to help with that.

It might not even be necessary to drop the ckeditor integration. entity_browser has passing (some) tests on D11 on top of ckeditor + embed + entity_embed, all it takes is lenient + a sed command: https://git.drupalcode.org/project/entity_browser/-/blob/8.x-2.x/.gitlab....

My suggestion would be to reduce this MR to the minimal change that it takes to have green tests on D11 and move everything else to other issues.

Created a new MR based off MR!16 that adds BC support to EmbedTypeBase::__construct(). I assume this is all we need?

Left one comment, but the ckeditor4 integration removal is IMHO still a bit problematic for a minor update. If nothing else, it breaks entity_browser/tests/src/FunctionalJavascript/EntityEmbedCKEditor4Test.php. And I expect it will also break sites for people that happen to be stuck on ckeditor4. Sure, bad idea, but ckeditor project still has 80k users.

IMHO, either keep that or we ned to do it as 2.x

Thanks @berdir I addressed your comment and reverted the removal of the CKEditor4 \Drupal\embed\EmbedCKEditorPluginBase, and deprecated it instead. I assume this is all we need to do?

Two more comments.

Re-added the legacy ckeditor 4 hooks.

Left this as NW as I don't really have an answer for comment about the test change. Left as unresolved.

Based on the code audit of 1.9 using Update Status automation. I updated the code with the new class signatures and the info file.

Not sure if this patch is useful for anyone though..
I tested it on Drupal 10.3.2

Won't be testing it further until I get a Drupal 11 build running.

On path: admin/config/content/embed/button/add
After adding embed button, below is the output we are getting.

MR !24 is working fine on Drupal 11.

Hello @dave,

It would be greatly appreciated if you could find some time to release a Drupal 11 compatible version of embed so that the community can upgrade to the most recent version of Drupal. Thanks!

How is drupal 11 coming along?

Tests are passing and someone seems to have manually tested this. Can this be committed?

Linking 📌 Automated Drupal 11 compatibility fixes for file_browser Active since file_browser depends on this landing first too.

@phenaproxima I agree with @heddn's comment, what do you think? Would be great to get this fixed, looking at the stack of modules waiting for this to be fixed. For that reason setting priority to Major.

First I think we now need to agree on how to proceed.

There are still legit unresolved comments in MR !24. Going through the history of this issue, I notice that there has been a release with a BC break which had to be reverted subsequently (#50). I am not surprised that the maintainers are reluctant to merge this as is.

It was suggested to keep changes to a minimum when adapting contrib modules to new core releases. I very much agree with that approach. Based on the recommendation in #53 I'm going to work on a new MR with only the changes required for a D11 compatible release. The rest should go into dedicated issues:

• 💬 Drop support of CKEditor4 and include functionjavascript tests for ckeditor5 Needs work
• 🐛 PHPStan issue reported in gitlab Active
• 🐛 Fix PhpCS issue reported in gitlab RTBC
• 📌 Fix ESLint pipeline Active

Automated tests are green for MR !25 . I did some manual testing with entity_embed and entity_browser. Haven't come across any problems so far. Also filed 📌 Add test coverage for EmbedSettingsForm Active as another follow-up.

znerol → changed the visibility of the branch 3430097-automated-drupal-11 to hidden.

Another follow-up: 📌 Inject ModuleExtensionList into EmbedTypeBase constructor Active

Posted an question, but otherwise, I think this is pretty close.

Thanks @heddn, fixed.

The changes done here by @znerol are the right way to go, these look to be is the most minimal changes needed to get d11 compatibility.

I would suggest to merge those and tag a new release, the followup issues created can be done in the next release imho

I can't see anything fundamentally wrong with this fix.

I've tagged @dave reid on a related section of his code that is being removed by this merge request. Should he not have any objections I believe we need to move forward!

I propose regardless, take the current MR , merge it and cut an rc1 release or a beta release. If you're really afraid of this change then please create a new branch and tag a beta release so that we can move forward with D11!

Yes we need a D11 release ASAP. Other module, like url_embed, require this module (drupal/embed). I am currently updating a distro to D11.1, and running into an issue. Both url_embed and embed do not have D11 releases, and one requires the other, so it is not possible to do my usual composer.json workaround for this by defining a repository that points to the D11 PR branch in Drupal's gitlab. Therefore, to get the working, I will need to have a D11 release for embed.

I agree with @borisson_, it's better to just get a release out, so that people can actually include this module in their project, then issues can be ironed out quickly using patches in separate tickets.

Both url_embed and embed do not have D11 releases

URL Embed does have a D11-compatible release -- https://www.drupal.org/project/url_embed/releases/3.0.0-beta2 → -- but you're right that since it declares a dependency on drupal/embed, you can't use it with a D11 site.

@phenaproxima, are we able to move forward with this? Even an alpha release would get us off of our overrides!

Very few objections here. I think we should probably not delete any CKEditor 4 stuff, although deprecating it is the right move. By all means let's open a follow-up to branch 2.x and rip CKEditor 4 support out.

Easy solution to ck4

^11
Core requirements
This MR to a new branch and make minimum core ^11

Or just, like, un-delete all the stuff in embed.module which was deleted. That's even easier.

@phenaproxima would it be an option to just merge !25 mergeable? That just does the minimum amount of work and doesn't touch the ck4 stuff at all.

Setting state for #90.

znerol → changed the visibility of the branch 3430097-automated-drupal-11 to active.

znerol → changed the visibility of the branch 3430097-automated-drupal-11 to hidden.

znerol → changed the visibility of the branch 3430097-d11-minimal to hidden.

znerol → changed the visibility of the branch 3430097-automated-drupal-11 to active.

Had a short chat and it was pointed out that the current MR is fine, if we just revert those deleted bits of `embed.module`.

Hence, setting to NW, fixes should go into MR 24.

In other issues, people have suggested that dropping old version support should not be done in a patch-level release. To follow that, these changes should go in 2.0.x. There could be a 2.1.x which drops supports for CKEditor 4 and perhaps Drupal 10 as well.

@liam morland, can you cite any examples? I just don't wanna get yelled at once this gets merged.

Sorry, I can't quickly find examples. It does make sense to me. On the other hand, as long as the versions for which support is being dropped are no longer supported, perhaps it doesn't matter. This merge request puts the minimum at Drupal 10.3 and that is the oldest supported.

Yeah...to be honest, I'm not super concerned about it.

Thanks all! Merged into 8.x-1.x.

Thanks, what's the plan for the 1.10 tagged release?
We've been using MR 24 in D11.1.1 , wondering if we wait for a tag or tag before we replace our composer override

@joseph.olstad there is a 1.10 release.

Great, thanks!

znerol → closed merge request !25

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