Automated Drupal 11 compatibility fixes for entity_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 → , 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.

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 comment was forced and has ignored the check if a change was already posted. This is only done when we want to update the issue without waiting for changes to happen.

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.1.0
2. mglaman/phpstan-drupal: 1.2.10
3. palantirnet/drupal-rector: 0.20.1

Balu Ertl → made their first commit to this issue’s fork.

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

Based on the answers on this Slack thread, I have the conclusion that it's not strictly necessary to rely on the DeprecationHelper::backwardsCompatibleCall() utility. I assume the module maintainers probably still plan to continue supporting D9, in which this deprecation handling solution was not present yet and thus would have no effect anyway.

Albeit the change record → about the deprecation of watchdog_exception() suggests using \Drupal\Core\Utility\Error::logException() but for me it still seems to be just another wrapper around the \Drupal\Core\Logger\LoggerChannel::log().

Therefore I simplified exception logging with a more informative solution I peeked from core's \Drupal\Core\EventSubscriber\DefaultExceptionHtmlSubscriber::makeSubrequest().

Reviewed and tested (module install, configuring, trying out, uninstalling) on a D11-beta1 site, found working as expected:

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.3, 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.2
2. mglaman/phpstan-drupal: 1.2.11
3. palantirnet/drupal-rector: 0.20.3

The tests are failing in head, but its possible thats due to current incompatibilities, which would be made worse with D11 support. Those should get fixed before this gets committed.

I believe the MR of 📌 Fix tests Needs review should be included first before we move forward with this one.

Drupal Rector still suggests one point to fix:

As discussed today with @japerry → , we agreed this could be ignored because there is a @todo comment above the line communicating this should be fixed eventually in the future. So setting ticket status back to Needs Review.

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.3, 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.4
2. mglaman/phpstan-drupal: 1.2.11
3. palantirnet/drupal-rector: 0.20.3

Balu Ertl → changed the visibility of the branch 8.x-1.x to hidden.

Patch including rector's fixes from entity_embed.1.x-dev.rector.patch and info file fix.

@ukor → thanks for contributing code. This issue already has an open merge request. Would you mind committing the content of your patch file onto the feature branch of this issue repository, please? This would make this issue solved earlier by easing the module maintainer's job of reviewing changes. Thanks in advance!

Could not apply patch MR 36.

abhinavk → closed merge request !50

ankitv18 → changed the visibility of the branch 3430164-d11_ready to hidden.

I get the following error after trying to use the latest MR. Drupal 10.3.8.

Error: Call to undefined method Drupal\embed\EmbedType\EmbedTypeBase::create() in Drupal\entity_embed\Plugin\EmbedType\Entity::create() (line 56 of modules/contrib/entity_embed/src/Plugin/EmbedType/Entity.php).

Tested MR !47 changes in Drupal 10.3 and Drupal 11, functionality is working fine for me.
Changing status to RTBC.

ankitv18 → changed the visibility of the branch 3430164-automated-drupal-11 to hidden.

The comment from #27 is concerning and suggests that there may have been a backwards compatibility break here - can we look into that, please?

embed.module support for D11 got rolled back. We'll need to wait on https://www.drupal.org/project/embed/issues/3430097 📌 Automated Drupal 11 compatibility fixes for embed Needs review to land first. I'm way down the line in https://www.drupal.org/project/media_migration/issues/3431903 📌 Automated Drupal 11 compatibility fixes for media_migration Needs review waiting for this to land. So hopefully the upstream issues land soonish.

Hello, you can disregard my previous message. I realized that I wasn’t using the latest D11 patch from the embed module. Once I updated it, the error disappeared, and I have been using the patch from this ticket in production since then, with no issues.

See #31

This is testing against 8.x-1.8 which is the D11 compatible release that was rolled back.

📌 Automated Drupal 11 compatibility fixes for embed Needs review now has a minimal MR that does not include the addition of EmbedTypeBase::create meaning we can't call parent::create. This looks like the MR that will eventually be committed.

We need to figure out how to run tests against a patched version of embed 8.x-1.9 using that MR.

