Automated Drupal 11 compatibility fixes for entity_embed

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.

Although 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:

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 #3415080: Fix functional javascript tests for entity_embed / ckeditor4 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.

Also, as he suggested the following open tickets should be resolved before we focus on D11 compatibility:

13 #3410132: Entity embed dialog can no longer use custom data- attributes in CKEditor 5

8 #3295818: TypeError: count(): Argument #1 ($value) must be of type Countable|array, null given

5 #3416512: Trying to edit embedded entities causes uncaught exception

4 #3402443: PHP error: Call to a member function id() on null

3 #3402255: Button label double-escaped in CKEditor5

2 #3362388: PHP 8.2 Deprecation: Creation dynamic property

1 #3450460: Warning: Array to string conversion in entity_embed/src/Form/EntityEmbedDialog.php on line 839

(In order of ticket subscribers.)

Version entity_embed 8.x-1.6
Patch including rector's fixes from entity_embed.1.x-dev.rector.patch and info.yml 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.

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.

#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 to land first. I'm way down the line in https://www.drupal.org/project/media_migration/issues/3431903 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

I've reviewed the blocker. It's ready to go imho. Action is needed by a maintainer.

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

#3430097: Automated Drupal 11 compatibility fixes for embed 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.');
}

Merging to a new branch and dropping ^10, changing core requirements to ^11 only would prevent D10 folks from installing the D11 release and getting stuck on missing ck4 support and would allow future D10 releases that support both while the D11 branch supports only D11+ck5.

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 (code removal). 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 and risks 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. It has been a bit tricky however so far we've got ck5 working ok although we did have to sacrifice some functionality like html filtering (due to overhauls that I still do not understand) and one contrib modules widget does not exist in ck5 that we'll migrate to another at some point.

Hoping for a tag release soon to simplify and accelerate testing and updates in projects.
A new 2.0.x branch dedicated to CKE5-only support could be helpful, while maintaining minimal support for CKE4 in the 8.x-1.x branch.

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

Enough with the dipsy doodling, for those ACTUALLY using Drupal 11 and adopting it, 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. We should fix as many of these as possible, and things that can't be fixed (maybe due to implementing interfaces that only exist in CKEditor 4) should be suppressed explicitly, with comments explaining why.

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 ambitiously 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 #3489179: Referring the same entity multiple times breaks _referringItem 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 #3496257: Race conditions in CacheCollector/State (again)).

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, since either one could be NULL.

Proof (thanks @berdir): https://3v4l.org/Shl6Y

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 💪