Problem/Motivation

After #3065951: Using \Drupal\commerce_cart\CartManager::emptyCart() vs \Drupal\commerce_cart\CartManager::removeOrderItem was committed, it's no longer possible to install the module commerce_promotion (and as a consequence, probably commerce at all for many people).

Steps to reproduce:

Tested with Drupal 9.0.x (4510338c85b77f25743be6dd4cfbb942cc3b171d) and commerce 2.x-dev (543a351)

Install minimal. Enable commerce promotion.

 eirik@eirik-ThinkPad-T470  ~/Sites/drupal8/drupal   9.0.x ●  drush si minimal                                                                                                             [09.06.20|16:31:42]

 You are about to DROP all tables in your 'drupal8_local' database. Do you want to continue? (yes/no) [yes]:
 > 

 [notice] Starting Drupal installation. This takes a while.
 [success] Installation complete.  User name: admin  User password: VRDfiFEFc2
 eirik@eirik-ThinkPad-T470  ~/Sites/drupal8/drupal   9.0.x ●  drush en commerce_promotion                                                                                                  [09.06.20|16:32:43]
The following module(s) will be enabled: commerce_promotion, commerce, address, entity, inline_entity_form, token, datetime, views, commerce_order, commerce_price, commerce_store, options, path, commerce_number_pattern, entity_reference_revisions, profile, state_machine

 Do you want to continue? (yes/no) [yes]:
 > 

