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.
Working on #1813186: Integrate scripts/stylesheets from info/layout files with libraries I found out that color module only look for CSS files declared in the info file with the stylesheets key. It doesn't look for CSS files that are declared in a library. For example see the bartik changes in #1813186-11: Integrate scripts/stylesheets from info/layout files with libraries.
Comment | File | Size | Author |
---|---|---|---|
#27 | color-module-issue-#2228745-27-test-only-FAIL.patch | 3.41 KB | Wim Leers |
#27 | color-module-issue-#2228745-27.patch | 5.42 KB | Wim Leers |
Comments
Comment #1
Dragan Eror CreditAttribution: Dragan Eror commentedWill check this one...
Comment #2
Dragan Eror CreditAttribution: Dragan Eror commentedComment #3
sqndr CreditAttribution: sqndr commentedAny thoughts on how to fix this one?
Comment #4
nod_Here is a patch, the functionality works but it needs tests.
Comment #6
nod_Comment #7
nod_Comment #9
sqndr CreditAttribution: sqndr commentedGave me a whitespace error. New patch added.
@nod_: What kind of tests does it need? Manual testing or ... If I could help, give a shout! :)
Comment #10
nod_Oh yeah feel free to take it up. Don't have much time for it.
The test needed is adding a new library to color_test_theme (in the color/tests/color_test folder), declare a css file from that library to the color module (in color.inc) and make sure that file gets rewritten properly.
Comment #11
sqndr CreditAttribution: sqndr commented@nod_
: Great, thanks. I'll try this out, assigning to myself.Comment #12
RainbowArrayAny progress on this, sqndr? It would be nice to get this in so we can get stylesheets out of info.yml and just have them in libraries.yml.
Comment #13
sqndr CreditAttribution: sqndr commented@mdrummond: No progress so far. You can pick this up if you want.
Comment #14
undertext CreditAttribution: undertext commentedI have added a new library to color_test_theme and declared a css file from that library to the color module. It seems that current tests are checking file rewriting ( 'Make sure the color stylesheet is included in the content' assertion').
Also list_themes() function is deprecated now. So i use \Drupal::service('theme_handler')->listInfo() in patch.
Should we create a new issue about replacing list_themes() function occurrences ?
Comment #15
nod_probably yes. Although I'm not certain what's the workflow those days.
Comment #16
m1r1k CreditAttribution: m1r1k commentedComment #17
RainbowArrayPart of the reason for this patch is so that theme info.yml files don't have stylesheet attachments inside them anymore. So it would be nice if this patch pulled those declarations out of info.yml. Alternatively that could be handled in a follow-up, but the todo points at this issue for that task.
Other things I spotted:
Is this the way this is being handled elsewhere? Using a global like this doesn't feel like a very d8 way to do this.
Why are some of these 3b3b3b and some 3c3c3c?
Missing newlines at the ends of these files.
Comment #18
tim.plunkett#17.1, #2228093: Modernize theme initialization *just* went in, so see https://www.drupal.org/node/2324935
Comment #19
sqndr CreditAttribution: sqndr commentedComment #20
sqndr CreditAttribution: sqndr commentedHere's a patch - and an interdiff with #14.
Comment #21
sqndr CreditAttribution: sqndr commentedComment #23
joelpittetNot sure if you meant that tag? :p
Comment #24
sqndr CreditAttribution: sqndr commentedHaha - brilliant. Sorry 'bout that :D
Comment #25
RainbowArrayWe still have one for 3b3b3b and two for 3c3c3c. Maybe there is something going on there that I don't understand. But if not, can we get these consistent?
Part of the point of this is that it allows us to no longer have stylesheet declarations in a theme info.yml file. Guessing we will handle that in a separate issue? The @todo points to this issue for that.
Comment #26
andypostNo reason to disturb theme_handler
Comment #27
Wim LeersColorTest
worked as intended before sqndr's changes — it didn't actually test anything forcolor_test_theme
— fortunately, it did test Bartik correctly.libraries
key. That worked, but Color module didn't support that yet.So, restoring the second hunk from #4 to do that again, and things work wonderfully well for Bartik :)
interdiff-lost-hunks.txt
.The test-only patch should have two failures: one for Bartik, one for the test theme.
+1!
Comment #28
Wim LeersBumping to major because this is a blocker to shipping with Bartik's assets in a
libraries.yml
file, and hence it prevents a best practice from being visible in Drupal 8's only front-end theme.Comment #30
nod_Comment #31
nod_Test looks good to me :D
Since the logic hasn't changed You could say wim RTBC'ed that part of the patch. Two halves makes one RTBC patch :p
Comment #32
Wim LeersHeh :)
Comment #33
RainbowArray+1 zillion to this.
Theme sanity, yay!
Comment #34
alexpottCommitted ce7fefd and pushed to 8.0.x. Thanks!
Comment #37
Jeff Burnz CreditAttribution: Jeff Burnz commented#2387535: Color module can't replace CSS files loaded via a library in hook_page_attachments_alter()
Comment #38
YesCT CreditAttribution: YesCT commentedthere was only one issue (this one with Developer Experience tag. I think DX (Developer Experience) is the one being used mostly.