Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Problem/Motivation
Removing the Workspaces module for the 8.6.0 alpha broke HEAD, because of this workaround code in ConfigImportAllTest:
// Delete all terms.
$terms = Term::loadMultiple();
entity_delete_multiple('taxonomy_term', array_keys($terms));
// Delete all filter formats.
$filters = FilterFormat::loadMultiple();
entity_delete_multiple('filter_format', array_keys($filters));
// Delete any shortcuts so the shortcut module can be uninstalled.
$shortcuts = Shortcut::loadMultiple();
entity_delete_multiple('shortcut', array_keys($shortcuts));
// Delete any workspaces so the workspace module can be uninstalled.
$workspaces = Workspace::loadMultiple();
\Drupal::entityTypeManager()->getStorage('workspace')->delete($workspaces);There is also this a few lines later:
$this->assertTrue(isset($modules_to_uninstall['comment']), 'The comment module will be disabled');
$this->assertTrue(isset($modules_to_uninstall['file']), 'The File module will be disabled');
$this->assertTrue(isset($modules_to_uninstall['editor']), 'The Editor module will be disabled');
Ideally we would not hardcode the existence of any particular optional module in the config import test. In particular, there should not be references to experimental modules outside that module's directory.
Proposed resolution
TBD
Remaining tasks
User interface changes
API changes
Data model changes
| Comment | File | Size | Author |
|---|---|---|---|
| #5 | 2986464-5.patch | 2.97 KB | alexpott |
| #5 | 4-5-interdiff.txt | 965 bytes | alexpott |
| #4 | 2986464-4.patch | 2.98 KB | alexpott |
Comments
Comment #3
alexpottThe only real option here is would be to not include experimental modules in the modules that are installed by the test. Atm it installs all core provided modules. If those modules do things in their
hook_install()that make them uninstallable then the test will fail. However if we don't install experimental modules in this test we miss important test coverage of them being installed via ConfigImport. I guess in an ideal world we could invert this and make such modules provide some code that allows them to be uninstalled.Or another way would be to remove all the content entities supplied by modules that are going to be uninstalled... going to write a patch to do that.
Comment #4
alexpottHere's a way that the necessary content and config entity deletes are a bit more generalised and if we add to the list of config entity deletes in the future wouldn't break in the same way.
Comment #5
alexpottStandard is never going to provide an entity type. (me thinks never say never but really hopefully in this case).
Comment #6
tstoecklerSo one way to fix this that I've thought about before is to not have a single test install and uninstall all modules but rather have something like
ModuleTestBasethat we extend from every single module that provides baseline coverage such as installing and uninstalling the module. I think the Rest test coverage has set a great example for this. Apart from the concrete benefit of fixing this issue, this would bring proper encapsulation and furthermore would allow contrib modules to benefit from this test coverage as well. We could then add/move additional tests to this base class to improve and define what we consider the "base line" functionality of a module, e.g. we could also move the testing of ahook_help()implementation there.Comment #7
catchWhile this would be cleaner, the 'do everything at once approach' in this test has caught several bugs because it's such a horrible blunt instrument, that we'd never have thought to add explicit coverage for in the first place.
I think #5 is absolutely fine to fix the HEAD breakage, we can open a follow-up for other approaches if we want to.
Comment #8
phenaproximaRead the patch, and it makes perfect sense to me. +1 RTBC.
Comment #9
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedUninstalling a module which provides content entities is now possible to do through the UI, thanks to the dedicated form for deleting all the entities of a certain type:
admin/modules/uninstall/entity/<entity_type_id>I think a better way to test that module are actually uninstallable through the UI, like this mega-test tries to prove, is to delete the existing content entities by using that form for each content entity type that has data instead of deleting them via API calls.
Comment #10
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedOops, @alexpott pointed out that purpose of this test is not to test module uninstalling via the UI, so ignore #9 :)
Comment #11
alexpott@amateescu I'm not sure that this test is testing that modules are uninstallable via the UI per se. It's really testing that is a module is uninstallable it can be uninstalled via configuration import (which happens to occur via the UI).
Comment #14
catchCommitted 92f3fe1 and pushed to 8.7.x, cherry-picked to 8.6.x. Thanks!
Had to fix a cherry-pick conflict on 8.6.x due to the hotfix, so we should allow tests to clear before doing anything further.