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
Two core configuration schema tests (DefaultConfigTest and TestViewsTest) validate all modules' config/install/ and test_views/ entities, including contributed modules outside core. This happens without installing these modules or even checking their dependencies. Two problems with that:
- If a module's dependencies are not available (which is a reasonable outcome of installing a package A that contains an optional submodule depending on B, but not installing B), then it might contain configuration objects whose schemas aren't available.
- A module (or one of its dependencies) can implement
hook_config_schema_info_alter()to modify schemas. Since the modules aren't installed while testing schemas, this hook isn't invoked, and the configuration is checked against the unaltered schema.
Symptoms
We're getting test failures on a site that has paragraphs, but not search_api.
The problem appears to be that paragraphs (whose main module does not depend on search_api) includes a "paragraphs_demo" submodule which depends on search_api. Even though the module should be ignored for its unfulfilled dependencies, its configuration files are still tested for schema errors. Without the search_api module, this causes its search_api-dependent configuration to fail testing.
We also get a test failure of Drupal\Tests\views\Kernel\TestViewsTest on ctools, because its submodule ctools_views installs views that use its own altered schema (adding a few extra display options). By checking these views against the unaltered schema, we get a false positive test failure.
Steps to reproduce
Proposed resolution
#10 suggests limiting the tests to just core modules
#16 suggests making the majority of this test could be some kind of base class, then contrib could just extend it and get a free checkup.
Remaining tasks
Decide on direction
Patch
Review
Commit
User interface changes
API changes
Data model changes
Release notes snippet
| Comment | File | Size | Author |
|---|---|---|---|
| #29 | 2834250-nr-bot.txt | 1.18 KB | needs-review-queue-bot |
| #19 | drupal-2834250-19-exclude-noncore.patch | 1.89 KB | cburschka |
| #12 | drupal-exclude-noncore-2834250.patch | 1.47 KB | cburschka |
Comments
Comment #2
cburschkaComment #3
cburschkaUnfortunately, while the most obvious solution would be to check the config entity's dependencies before trying to test its schema, we don't know that it is a config entity unless the schema exists. Therefore, the only option would seem to be to skip the uninstallable modules' config/install folder entirely.
Comment #4
cburschkaThe scope of this is a bit bigger than that.
In Drupal\Tests\views\Kernel\TestViewsTest, for example, all views are tested against schemas, but without installing their modules. With a module like ctools_views, which alters the schema, this means that the tests fail because the view doesn't match the unaltered schema.
Comment #5
cburschkaComment #6
cburschkaActually, installing the whole module would be a lot of overhead, especially since the test is intended to check all available modules. It also introduces the potential for many unrelated errors that occur during installation.
It would be enough to do two things for every module before checking its configuration:
1. Check that all of its dependencies are available (otherwise skip it, because uninstallable modules may contain configuration with unavailable schemas)
2. Create a copy of the schema data, and pass it through the module's (and all of its dependencies') hook_config_schema_info_alter() hooks.
Comment #7
cburschka(Should probably be in Drupal\config\Tests\SchemaCheckTestTrait, to cover the dozens of tests that check schemas for various configuration entities.)
Comment #8
cburschkaComment #9
cburschkaComment #10
alexpottPerhaps we should just limit the test to core modules only - that'd make it always pass but potentially less useful to contrib and real sites.
Comment #11
cburschkaI suppose that would be justifiable - core unit tests are designed to test core, after all, so checking contributed modules could be considered an unintended side effect.
In that case I'd suggest creating a suite of tests that run all the schema checks (DefaultConfigTest, TestViewsTest, etc.) for a specific contrib module, and maybe include it when the DrupalCI Test Runner tests contrib projects.
(Because the lack of contrib schema testing on d.o. is a separate but related issue: #2834172: Run DefaultConfigTest for contrib modules.)
Comment #12
cburschkaThis patch excludes extensions outside core in TestInstallStorage, so contrib modules will no longer cause the core schema tests to fail.
I'm not really satisfied with the implementation, but TestInstallStorage uses the regular ExtensionDiscovery class, which cannot be told to exclude non-core folders. Filtering the extension object by path name seems to be the next best option.
This will also make
Drupal\config_test\TestInstallStorageunusable for contrib testing, and any code that needs to include contrib will need to recreate the class. It's a very small class though.Comment #13
alexpott@cburschka I'd prefer to change the test to exclude non-core modules rather than this code. Elegant fix though :)
Comment #14
cburschkaThat could be the better approach. It looked a bit daunting as there are 38 different test classes that check schemas, but actually DefaultConfigTest and TestViewsTest are the only ones that use TestInstallStorage to scan the whole site (including contrib) for configurations.
The rest all limit themselves to explicitly listed modules - like BlockConfigSchemaTest, which installs the 11 block-defining core modules by name before checking their blocks' schemas.
Comment #15
cburschkaComment #16
Mile23If the majority of this test could be some kind of base class, then contrib could just extend it and get a free checkup.
Comment #19
cburschkaRerolled for 8.4.x (short array syntax).
Comment #26
quietone CreditAttribution: quietone at PreviousNext commentedThis is a bug smash initiative triage fortnightly target.
Updated the IS and leaving at NR for discussion of the direction.
Comment #29
needs-review-queue-bot CreditAttribution: needs-review-queue-bot as a volunteer commentedThe Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".
Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.
Comment #30
andypostOne more issue found in #3376339-15: Remove Tour module from the Umami profile
For tour removal the config is moved to tour module but
DefaultConfigTestcan't install demo_umami (as not a module), so added todo to remove here when profile dependency can be resolved in the testComment #31
andypost