Drupal 10 update

Added couple of additional fixes needed for PHP8 / D10

Applied MR changes still found few non ignorable error in drupal check. Created new patch for fixing such deprecated errors. Attached screenshot for reference.

I have tested Patch #7 in Drupal 10.0.9. It works fine for me.
Moving to RTBC.

Hi guys

@Centarro, @jsacksick, @mglaman, @attisan

Who can merge this MR and create a proper release?

It looks like an OK and it was reviewed ~1 month ago and no objections yet.

Kind regards,
Andrei

Hi,

Patch in #7 was not applying in composer for me. Attaching the rerolled patch.

Thanks

A patch is directly generated from the merge request; there is no need to add patches manually.
https://git.drupalcode.org/project/commerce_cart_api/-/merge_requests/3....

I run it on PHP8.1, and D10 with Commerce 2.36 on the live site with no issues for the last two weeks.

The "plain diff" text, though, is not having the best visibility, UX-wise.

As well moving back to "Needs review" - the merge request.

Patch from #7 does not include all required changes

Drupal 9 EOL is close, indeed.

because of #11

please, merge

@valic: Can you elaborate on what changes are missing please?

Reviewing

The patch in #10 and #7 are missing fixes for the CartBlock and file_create_url and drupal_get_path.

The MR is missing the test changes for setUp return type and $modules visibility. Fixing.

I updated the MR and I'm running D9 and D10 tests.

Tests are failing on the current dev https://www.drupal.org/node/2961571/qa →
I don't understand why tests should be a blocker for the code in this issue

The PHP 8.1 & MySQL 5.7, Drupal 9.5.x failures are due to the joy of trailing decimals.

I don't know what's going on for PHP 8.1 & MySQL 5.7, Drupal 10.1.x, they're just hanging.

Hiding files and assigning issue credit.

I'm done for now. The Drupal 10 tests are failing but it looks like Drupal 9 (our baseline) are passing. Someone will need to collaborate on the MR to fix the failures. I made the adjustments via the GitLab Web IDE as I didn't have time to work in a full environment. Since the D10 jobs on DrupalCI wouldn't finish, I have no idea why they're failing.

can we merge the code into the dev branch while somebody else is figuring out the tests?
It would unblock composer installations at least

No. If you're blocked with Composer feel free to use my lenient Composer plugin.

https://github.com/mglaman/composer-drupal-lenient

If the tests fail that means it doesn't actually work.

or tests are not working because of infrastructure or tests themselves

we have RTBC from https://www.drupal.org/project/commerce_cart_api/issues/3332548#comment-... 📌 Drupal 10 update Needs review which I trust more than tests

@podarok if you can run the tests locally on D10 and screenshot green results, we can trust that.

14:02:29 ESLint couldn't find the config "eslint-config-airbnb" to extend from. Please check that the name of the config is correct.
14:02:29 
14:02:29 The config "eslint-config-airbnb" was referenced from the config file in "/var/www/html/modules/contrib/commerce_cart_api/tests/modules/commerce_cart_js/.eslintrc.json".

Maybe that one causes the issues? Can we exclude that from CI perhaps?

@mglaman I used your lenient Composer plugin to get the Commerce API module installed in Drupal 10 however I cannot tick the checkbox to enable the module under Admin>Extend "This version is not compatible with Drupal 10.1.2 and should be replaced". Any way to circumvent this?

The eslint is just a warning.

If you use the lenient plugin you also need to patch it with this patch.

Added a few comments. The tests don't run because of non-existent permissions:

Adding non-existent permissions to a role is deprecated in drupal:9.3.0 and triggers a runtime exception before drupal:10.0.0

