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.
We should be testing the CachedStorage class using ConfigStorageTestBase since we no longer use it for the active configuration. This has the added benefit of making #2262861: Add concept of collections to config storages smaller since that introduces changes to CachedStorage that need testing.
Comment | File | Size | Author |
---|---|---|---|
#3 | 2263287.3.patch | 7.42 KB | alexpott |
#3 | 0-3-interdiff.txt | 2.56 KB | alexpott |
cachedstorage-test.patch | 8.5 KB | alexpott | |
Comments
Comment #1
alexpottComment #2
BerdirA few lines above it says that we don't do the invalid data tests?
Why are we even testing something like this? I noticed a lot of weird over-protective code in the CachedStorage that verified that the things we got back from the cache are valid and so on, we don't do that anywhere else...
that's like writing a test that inserts bogus values in the the node table and then tests if the entity storage can deal with that?
Comment #3
alexpottIn IRC @Berdir asked me if the PHPUnit test
Drupal\Tests\Core\Config\CachedStorageTest
covers this. I think we gain consistency since all classes that implement StorageInterface are tested with the same base test. So if we add further test coverage to this (as we do in #2262861: Add concept of collections to config storages) then CachedStorage automatically gets tested.Comment #4
Gábor HojtsyTwo criticals are postponed on this (#2262861: Add concept of collections to config storages and #2224887: Language configuration overrides should have their own storage). The later one is even a beta blocker. So elevating to critical.
Looks to me like all changes proposed by Berdir have been fixed and the patch makes sense to me too :)
Comment #6
catchCommitted/pushed to 8.x, thanks!