Error: Class 'Drupal\commerce_cart\Event\CartEvents' not found in /home/eirik/Sites/drupal8/drupal/modules/contrib/commerce/modules/promotion/src/EventSubscriber/CartEventSubscriber.php on line 16 #0 /home/eirik/Sites/drupal8/drupal/core/lib/Drupal/Core/DependencyInjection/Compiler/RegisterEventSubscribersPass.php(37): Drupal\commerce_promotion\EventSubscriber\CartEventSubscriber::getSubscribedEvents()
#1 /home/eirik/Sites/drupal8/drupal/vendor/symfony/dependency-injection/Compiler/Compiler.php(94): Drupal\Core\DependencyInjection\Compiler\RegisterEventSubscribersPass->process(Object(Drupal\Core\DependencyInjection\ContainerBuilder))
#2 /home/eirik/Sites/drupal8/drupal/vendor/symfony/dependency-injection/ContainerBuilder.php(762): Symfony\Component\DependencyInjection\Compiler\Compiler->compile(Object(Drupal\Core\DependencyInjection\ContainerBuilder))
#3 /home/eirik/Sites/drupal8/drupal/core/lib/Drupal/Core/DrupalKernel.php(1295): Symfony\Component\DependencyInjection\ContainerBuilder->compile()
#4 /home/eirik/Sites/drupal8/drupal/core/lib/Drupal/Core/DrupalKernel.php(897): Drupal\Core\DrupalKernel->compileContainer()
#5 /home/eirik/Sites/drupal8/drupal/vendor/drush/drush/src/Drupal/DrupalKernelTrait.php(69): Drupal\Core\DrupalKernel->initializeContainer()
#6 /home/eirik/Sites/drupal8/drupal/core/lib/Drupal/Core/DrupalKernel.php(818): Drush\Drupal\DrupalKernel->initializeContainer()
#7 /home/eirik/Sites/drupal8/drupal/core/lib/Drupal/Core/Extension/ModuleInstaller.php(579): Drupal\Core\DrupalKernel->updateModules(Array, Array)
#8 /home/eirik/Sites/drupal8/drupal/core/lib/Drupal/Core/Extension/ModuleInstaller.php(211): Drupal\Core\Extension\ModuleInstaller->updateKernel(Array)
#9 /home/eirik/Sites/drupal8/drupal/core/lib/Drupal/Core/ProxyClass/Extension/ModuleInstaller.php(83): Drupal\Core\Extension\ModuleInstaller->install(Array, true)
#10 /home/eirik/Sites/drupal8/drupal/vendor/drush/drush/src/Drupal/Commands/pm/PmCommands.php(90): Drupal\Core\ProxyClass\Extension\ModuleInstaller->install(Array, true)
#11 [internal function]: Drush\Drupal\Commands\pm\PmCommands->enable(Array, Array)
#12 /home/eirik/Sites/drupal8/drupal/vendor/consolidation/annotated-command/src/CommandProcessor.php(257): call_user_func_array(Array, Array)
#13 /home/eirik/Sites/drupal8/drupal/vendor/consolidation/annotated-command/src/CommandProcessor.php(212): Consolidation\AnnotatedCommand\CommandProcessor->runCommandCallback(Array, Object(Consolidation\AnnotatedCommand\CommandData))
#14 /home/eirik/Sites/drupal8/drupal/vendor/consolidation/annotated-command/src/CommandProcessor.php(176): Consolidation\AnnotatedCommand\CommandProcessor->validateRunAndAlter(Array, Array, Object(Consolidation\AnnotatedCommand\CommandData))
#15 /home/eirik/Sites/drupal8/drupal/vendor/consolidation/annotated-command/src/AnnotatedCommand.php(302): Consolidation\AnnotatedCommand\CommandProcessor->process(Object(Symfony\Component\Console\Output\ConsoleOutput), Array, Array, Object(Consolidation\AnnotatedCommand\CommandData))
#16 /home/eirik/Sites/drupal8/drupal/vendor/symfony/console/Command/Command.php(255): Consolidation\AnnotatedCommand\AnnotatedCommand->execute(Object(Drush\Symfony\DrushArgvInput), Object(Symfony\Component\Console\Output\ConsoleOutput))
#17 /home/eirik/Sites/drupal8/drupal/vendor/symfony/console/Application.php(1018): Symfony\Component\Console\Command\Command->run(Object(Drush\Symfony\DrushArgvInput), Object(Symfony\Component\Console\Output\ConsoleOutput))
#18 /home/eirik/Sites/drupal8/drupal/vendor/symfony/console/Application.php(271): Symfony\Component\Console\Application->doRunCommand(Object(Consolidation\AnnotatedCommand\AnnotatedCommand), Object(Drush\Symfony\DrushArgvInput), Object(Symfony\Component\Console\Output\ConsoleOutput))
#19 /home/eirik/Sites/drupal8/drupal/vendor/symfony/console/Application.php(147): Symfony\Component\Console\Application->doRun(Object(Drush\Symfony\DrushArgvInput), Object(Symfony\Component\Console\Output\ConsoleOutput))
#20 /home/eirik/Sites/drupal8/drupal/vendor/drush/drush/src/Runtime/Runtime.php(118): Symfony\Component\Console\Application->run(Object(Drush\Symfony\DrushArgvInput), Object(Symfony\Component\Console\Output\ConsoleOutput))
#21 /home/eirik/Sites/drupal8/drupal/vendor/drush/drush/src/Runtime/Runtime.php(49): Drush\Runtime\Runtime->doRun(Array, Object(Symfony\Component\Console\Output\ConsoleOutput))
#22 /home/eirik/Sites/drupal8/drupal/vendor/drush/drush/drush.php(72): Drush\Runtime\Runtime->run(Array)
#23 /home/eirik/Sites/drupal8/drupal/vendor/drush/drush/drush(4): require('/home/eirik/Sit...')
#24 {main}
Error: Class 'Drupal\commerce_cart\Event\CartEvents' not found in Drupal\commerce_promotion\EventSubscriber\CartEventSubscriber::getSubscribedEvents() (line 16 of /home/eirik/Sites/drupal8/drupal/modules/contrib/commerce/modules/promotion/src/EventSubscriber/CartEventSubscriber.php).
 [warning] Drush command terminated abnormally.

The problem is now that commerce_promotion has a hidden dependency on cart. We should either make it depend on cart, or guard against an unknown class in that event subscriber.

We should also make a test to show the problem, and avoid regressions.

Proposed resolution

Create a patch and tests.

Remaining tasks

Agree on best solution. Create a patch. Commit.

User interface changes

n/a

API changes

n/a

Data model changes

n/a

Release notes snippet

n/a

