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
Comment | File | Size | Author |
---|---|---|---|
#26 | not_possible_to_inst-3150268-26.patch | 2.97 KB | mglaman |
| |||
#25 | not_possible_to_inst-3150268-25.patch | 2.97 KB | mglaman |
#25 | interdiff-3150268-23-25.txt | 2.41 KB | mglaman |
#23 | not_possible_to_inst-3150268-22.patch | 1.7 KB | mglaman |
| |||
#18 | 3150268-18.patch | 1.67 KB | amateescu |
Issue fork commerce-3150268
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:
Comments
Comment #2
eiriksmHere is at least the most basic fix I could come up with.
Comment #3
eiriksmJust 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?
Comment #4
eiriksmInteresting.
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.
Comment #7
eiriksmHm, 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.
Comment #8
mglamanKernel 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 💥
Comment #9
eiriksmSo 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)
Comment #10
eiriksmYes, 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
Comment #11
eiriksmTrying to to run the test as a javascript test, to see if that can trigger the failure?
Comment #12
eiriksmThat didn't work. Let's try to clear the apc cache
Comment #13
eiriksm...or maybe clearing the Drupal cache? All of these patches fail locally for me, this is utterly frustrating :)
Comment #14
eiriksmAnother try at making this fail
Comment #15
amateescu CreditAttribution: amateescu as a volunteer commentedInstead 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..
Comment #16
mglamanThis is in the same module, do we need to change it?
This service is only injected if Cart is installed, which means the constant is available
Should we do this, or add a service provider which removes this definition if the cart module isn't installed?
Comment #17
mglamanThis 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.)
Comment #18
amateescu CreditAttribution: amateescu as a volunteer commentedI did it the other way around, we only register the service if
commerce_cart
is installed, to match what we do incommerce_log
:)Comment #20
millenniumtreeI 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?
Comment #21
quietone CreditAttribution: quietone as a volunteer commentedJust 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.'
Comment #22
mglamanThe module is commerce_promotion so it needs to be CommercePromotionServiceProvider
Comment #23
mglamanRenamed file
Comment #24
mglamanI 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.
Comment #25
mglamanOkay here is a failing patch!
Comment #26
mglamanWell, locally it failed. Here's the full patch with the test and fix from #18 (renamed in #23)
Comment #27
mglamanAssigning to jsacksick for final sign off.
Comment #28
jsacksick CreditAttribution: jsacksick at Centarro commentedTests are passing, I'd say that is ready to be committed!
Comment #30
jsacksick CreditAttribution: jsacksick at Centarro commentedCommitted!