#ProjectUpdateBotD11

I'm going to make a release soon so looking at all recently fixed issues along with RTBC and needs review.

Fixed all issues and merged the changes to the dev branch.

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

torfj → changed the visibility of the branch 3431948-automated-drupal-11 to hidden.

The Drupal 11 compatibility fixes for leave_confirm have resolved the issues.

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

Merged.

Tests pass on MR 31. Updates made by @Fabsgugu no longer give an error.

Marking as RTBC. Thanks so much!

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

Thanks @dww.
Committing. 🍻

Hello! Is there any estimate of obtaining the compatibility merge and being able to have a Drupal 11 version of the module? I need help with something, I am available for anything.

aastrong → closed merge request !11

aastrong → closed merge request !16

Added an hook_update_N in order to actually update the entity type definition.

Will there also be a normal release with Drupal 11 compatibility?

At the moment the dev version is compatible with Drupal 11 but the stable version is not...

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

Merged for 3.0.x

Merged for 2.0.x

I know this module is minimally maintained, but could you please, please, please 🥺 release a new version for Drupal 11?

There is no stable release yet. So let's forget about BC.

Merging this even though the pipeline needs work, unblocks other tasks.

I think ReportDateField can be updated to not override the contructors by simply overridding the create method and calling the parent create() method. This way we don't have to keep track of the parent constructor parameters.

This would be a BC break for any contrib / custom code which inherits from ReportDateField. Is that acceptable?

Also dug up the CR → for the views EntityField change. This change was introduced in Drupal 10.3.

smustgrave → closed merge request !17

• Calling createUser() with $values as the first parameter and/or $permissions as the second parameter is deprecated → is 10.1 (though since invocations are now using named parameters, this could work in previous core versions as well).
• Url::toRenderArray() and Url::renderAccess() are deprecated → is 10.1 (thogh Link::fromTextAndUrl() did exist before, thus could work with previous core versions as well)

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

I'm curious? What is the change that forces to ditch compatibility < D10.3?
I think ReportDateField can be updated to not override the contructors by simply overridding the create method and calling the parent create() method. This way we don't have to keep track of the parent constructor parameters.

Can we change this so I can merge the MR?

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

📌 Offering to Support and Maintain the Gin Type Tray module Active

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

Maybe we need something like that.

Attached a possible patch.

Yes please.

@kevinquilen:
I have tested the module with Drupal 11 and it is not yet compatible.
The function menu_ui_form_node_form_alter() does not exist anymore under Drupal 11.

The hook as been moved to src/Hook/MenuUiHooks.php

Basically, all node form edits end up in a fatal error if you have this module installed.

Plus one for merging this. It's not a big change but its time has come.

Prepare for Drupal 11

Better to review and merge 📌 Add gitlab ci Active

This has been released releases ago, but forgot to move the issue to 'Fixed'.

I've merged and there were no issues. Released at 1.0.1

After visiting /admin/structure/types/manage/article/fields/add-field page, I get Error 500:

TypeError: Cannot access offset of type Drupal\Core\StringTranslation\TranslatableMarkup in isset or empty in Drupal\Core\Plugin\DefaultPluginManager->doGetDefinition() (line 45 of core/lib/Drupal/Component/Plugin/Discovery/DiscoveryTrait.php).

It works fine in D10.

Merged and released at 1.1.9. Now the module is fully compatible with Drupal 11.

The tests fails, I guess because Salesforce module was not yet compatible with 11, but now it is compatible.

I tested it locally and it works fine.

I tested the module locally on D11, and found no issues.

Alright, I just tested it and I run into the following issue: 🐛 "InvalidArgumentException: Field amount is unknown." when accessing "/admin/commerce/reports/orders" Active . Although this is completely unrelated to this issue, as it also happens pre patch, and is related to the "amount" entity field, and NOT the changes done for "$row['title']['data']".

When commenting out the "amount" related code inside "ReportsListBuilder" (which were untouched in this MR), everything works as expected! Meaning, the changes by @znerol targeting "$row['title']['data']" inside the "ReportsListBuilder" are working as expected!

So RTBC!

Released at 2.0.2

Released at 1.1.1

Wow, great work @znerol! Code LGTM! I'll test it locally and come back here.

PHPunit tests fails after adapting the code to Drupal 11. It is needed reviewing what is happening and fix it.

🐛 Fix tests on HEAD Active for the tests fixes, otherwise will release 1.2.0 with these changes.

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

Updating status

Entity embed 1.7 is out

Great work everyone 💪

The Drupal 11 compatibility issues for have been fixed.

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

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.

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

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.

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

It would be great to have a release compatible with Drupal 11

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

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.

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

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

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

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

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

Done; new release made - just wanted to fix some phpcs/phpstan warnings first.

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

