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
Comment | File | Size | Author |
---|---|---|---|
#18 | 2901692-fielddependency-18.patch | 2.33 KB | tim.plunkett |
#18 | 2901692-fielddependency-18-interdiff.txt | 1.33 KB | tim.plunkett |
#16 | 2901692-fielddependency-16.patch | 1.78 KB | tim.plunkett |
#2 | 2901692-experimental-2.patch | 1.11 KB | tim.plunkett |
Comments
Comment #2
tim.plunkettComment #3
tim.plunkettThis blocks #2884601: Add a Layout Builder to core
Comment #4
tim.plunkettSame as #2785913: StableTemplateOverrideTest does not respect experimental modules and #2745915: StableLibraryOverrideTest does not respect experimental modules
Comment #5
EclipseGc CreditAttribution: EclipseGc commentedSeems straight forward enough to me, RTBC.
Eclipse
Comment #9
star-szrCommitted and pushed 6211b5d7d5 to 8.5.x and 333e99c170 to 8.4.x. Thanks!
Comment #10
xjmWhy must it exclude them?
Comment #11
tim.plunkettIMO this could be reverted and re-discussed for 8.5.x
Comment #14
xjmReverted. 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.
Comment #15
xjmComment #16
tim.plunkettWith 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.
Comment #17
dawehnerI'm curious whether this part is actually needed. Won't the underlying APIs handle this automatically for us?
Comment #18
tim.plunkettThe 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?
Comment #19
dawehnerWell, 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 :)
Comment #20
tim.plunkettComment #21
webchickCool, 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!
Comment #24
dawehnerThis 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
Comment #25
benjifisherDo we need a change record for this?
Comment #26
benjifisherI 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.
Comment #27
benjifisherComment #29
benjifisherSorry, 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.
Comment #30
voleger+1 for RTBC
Patch from #18 looks good for 8.4.x.
Comment #31
xjmI think this is fine for backport. Cherry-picked to 8.4.x. Thanks!