the MR for embed was merged however we're still waiting for a tagged release.

Done.

This needs significant work; I'm seeing many changes that appear to be unrelated to Drupal 11 compatibility. For reference:

• Let's deprecate CKEditor 4 support, but not remove it.
• Let's not add any new code or do any refactoring beyond what's needed for Drupal 11 compatibility.

Godspeed!

ck4 tests won't pass in D11

Then add something like this to them in setUp():

if (version_compare(\Drupal::VERSION, 10, '>')) {
  $this->markTestSkipped('This test will not run on Drupal 11 because CKEditor 4 is no longer supported.');
}

New branch and minimum release ^11 would prevent D10 installs from having ck4 issues.

I believe everything is actioned, and the pipeline's green.

It looks like changes in JavaScript source files have been reverted. I think that js/build/drupalentity.js should be reverted as well.

On the general approach: I think it is quite risky to attempt to convert all tests from cke4 to cke5 in this issue. It looks like some people are thinking that it is impossible to run cke4 tests in D11. Luckily this is not true, i.e., it is in fact possible to run tests against cke4 in D11.

In order to do that, the following is needed:

• Specify _LENIENT_ALLOW_LIST: "ckeditor" in .gitlab-ci.yml
• Add a before_script which unconditionally patches all contrib modules so as to make them installable in the current core version.

An example of this approach can be found in .gitlab-ci.yml of Entity Browser → which has had D11 releases for weeks even though embed and entity embed were still lacking D11 support.

I propose to try this here as well.

@znerol , changes were made also to core in D11 relating to the editor module and ck4. I wanted to run ck4 in D11 but I saw the core changes and decided to switch to ck5 (for better or for worse) due to the core and contrib complexity involved and the fact that ck4 might be dropped from contrib also. So even if I was to fork ckeditor and upgrade it to D11, I asked myself what the likelihood that ck4 was still intact in contrib and decided that it was a pretty risky adventure to embark on. So far we've got ck5 working ok although we did have to sacrifice some functionality like html filtering and one contrib modules widget.

Hoping for a tag release soon to ease and speed up testing and updates in projects.
Maybe a new 2.0.x branch can help with CKE5 only supported version, keeping the 8.x-1.x branch with minimal support for CKE4

@rajab natshah , I 100% agree with your comment in #48

Enough with the dipsy doodling, for those ACTUALLY using Drupal 11, only ckeditor 5 makes sense. Forget ck4 in Drupal 11, it's a non-starter.

Working on the tests in order to minimize the amount of coverage loss.

This is good to go. There is one weird test fail in phpunit (previous minor). I'd probably just ignore it for the moment.

This module needs a big effort to increase functional JavaScript test coverage for CKEditor5. I wouldn't attempt to convert the existing tests. Just write new ones for the new editor and leave the old ones there for reference until CKE4 support is removed completely. That work needs to happen in another issue though.

Great job, thank you very much!

To resolve the test issue, just adjust the core requirements:

^10.4 || ^11.1

There's no need to support 10.3 for much longer anyway.

Tests are currently passing on 10.4 and 11.1.1

Removed OPT_IN_TEST_PREVIOUS_MINOR from .gitlab-ci.yml.

There's no need to support 10.3 for much longer anyway.

Security support is for another 4 months, there's no harm in keeping it here.

A few questions on the MR, which I've left comments for.

My main concern here is that most of the CI jobs are actually failing: https://git.drupalcode.org/issue/entity_embed-3430164/-/pipelines/410987. Too many for comfort.

The ones I'm most concerned about are:

• phpunit (next minor) and phpunit (max PHP version) -- if we're aiming for D11 compatibility, neither of these should be failing. I'd like these to be fully green in order to merge this.
• All of the PHPStan jobs. I usually dislike PHPStan because it has a tendency to treat harmless quirks as errors, but in this case it looks like it is catching what might be legitimate bugs; see https://git.drupalcode.org/issue/entity_embed-3430164/-/jobs/4198055#L84, https://git.drupalcode.org/issue/entity_embed-3430164/-/jobs/4198055#L84, and several others.

