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
WebTestBase
does config schema checking for the site under test. BrowserTestBase
should have this too. Per Alex Pott in #2547447-14: Stop creating services.yml by default and -15.
Proposed resolution
Let BrowserTestBase
also check config schema for the installed config.
Remaining tasks
- Add tests.
- Get patch green.
- Review.
- Commit.
User interface changes
None.
API changes
None. Soft behavior change: config schemas will be checked also for BrowserTestBase
tests. Which means broken config will be caught in those tests too.
Data model changes
None.
Comment | File | Size | Author |
---|---|---|---|
#38 | 2553733-38.patch | 8.25 KB | alexpott |
#38 | 34-38-interdiff.txt | 1015 bytes | alexpott |
#34 | 2553733-34.patch | 7.99 KB | alexpott |
#34 | 2553733-34.test-only.patch | 5.2 KB | alexpott |
#32 | 2553733-32.patch | 4.05 KB | alexpott |
Comments
Comment #2
Wim LeersComment #4
dawehnerIs that some logic we can move into a trait and share so?
Comment #5
Wim LeersGood idea!
(If
BrowserTestBase
extendedTestBase
we could put it there. But since it doesn't, a trait is the only option.)Comment #6
dawehnerAnd the better one ... IMHO you also maybe want to validate config on kernel tests?
Comment #7
jibranCan we include this fix in next reroll https://github.com/jibran/drupal/commit/31dd5cc5a01ed863a36299508f1098d9...?
Comment #8
Wim LeersI suspect we should still do this, because it'll catch problems that'd otherwise go unnoticed.
Comment #9
Wim Leers(And adding it after RC would mean breaking contrib projects.)
Comment #10
dawehnerYeah we totally should.
Comment #11
jibranBTB for contrib is kinda a broken #2503547: Contrib can't run Functional tests so I think it is ok after RC as well.
Comment #12
Wim LeersInteresting, thanks for letting us know!
Comment #13
Wim LeersAdded beta evaluation explaining impact vs. disruption wrt #8's added tags.
tag; better justifyingComment #14
xjmWe have a tag for that now. :)
Comment #15
alexpottAs a test infrastructure change this is actually "rc eligible"
Comment #16
dawehnerLet's fix it.
Comment #17
alexpottI was going to say that this is a separate issue - but it is not! This is why config schema checking is a good thing(tm)
The one in KernelTestBase test implies we don't have schema checking turned on there - I think we should have - but that is a separate issue.
I guess we should have a use statement instead of a fully qualified class.
Comment #18
dawehnerI'm like 99.5% sure that at least the new kernel test has schema checking turned on by default. For contrib modules I used kernel tests to ensure that schema is actually there.
Let's go with the variant in
\Component
, in case we ever will make it possible to use the PECL implementation.Comment #20
dawehner.
Comment #22
dawehner.
Comment #23
dawehnerReapply to 8.2.x
Comment #24
alexpottOh wow I'd forgotten about this. This is now a critical regression because we have many more BrowserTestBase tests. :(
Comment #25
alexpottRerolled
Comment #26
alexpottHere's some tests.
Comment #27
dawehnerI guess this should have its own dedicated issue?
Comment #28
alexpott@dawehner agreed...
Comment #29
alexpottCreate #2779459: KernelTestBase thinks private file setting is in config for the KernelTestBase issue.
Comment #30
dawehnerI think this patch totally solves the problems we have, adds test coverage. What else would one want.
Comment #32
alexpottMinor fix to a comment placement. And removal of unnecessary change.
Comment #33
alexpottHmmm... actually I think I should port \Drupal\config\Tests\SchemaConfigListenerWebTest to BTB.
Comment #34
alexpottSo instead of \Drupal\config\Tests\SchemaConfigListenerWebTest I refactored \Drupal\KernelTests\Core\Config\SchemaConfigListenerTest to use a trait and used that.
Comment #36
dawehnerWorks for me.
Comment #37
jibranLet's use class constant here.
Comment #38
alexpottSure why not
Comment #39
catchCommitted/pushed to 8.3.x and cherry-picked to 8.2.x. Thanks!