Remaining self deprecation notices (37)

  6x: Adding non-existent permissions to a role is deprecated in drupal:9.3.0 and triggers a runtime exception before drupal:10.0.0. The incorrect permissions are "restful post commerce_cart_add". Permissions should be defined in a permissions.yml file or a permission callback. See https://www.drupal.org/node/3193348
    1x in CartAddResourceTest::testMalformedPayload from Drupal\Tests\commerce_cart_api\Functional
    1x in CartAddResourceTest::testInvalidPurchasedEntity from Drupal\Tests\commerce_cart_api\Functional
    1x in CartAddResourceTest::testPostOrderItems from Drupal\Tests\commerce_cart_api\Functional
    1x in CartAddResourceTest::testCombine from Drupal\Tests\commerce_cart_api\Functional
    1x in TranslationCartAddResourceTest::testPurchasedEntityAdded from Drupal\Tests\commerce_cart_api\Functional
    1x in TranslationCartAddResourceTest::testProperTranslatedPurchasedEntityAdded from Drupal\Tests\commerce_cart_api\Functional

  5x: Adding non-existent permissions to a role is deprecated in drupal:9.3.0 and triggers a runtime exception before drupal:10.0.0. The incorrect permissions are "restful get commerce_cart_update_item". Permissions should be defined in a permissions.yml file or a permission callback. See https://www.drupal.org/node/3193348
    1x in CartAccessApiResourceTest::testNoParameters from Drupal\Tests\commerce_cart_api\Functional
    1x in CartAccessApiResourceTest::testNoCart from Drupal\Tests\commerce_cart_api\Functional
    1x in CartAccessApiResourceTest::testInvalidCart from Drupal\Tests\commerce_cart_api\Functional
    1x in CartAccessApiResourceTest::testNotUsersCart from Drupal\Tests\commerce_cart_api\Functional
    1x in CartAccessApiResourceTest::testInvalidOrderItemCart from Drupal\Tests\commerce_cart_api\Functional

  5x: Adding non-existent permissions to a role is deprecated in drupal:9.3.0 and triggers a runtime exception before drupal:10.0.0. The incorrect permissions are "restful get commerce_cart_update_item", "restful patch commerce_cart_update_item". Permissions should be defined in a permissions.yml file or a permission callback. See https://www.drupal.org/node/3193348
    1x in CartAccessApiResourceTest::testNoParameters from Drupal\Tests\commerce_cart_api\Functional
    1x in CartAccessApiResourceTest::testNoCart from Drupal\Tests\commerce_cart_api\Functional
    1x in CartAccessApiResourceTest::testInvalidCart from Drupal\Tests\commerce_cart_api\Functional
    1x in CartAccessApiResourceTest::testNotUsersCart from Drupal\Tests\commerce_cart_api\Functional
    1x in CartAccessApiResourceTest::testInvalidOrderItemCart from Drupal\Tests\commerce_cart_api\Functional

  5x: Adding non-existent permissions to a role is deprecated in drupal:9.3.0 and triggers a runtime exception before drupal:10.0.0. The incorrect permissions are "restful patch commerce_cart_update_item". Permissions should be defined in a permissions.yml file or a permission callback. See https://www.drupal.org/node/3193348
    1x in CartUpdateItemResourceTest::testMissingCart from Drupal\Tests\commerce_cart_api\Functional
    1x in CartUpdateItemResourceTest::testMissingOrderItem from Drupal\Tests\commerce_cart_api\Functional
    1x in CartUpdateItemResourceTest::testCartNoAccess from Drupal\Tests\commerce_cart_api\Functional
    1x in CartUpdateItemResourceTest::testInvalidPayload from Drupal\Tests\commerce_cart_api\Functional
    1x in CartUpdateItemResourceTest::testPatchOrderItem from Drupal\Tests\commerce_cart_api\Functional

  3x: Adding non-existent permissions to a role is deprecated in drupal:9.3.0 and triggers a runtime exception before drupal:10.0.0. The incorrect permissions are "restful patch commerce_cart_update_items". Permissions should be defined in a permissions.yml file or a permission callback. See https://www.drupal.org/node/3193348
    1x in CartUpdateItemsResourceTest::testMissingCart from Drupal\Tests\commerce_cart_api\Functional
    1x in CartUpdateItemsResourceTest::testInvalidPayload from Drupal\Tests\commerce_cart_api\Functional
    1x in CartUpdateItemsResourceTest::testPatchOrderItems from Drupal\Tests\commerce_cart_api\Functional

  2x: Adding non-existent permissions to a role is deprecated in drupal:9.3.0 and triggers a runtime exception before drupal:10.0.0. The incorrect permissions are "restful get commerce_cart_canonical". Permissions should be defined in a permissions.yml file or a permission callback. See https://www.drupal.org/node/3193348
    1x in CartCanonicalResourceTest::testNoCartAvailable from Drupal\Tests\commerce_cart_api\Functional
    1x in CartCanonicalResourceTest::testGetCart from Drupal\Tests\commerce_cart_api\Functional

  2x: Adding non-existent permissions to a role is deprecated in drupal:9.3.0 and triggers a runtime exception before drupal:10.0.0. The incorrect permissions are "restful get commerce_cart_collection". Permissions should be defined in a permissions.yml file or a permission callback. See https://www.drupal.org/node/3193348
    1x in CartCollectionResourceTest::testNoCartAvailable from Drupal\Tests\commerce_cart_api\Functional
    1x in CartCollectionResourceTest::testGetCarts from Drupal\Tests\commerce_cart_api\Functional

  2x: Adding non-existent permissions to a role is deprecated in drupal:9.3.0 and triggers a runtime exception before drupal:10.0.0. The incorrect permissions are "restful get commerce_cart_coupons". Permissions should be defined in a permissions.yml file or a permission callback. See https://www.drupal.org/node/3193348
    1x in CartCouponsResourceTest::testApplyCoupons from Drupal\Tests\commerce_cart_api\Functional
    1x in CartCouponsResourceTest::testInvalidCoupons from Drupal\Tests\commerce_cart_api\Functional

  2x: Adding non-existent permissions to a role is deprecated in drupal:9.3.0 and triggers a runtime exception before drupal:10.0.0. The incorrect permissions are "restful get commerce_cart_coupons", "restful patch commerce_cart_coupons". Permissions should be defined in a permissions.yml file or a permission callback. See https://www.drupal.org/node/3193348
    1x in CartCouponsResourceTest::testApplyCoupons from Drupal\Tests\commerce_cart_api\Functional
    1x in CartCouponsResourceTest::testInvalidCoupons from Drupal\Tests\commerce_cart_api\Functional

  2x: Adding non-existent permissions to a role is deprecated in drupal:9.3.0 and triggers a runtime exception before drupal:10.0.0. The incorrect permissions are "restful get commerce_cart_coupons", "restful patch commerce_cart_coupons", "restful delete commerce_cart_coupons". Permissions should be defined in a permissions.yml file or a permission callback. See https://www.drupal.org/node/3193348
    1x in CartCouponsResourceTest::testApplyCoupons from Drupal\Tests\commerce_cart_api\Functional
    1x in CartCouponsResourceTest::testInvalidCoupons from Drupal\Tests\commerce_cart_api\Functional

  2x: Adding non-existent permissions to a role is deprecated in drupal:9.3.0 and triggers a runtime exception before drupal:10.0.0. The incorrect permissions are "restful delete commerce_cart_remove_item". Permissions should be defined in a permissions.yml file or a permission callback. See https://www.drupal.org/node/3193348
    1x in CartRemoveItemResourceTest::testNoCartRemoveItem from Drupal\Tests\commerce_cart_api\Functional
    1x in CartRemoveItemResourceTest::testRemoveItem from Drupal\Tests\commerce_cart_api\Functional

  1x: Adding non-existent permissions to a role is deprecated in drupal:9.3.0 and triggers a runtime exception before drupal:10.0.0. The incorrect permissions are "restful delete commerce_cart_clear". Permissions should be defined in a permissions.yml file or a permission callback. See https://www.drupal.org/node/3193348
    1x in CartClearResourceTest::testClearCart from Drupal\Tests\commerce_cart_api\Functional