The other stuff would be fine to fix in follow-ups.

@phenaproxima , next minor and next major have not been released yet. It is completely normal that they could be failing at this point in time. We should focus on 10.4 and 11.1 and if ambitious 10.3 and 11.0.

I was considering to suggest to disable those jobs earlier, especially php max (which is PHP 8.4 now), but yeah, they are definitely not in scope of this issue.

About phpstan, anything that isn't clearly a D11 thing should IMHO also be ignored here. For one I opened an issue because that's IMHO not correct: https://github.com/mglaman/phpstan-drupal/issues/824.

So, the test fail in phpunit (next minor) is in fact really interesting. MediaImageDecorator::build() attempts to pass the alt and title attribute values into the referenced entity image / thumbnail.

This worked well until 🐛 Referring the same entity multiple times breaks _referringItem Active fixed a corner case by introducing a clone $entity which is triggered during parent::build(). As a result, $build['#media'] and $this->decorated->getEntityFromContext(); are not referring to the same object anymore. Modifications to the latter do not have any effect anymore.

When I revert core commit 40c1ab52 tests are passing.

Found a way to refresh the entity context value.

Great sleuthing on the next minor fix @znerol ! that core commit happened only about 9 days ago

Looking at failing phpunit (max PHP version) now.

A lot of noise is due to one line in EntityEmbedDialog.php, lets get rid of that:

    Deprecated: Drupal\entity_embed\Form\EntityEmbedDialog::buildForm(): Implicitly marking parameter $editor as nullable is deprecated, the explicit nullable type must be used instead in /builds/issue/entity_embed-3430164/src/Form/EntityEmbedDialog.php on line 183
    
    Deprecated: Drupal\entity_embed\Form\EntityEmbedDialog::buildForm(): Implicitly marking parameter $embed_button as nullable is deprecated, the explicit nullable type must be used instead in /builds/issue/entity_embed-3430164/src/Form/EntityEmbedDialog.php on line 183

Looks like phpunit (max PHP version) was failing because of PHP8.4 deprecations around implicit nullable type hints. All phpunit jobs are green now.

Fixed the phpstan jobs as well. The phpstan.neon config is based on the official drupal ci template.

Additionally it ignores CKEditor 4 plugin paths. I also added a phpstan baseline in order to suppress the following non-D11 related isues:

1. \Drupal calls should be avoided in classes, use dependency injection instead
2. config:entity_view_mode_list cache tag might be unclear and does not contain the cache key in it. (see #58.

Note that EntityEmbedHooksTest randomly fails in any of the phpstan jobs. This is most probably due to its reliance on state to activate the hook in the test module (see 🐛 Race conditions/bad lock logic in CacheCollector/State Active ).

Thanks for this effort @znerol! And thanks for explaining some of the test changes.

I think I'm mostly okay with this, but we probably shouldn't change parameter types in base classes (I'm okay with it in forms -- those shouldn't be subclassed anyway) since that would violate PHP's contravariance rules, IIRC. I know that restoring them would cause some deprecation notices but if it's just those, I think that's worthwhile in order to not break this module for people who might have subclassed it.

Adding ? is not a type change, it's fine to add this.

OK, I took a look at the original PHP 7.1 RFC for nullable types: https://wiki.php.net/rfc/nullable_types, and I think you're probably right. Quoting:

Because a parameter with = null is a superset of ?, you can use a parameter with a default value of null where a nullable type existed in the parent.

interface Contract {
    function method(?Foo $foo): bool;
}
 
class Implementation implements Contract {
    function method(?Foo $foo = null): bool {
        return is_null($foo);
    }
}

The reverse is not true, however: you cannot use only a nullable type where a default value existed in the parent, because the parameter is no longer optional.

If I understand this correctly, ?Type = NULL (which is what will be in the parent) and Type = NULL (which is what will be in any subclasses) are considered compatible by PHP.

So okay...I think that addresses my concerns. Let's ship it.

Merged into 8.x-1.x and will tag shortly. Thanks, everyone.

Great work everyone 💪

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