Problem/Motivation

KernelTestBase::enableModules() does not enable the dependencies of modules.

FieldDefinitionIntegrityTest attempts to enable all modules that provide a field type.

With the proposed Layout Builder module, this test fails because:
- Layout Builder provides a field type
- Layout Builder has a dependency on Layout Discovery
- Layout Discovery does not provide a field type
- Layout Builder's services.yml uses a service provided by Layout Discovery

This set of conditions happens to not exist yet in core.
It is also impossible to test, since this test explicitly filters out all Testing modules.

Proposed resolution

Ensure that all dependencies of the field-type-providing modules are enabled.

Remaining tasks

N/A

User interface changes

N/A

API changes

N/A

Data model changes

N/A

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett created an issue. See original summary.

tim.plunkett’s picture

Status: Active » Needs review
FileSize
1.11 KB
tim.plunkett’s picture

tim.plunkett’s picture

EclipseGc’s picture

Status: Needs review » Reviewed & tested by the community

Seems straight forward enough to me, RTBC.

Eclipse

  • Cottser committed 6211b5d on 8.5.x
    Issue #2901692 by tim.plunkett, EclipseGc: FieldDefinitionIntegrityTest...

  • Cottser committed 333e99c on 8.4.x
    Issue #2901692 by tim.plunkett, EclipseGc: FieldDefinitionIntegrityTest...

Cottser credited Cottser.

star-szr’s picture

Version: 8.5.x-dev » 8.4.x-dev
Status: Reviewed & tested by the community » Fixed

Committed and pushed 6211b5d7d5 to 8.5.x and 333e99c170 to 8.4.x. Thanks!

xjm’s picture

Why must it exclude them?

tim.plunkett’s picture

Title: FieldDefinitionIntegrityTest does not respect experimental modules » REVERT: FieldDefinitionIntegrityTest does not respect experimental modules
Status: Fixed » Reviewed & tested by the community

IMO this could be reverted and re-discussed for 8.5.x

  • xjm committed 787b7cc on 8.4.x
    Revert "Issue #2901692 by tim.plunkett, EclipseGc:...

  • xjm committed d70110c on 8.5.x
    Revert "Issue #2901692 by tim.plunkett, EclipseGc:...
xjm’s picture

Status: Reviewed & tested by the community » Active

Reverted. Setting active because it's a totally different fix, or a wontfix and a different bugfix.

The other places we skip tests for the experimental package are in Stable tests, and we do that specifically because stable should not contain stuff from test themes.

However, this test is not that. In the case of this test, TIm says he tracked the problem to enabling modules and their dependencies not being respected, at least for the layout_builder issue.

xjm’s picture

Title: REVERT: FieldDefinitionIntegrityTest does not respect experimental modules » FieldDefinitionIntegrityTest does not respect module dependencies
tim.plunkett’s picture

Version: 8.4.x-dev » 8.5.x-dev
Status: Active » Needs review
FileSize
1.78 KB

With the Layout Builder module, this test fails because:
- Layout Builder provides a field type
- Layout Builder has a dependency on Layout Discovery
- Layout Discovery does not provide a field type
- Layout Builder's services.yml uses a service provided by Layout Discovery

This set of conditions happens to not exist yet in core.
It is also impossible to test, since this test explicitly filters out all Testing modules.

The attached patch ensures that all dependencies of the modules are enabled.

dawehner’s picture

+++ b/core/modules/field/tests/src/Kernel/FieldDefinitionIntegrityTest.php
@@ -36,7 +37,15 @@ public function testFieldPluginDefinitionIntegrity() {
+    $modules = array_unique(NestedArray::mergeDeep(array_keys($modules), $dependencies));
+    // Remove any modules already enabled.
+    $modules = array_diff($modules, static::$modules);

I'm curious whether this part is actually needed. Won't the underlying APIs handle this automatically for us?

tim.plunkett’s picture

The APIs are fine, but KTB doesn't use them. It writes to core.extension directly.

This would fix that issue, but might be out of scope?

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

This would fix that issue, but might be out of scope?

Well, especially its basically the core of kernel tests that it actually doesn't install the modules fully, let's not try to fix that :)

Beside of that, I think keeping the unique is fine :)

tim.plunkett’s picture

Issue summary: View changes
webchick’s picture

Status: Reviewed & tested by the community » Fixed

Cool, this seems like a much more holistic fix that's likely to pre-emptively solve other problems that might come up in the course of developing other experimental modules.

Committed and pushed to 8.5.x. Thanks!

  • webchick committed 3a4f6f2 on 8.5.x
    Issue #2901692 by tim.plunkett, xjm, Cottser, dawehner, EclipseGc:...

Status: Fixed » Closed (fixed)

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

dawehner’s picture

Version: 8.5.x-dev » 8.4.x-dev
Status: Closed (fixed) » Needs work

This is needed for #2903161: Fix incorrect FieldFormatter id for "weight" field in base field definition in display options which is a bug we can still fix in 8.4.x, so it would be nice to get this into 8.4.x

benjifisher’s picture

Do we need a change record for this?

--- a/core/tests/Drupal/KernelTests/KernelTestBase.php
+++ b/core/tests/Drupal/KernelTests/KernelTestBase.php
@@ -814,7 +814,7 @@ protected function enableModules(array $modules) {
 
     foreach ($modules as $module) {
       if ($module_handler->moduleExists($module)) {
-        throw new \LogicException("$module module is already enabled.");
+        continue;
       }
       $module_handler->addModule($module, $module_list[$module]->getPath());
       // Maintain the list of enabled modules in configuration.
benjifisher’s picture

I do not see any reason not to apply the patch from #18 to 8.4.x. It applies cleanly, and FieldDefinitionIntegrityTest passes locally. The testbot did not complain when that patch was originally posted, but I will queue it up to try again.

benjifisher’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 18: 2901692-fielddependency-18.patch, failed testing. View results

benjifisher’s picture

Status: Needs work » Reviewed & tested by the community

Sorry, I accidentally retested on 8.5.x as well as 8.4.x. Of course, the patch fails to apply on 8.5.x since it has already been applied.

It still looks good for 8.4.x.

voleger’s picture

+1 for RTBC

Patch from #18 looks good for 8.4.x.

xjm’s picture

Status: Reviewed & tested by the community » Fixed

I think this is fine for backport. Cherry-picked to 8.4.x. Thanks!

  • xjm committed bed6f2e on 8.4.x authored by webchick
    Issue #2901692 by tim.plunkett, xjm, Cottser, dawehner, EclipseGc:...

Status: Fixed » Closed (fixed)

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