Great! Tests seem to run now. Now we should discuss whether to drop the Drupal 9 support here and resolve the remaining comments. Maybe one of the maintainers could take a look? I could implement the proposed changes afterwards.

No. Do not drop D9 support.

https://mglaman.dev/blog/drupal-module-semantic-versioning-drupal-core-s...

It's possible to support 9.5 and 10.0.

I thought so, we did that with CAPTCHA, which caused @japerry to get really unhappy 😅. Further causing abysmal versioning for that module...

But I still haven't figured out best practice methods to solve issues between Symfony incompatibilities, e.g., this commit by @czigor causes incompatibilities with Drupal < 10.

So do we really need to adjust the code along the lines of:

    $isMainRequest = FALSE;
    if (version_compare(\Drupal::VERSION, '10', '<')) {
      $isMainRequest = $event->isMasterRequest();
    }
    $isMainRequest = $event->isMainRequest();
    if (!$isMainRequest) {
      return;
    }

? This doesn't feel very clean.

I thought isMasterRequest still worked on D10 and had a backwards compatibility layer in Drupal core or Symfony since it was so disruptive.

Otherwise, yeah your example code is what we do. That's just how it goes

Possible! I haven't tested it, just saw this change record → and your phpstan-drupal issue, where no backwards compatibility was mentioned!

