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.