Thanks for the offer, looks like its merged already, just needs a new release. Will take a look

The Drupal 11 compatibility issues for have been fixed.

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

Found a way to refresh the entity context value.

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.

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.

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

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.

Thanks for the update, Erik! I’ve implemented the patch and it’s fixed. Let me know if anything else comes up.

The Drupal 11 compatibility issue has been fixed.

Thanks, David! I really appreciate your time.
I had my final testing round and hoping for a pre-release tag, or the 8.x-2.2 tag.

Cheers. Committed! 🍻

Test on Drupal 11.
phpstan passes.

Fixed CI and phpstan. Other CI tasks should be addressed as separate issues.

vladimiraus → changed the visibility of the branch 3438617-d11-ready to hidden.

Could somebody please take a look at the merge request MR!6 above at #5?

Thanks in advance!

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

Could this get merged please?

The changes from MR!10 above at #6 look "legit" 👌

Would be great if someone with write access could take a look at this soon!
Thanks!

dydave → changed the visibility of the branch 3430612-automated-drupal-11 to hidden.

Additional commits look good to me and the proof is in the green tests. However standard review procedures prevent me from marking this RTBC. Thanks for the update @keszthelyi.

Thank you for contribution and patience.

The compatibility issue has been fixed.

The compatibility issue has been fixed.

1.0.2 is compatible with Drupal 11

Fixed!

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

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

Duplicate of 📌 Automated Drupal 11 compatibility fixes for updated Needs review .

When will the Drupal 11 version of this module be released?

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

Thanks bot, proposed patch merged.

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

Thanks!

Reviews and tested by 2 developers in Droptica

does not apply to latest dev branch

So I think we can mark as "Needs Works", right?!
Plan is merge that into 3.0.x branch
If can't be merged now is necessary effort to fix that
So lets mark as NR to track that
If someone needs, can create a patch file to be applied on 3.0.0 via composer
However the MR should be merged into dev branch, so IMHO makes sense mark that to work

After upgrading the project to D11 this way, I got this error in status reports:
AMP
Not available
The AMP module requires the PHP AMP library.

So this still needs work.

joevagyok → closed merge request !4

8.x-1.6 is Drupal 11 compatible.

Added 🐛 Fix phpcs, phpstan, cspell Postponed for after this is merged (set it to postponed for now)

Adding this in my repositories section in my composer.json file helped me (thanks to #3325977 🐛 php 8.1 - lullabot/amp seems abandoned: replace abandoned querypath/querypath with gravitypdf/querypath Active ):

{
            "type": "package",
            "package": {
                "name": "lullabot/amp",
                "version": "2.0.4",
                "type": "composer",
                "source": {
                    "type": "git",
                    "url": "https://github.com/deimosindustries/amp-library.git",
                    "reference": "6a3074586ead73cd47cb4fc4e2c429df19fcc2b6"
                }
            }
        },

with this added in + adding this issue via drupal lenient, I was able to upgrade my project to D11.

Okay tests now pass with this (but I've had to take the code from 🐛 Dynamic property issue in ConfigEntityQuery with PHP 8.2 RTBC for tests to pass otherwise the deprecation is a fatal error in php 8.3).

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

I can confirm that the MR applies BUT i can also confirm what @elisur says: Even with this MR applied, you cant use this module in your D11 project because of the version requirement of lullabot/amp. That should be updated as well.

This is the only module blocking the update to D11 in this project.

Will it work if the required lullabot/amp version gets updated in the MR ?
Maybe "lullabot/amp": "^1.1 || ^2.0.4", should become "lullabot/amp": "^1.1 || ^2.0.4 || ",

Test failure for D11:

1) Drupal\Tests\config_views\Functional\DefaultViewsTest::testViews
Drupal\Core\Extension\Exception\UnknownExtensionException: Unknown themes: classy.

fixed in https://www.drupal.org/project/verf/releases/2.1.0 →
tnx

Patch does not apply to 3.0.0

The MR applies to the 3.0.0 branch but does not apply to latest dev branch at tis moment.

The drupal/amp module is still not compatible with Drupal 11 due to its dependencies in composer.json. It requires: "lullabot/amp": "^1.1 || ^2.0.4" (PHP >=5.6). The latest stable version of lullabot/amp is 2.1.1, followed by dev-main, but even these still require PHP >=5.6. As long as this does not change, the drupal/amp module will not be compatible with Drupal 11.

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.

Removed OPT_IN_TEST_PREVIOUS_MINOR from .gitlab-ci.yml.

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

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.

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

Note: probably not wise to declare D12 compatibility until D12's API is finalized. 😅 See 🐛 Premature declaration of D12 compatibility Active

Hi, any chance we can merge this change in soon? The patch works for me.

Ready to merge upstream.