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
StableLibraryOverrideTest attempts to assert that all core modules have their CSS copied to the Stable theme.
It excludes contrib modules, hidden modules, testing modules.
It does not exclude experimental modules, which should NOT have their CSS in Stable.
Proposed resolution
Ignore experimental modules.
Remaining tasks
User interface changes
API changes
Data model changes
| Comment | File | Size | Author |
|---|---|---|---|
| #7 | 2745915-stabletheme-7-to-be-committed.patch | 1.13 KB | tim.plunkett |
| #7 | 2745915-StableLibraryOverrideTest-7-PASS-DO-NOT-COMMIT.patch | 26.38 KB | tim.plunkett |
| #7 | 2745915-StableLibraryOverrideTest-7-FAIL.patch | 25.25 KB | tim.plunkett |
| #2 | 2745915-stabletheme-2.patch | 1.13 KB | tim.plunkett |
Comments
Comment #2
tim.plunkettNot 100% sure how to prove this is working
Comment #3
Wim LeersA test with a hidden experimental module perhaps?
Although then we're testing a test… :P
Comment #4
davidhernandezAdd an experimental module with a library.
getAllLibraries()grabs all libraries, correct? So it shouldn't appear in that group. Does\Drupal::moduleHandler()->getModuleList();only return installed modules? Is that sufficient to check, or do assets actually need to be checked?As as I can tell there is also nothing checking the other module types, so should there be individual checks for all of them? A dummy experimental module, test module, contrib module, hidden module.
Comment #5
tim.plunkettIf we ship with a dummy experimental module that isn't hidden, it will show to all end users of Drupal. Isn't that hugely undesirable?
Comment #6
Wim LeersTim pointed out to me that the existing condition is already checking for hidden modules, and is excluding those. So, we can't write a test with a hidden module, because that'd already work without any changes.
Therefore the only way to prove this works IMO is to write a FAIL test here, but commit #2.
Finally, I want to stress again that writing tests for this would mean writing test coverage for a test. So, removing the issue tag.
Comment #7
tim.plunkettOkay, just reusing the code from #2724819: Create experimental module for place block on any page feature
Comment #10
tedbow#7 Looks good to me. I tested all three patches and got the expected results.
Comment #11
tedbowLet's get this in!
Comment #12
davidhernandez+1
Comment #15
star-szrMakes sense. Committed and pushed bbfb417 to 8.2.x and 9365344 to 8.1.x. Thanks!
Comment #16
star-szr