Alright, we have ANOTHER Drupal 9 compatibility issue. The js libraries "backbone" and "underscore" are deprecated in Drupal 10 and marked internal. So I guess if we want to keep compatibility with both Drupal versions, we need to rewrite the JavaScript files to use native js instead of backbone / underscore.

Another option would be to drop the Drupal 9 compatibility, define the libraries internal (see MR, although it is stated, that this explicitly should NOT be done), create a Drupal 10 only alpha release and a major follow-up issue to adjust all code related to "backbone" and "underscore".

I would prefer to directly rewrite the code inside this issue. Although this would take more time.

The maintainers should decide, anything else should be fine now.

Please note: Just 1 month left!

Switch the libraries to use the internal names for them. It's fine.

Alright, back to "Needs review" then! Maybe we add a follow-up issue for the endless test run, which results in the CI being aborted?

Maybe we add a follow-up issue for the endless test run, which results in the CI being aborted?

Yes please, that should be documented and aborted. I don't think it's related to the changes here.

As of #40, we should simply switch the libraries dependency to their internal counterpart. Apparently, "internal.backbone" will simply resolve to "backbone" in D9.

Taken from a conversation in the slack "#contribute" channel:

Grevil
Main problem currently is the use of "backbone" and "underscore", we could use "internal.backbone" and "internal.underscore" for D10 compatibility temporarily, but I am unsure how the "internal" libraries would resolve in D9? Since in D9 they are called "backbone" and "underscore" respectively.

mglaman
This was fixed in some other modules. I wish I could recall which. But internal.backbone was added when backbone was deprecated

Grevil
Do we need to implement "hook_library_info_alter()" for this?

mglaman
No

mglaman
Both will exist

Grevil
Ah both exist in D9? Great!

See https://drupal.slack.com/archives/C1BMUQ9U6/p1696945452239079

The rest of the unresolved threads can be resolved (which I can't do myself).

Adjusted the version requirement for the test modules. Should be all working now. @mglaman should decide if we take the risk and push the changes without running tests, or wait, that somebody finds out how to fix them (Even running them locally will result in a timeout).

Thanks for working on drupal 10.

Uncaught PHP Exception Error: "Class "Drupal\\commerce_cart_api\\Normalizer\\FieldNormalizer" not found" at core/lib/Drupal/Component/DependencyInjection/Container.php line 259

We are getting the above issue after the upgrade. The site fails to load with a couple of other errors.

Same here - thank you for working on this upgrade. I'm getting the same result, though:

Uncaught PHP Exception Error: "Class "Drupal\\commerce_cart_api\\Normalizer\\FieldNormalizer" not found" at core/lib/Drupal/Component/DependencyInjection/Container.php line 259

Hi @Centarro, @jsacksick, @mglaman, @attisan @grevil

Thank you for your work so far on this.

What is the best way to proceed with this?

Increasing priority of this ticket, since Drupal 8.8 and 9 are unsupported, making it impossible to use a supported version of Drupal when relying on this module.

Setup Gitlab CI, let's see what the tests say.

There's an issue with the EntityReferenceNormalizerTest but I can't really figure out why... This particular test hangs on D10 but passes on D9.5.

Way to go @jsacksick!

Looking into the D9 failures.

D9 is green, now.

Committed, thanks everyone!!

Amazing! Thank you @jsacksick @mglaman @svendecabooter and everyone who helped get this to D10.

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

Great stuff! Thanks all! @mttsmmrssprks could you credit me? Thanks :)

Crediting Grevil for testing and receiving work

Cheers @mglaman!