Issue fork commerce-3150268

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

    Support from Acquia helps fund testing for Drupal Acquia logo

    Comments

    eiriksm created an issue. See original summary.

    eiriksm’s picture

    Status: Active » Needs review
    FileSize
    633 bytes

    Here is at least the most basic fix I could come up with.

    eiriksm’s picture

    Just for the record.

    This also happens on 8.9.0 and the 2.19 release, so its not just because I am such a bleeding edge person ;)

    Now to find out why the tests does not fail, that is weird. Should be picked up by functional tests at least?

    eiriksm’s picture

    Interesting.

    I have to run, so I will get back to my findings, but here is a failing test, and a patch with the test included.

    The last submitted patch, 4: 3150268-test-only.patch, failed testing. View results
    - codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

    Status: Needs review » Needs work

    The last submitted patch, 4: 3150268.patch, failed testing. View results
    - codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

    eiriksm’s picture

    Status: Needs work » Needs review
    FileSize
    1.71 KB

    Hm, let's get rid of the deprecation error at least.

    So weird...

    It seems the autoloader can find the file if I run things within a kernel test (for example), but for me it totally crashes when you either enable the module through the interface or with drush.

    And of course this fails in Travis for installing a project for me, which is why I found out in the first place.

    mglaman’s picture

    Kernel tests autoload all available modules. I think Functional tests do too :(

    Crap, so we used a concrete reference to a constant, which caused things to go 💥

    eiriksm’s picture

    So weird. I even ran the tests with run-tests.sh and my test-only patch consistently fails.

    I did not however run all tests in commerce, and run with concurrency 32, and in the same containers, so I am beginning to suspect something something autoloader apc. Which is not a rabbit hole I am going down today.

    Great if someone else can confirm it fails locally at least. And open to suggestions on making the test fail on drupal ci.

    Still think this is critical, as it does not seem possible to install commerce in a normal way (one being from config)

    eiriksm’s picture

    Kernel tests autoload all available modules. I think Functional tests do too :(

    Yes, but the functional test goes to the module page and enables the module, which should only then contain the active modules in it's autoloader. Hm did not check if maybe running a different webserver not trigger the error

    eiriksm’s picture

    Trying to to run the test as a javascript test, to see if that can trigger the failure?

    eiriksm’s picture

    That didn't work. Let's try to clear the apc cache

    eiriksm’s picture

    ...or maybe clearing the Drupal cache? All of these patches fail locally for me, this is utterly frustrating :)

    eiriksm’s picture

    Another try at making this fail

    amateescu’s picture

    Instead of adding an explicit dependency on commerce_cart or guarding against an unknown class, we could standardize on using event strings instead of class constants, like core will probably do as well: #2825358: Event class constants for event names can cause fatal errors when a module is installed

    I see that both event string and class constants are used throughout Commerce, so it would be good to standardize on a pattern anyway :)

    Also, I'm not sure whether this can be or if it's really worth testing..

    mglaman’s picture

    1. +++ b/modules/cart/src/EventSubscriber/CartEventSubscriber.php
      @@ -39,7 +39,7 @@ class CartEventSubscriber implements EventSubscriberInterface {
      -      CartEvents::CART_ENTITY_ADD => 'displayAddToCartMessage',
      +      'commerce_cart.entity.add' => 'displayAddToCartMessage',
      
      +++ b/modules/checkout/tests/modules/commerce_checkout_test/src/EventSubscriber/CheckoutSubscriber.php
      @@ -12,7 +12,7 @@ class CheckoutSubscriber implements EventSubscriberInterface {
      -    $events[CheckoutEvents::COMPLETION_REGISTER][] = 'onRegister';
      +    $events['commerce_checkout.completion_register'][] = 'onRegister';
      
      +++ b/modules/order/tests/modules/commerce_order_test/src/EventSubscriber/OrderPaidSubscriber.php
      @@ -13,7 +13,7 @@ class OrderPaidSubscriber implements EventSubscriberInterface {
      -      OrderEvents::ORDER_PAID => 'onPaid',
      +      'commerce_order.order.paid' => 'onPaid',
      
      +++ b/modules/payment/tests/modules/commerce_payment_test/src/EventSubscriber/FilterPaymentGatewaysSubscriber.php
      @@ -13,7 +13,7 @@ class FilterPaymentGatewaysSubscriber implements EventSubscriberInterface {
      -      PaymentEvents::FILTER_PAYMENT_GATEWAYS => 'onFilter',
      +      'commerce_payment.filter_payment_gateways' => 'onFilter',
      
      +++ b/modules/product/tests/modules/commerce_product_test/src/EventSubscriber/DefaultVariationSubscriber.php
      @@ -13,7 +13,7 @@ class DefaultVariationSubscriber implements EventSubscriberInterface {
      -      ProductEvents::PRODUCT_DEFAULT_VARIATION => 'onDefaultVariation',
      +      'commerce_product.commerce_product.default_variation' => 'onDefaultVariation',
      
      +++ b/tests/modules/commerce_test/src/EventSubscriber/ReferenceablePluginTypesSubscriber.php
      @@ -15,7 +15,7 @@ class ReferenceablePluginTypesSubscriber implements EventSubscriberInterface {
      -    $events[CommerceEvents::REFERENCEABLE_PLUGIN_TYPES][] = ['onPluginTypes'];
      +    $events['commerce.referenceable_plugin_types'][] = ['onPluginTypes'];
      

      This is in the same module, do we need to change it?

    2. +++ b/modules/log/src/EventSubscriber/CartEventSubscriber.php
      @@ -32,8 +32,8 @@ class CartEventSubscriber implements EventSubscriberInterface {
      -      CartEvents::CART_ENTITY_ADD => ['onCartEntityAdd', -100],
      -      CartEvents::CART_ORDER_ITEM_REMOVE => ['onCartOrderItemRemove', -100],
      +      'commerce_cart.entity.add' => ['onCartEntityAdd', -100],
      +      'commerce_cart.order_item.remove' => ['onCartOrderItemRemove', -100],
      

      This service is only injected if Cart is installed, which means the constant is available

    3. +++ b/modules/promotion/src/EventSubscriber/CartEventSubscriber.php
      @@ -13,7 +13,7 @@ class CartEventSubscriber implements EventSubscriberInterface {
      -      CartEvents::CART_EMPTY => ['onCartEmpty'],
      +      'commerce_cart.cart.empty' => ['onCartEmpty'],
      

      Should we do this, or add a service provider which removes this definition if the cart module isn't installed?

    mglaman’s picture

    Status: Needs review » Needs work

    This issue is showing we're registering a service that depends on a module. We should add a ServiceProvider that removes the definition if Cart is not installed. This way we don't register a useless event subscriber to the container and can still use the constant (I like the constants over strings.)

    amateescu’s picture

    Status: Needs work » Needs review
    FileSize
    1.67 KB

    I did it the other way around, we only register the service if commerce_cart is installed, to match what we do in commerce_log :)

    Status: Needs review » Needs work

    The last submitted patch, 18: 3150268-18.patch, failed testing. View results
    - codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

    millenniumtree’s picture

    I actually patched this with a simple dependency addition in 3168004, before being shown this issue that predates it.

    This issue _WILL_ render your site unusable if you install commerce_promotion before commerce_cart, so someone should choose a solution and stick with it, please. The dependency addition is simple, and why would anyone be using promotion without cart?

    quietone’s picture

    Just ran into this today, on Drupal 8.9.x, Commerce 2.17 while working on Commerce Migrate. I tried to install Promotion from the UI without Cart enabled. Made the site unusable. I could not find a way to recover from it, all drush commands failed and UI just gave 'The website encountered an unexpected error. Please try again later.'

    mglaman’s picture

    +++ b/modules/promotion/commerce_promotion.services.yml
    --- /dev/null
    +++ b/modules/promotion/src/PromotionServiceProvider.php
    

    The module is commerce_promotion so it needs to be CommercePromotionServiceProvider

    mglaman’s picture

    Status: Needs work » Needs review
    FileSize
    1.7 KB

    Renamed file

    mglaman’s picture

    I don't want to commit quite yet. I'm kind of concerned as to why HEAD didn't crash. The Functional and FunctionalJavascript tests should have failed on install.

    mglaman’s picture

    mglaman’s picture

    Well, locally it failed. Here's the full patch with the test and fix from #18 (renamed in #23)

    mglaman’s picture

    Assigned: Unassigned » jsacksick

    Assigning to jsacksick for final sign off.

    jsacksick’s picture

    Status: Needs review » Reviewed & tested by the community

    Tests are passing, I'd say that is ready to be committed!

    • jsacksick committed 54bf7b5 on 8.x-2.x authored by mglaman
      Issue #3150268 by eiriksm, mglaman, amateescu: Ensure commerce_promotion...
    jsacksick’s picture

    Title: Not possible to install commerce_promotion » Ensure commerce_promotion can be installed without the cart module.
    Status: Reviewed & tested by the community » Fixed

    Committed!

    • jsacksick committed 0d250a4 on 8.x-2.x
      Issue #3150268 followup: Commit missing files.
      

    Status: Fixed » Closed (fixed)

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