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.
Remove drupal_add_css() from _drupal_maintenance_theme() - use #attached instead.
Meta issue: http://drupal.org/node/1839338
Comment | File | Size | Author |
---|---|---|---|
#34 | interdiff.txt | 2.95 KB | Wim Leers |
#34 | 2008270-remove-add_css-maintenance-34.patch | 4.37 KB | Wim Leers |
Comments
Comment #1
mcjim CreditAttribution: mcjim commentedPatch removes drupal_add_css() from _drupal_maintenance_theme().
CSS files are attached to the content in template_preprocess_maintenance_page(). Not sure if it's the best approach, but it seems to work :-)
Comment #2
tstoecklerLooks good. I'm not sure about the type-checking, though. In which cases is $variables['content'] already string, and which cases isn't it?
Comment #3
mcjim CreditAttribution: mcjim commentedTBH, it's probably always a string. Think I'm being overly-defensive, there.
Comment #4
tstoecklerOK, can we remove the check then please? Let's just declare $variables['content'] all in one and be done with it.
Whether something has been rendered or not is extremely important information for themers. We always want to render as late as possible to make everything alterable. But for that reason it needs to be 100% clear at which point we have render arrays, and at which point we don't. Therefore - in my humble opinion - these sort of "safety checks", while certainly well intentioned, don't get me wrong, actually hide bugs instead of preventing them.
Comment #5
mcjim CreditAttribution: mcjim commentedThanks for reviewing, @tstoeckler.
Check removed and amended comment to clarify what it is happening.
Comment #6
tstoecklerAwesome!
Comment #7
catchIt's not immediately clear where $variables['content'] gets set - is that $variables[$region] above? What if a theme doesn't have a region named 'content'?
Comment #8
mcjim CreditAttribution: mcjim commented$variables['content'] gets set in the MaintenanceModeSubscriber with this line:
So, hello subscribers. Nice to have met you :-)
So… should I assume we can't depend on this?
Comment #9
tstoecklerRe #8: Thanks for figuring that out! For extra credit, could you go ahead and change that to remove the theme() call altogether? I.e. change that line to:
That would be super cool!
Comment #10
tstoecklerOh, and then we can remove that. I forgot to mention that.
Comment #11
mcjim CreditAttribution: mcjim commentedSuggested approach attached.
Just adding and rendering the CSS within the preprocess.
It doesn't touch MaintenanceModeSubscriber, sorry @tstoeckler, as maybe that should be a separate issue under https://drupal.org/node/2006152? Also: I couldn't make it work for now, but I may be just tired ;-)
Comment #12
Wim Leers#11: 2008270-11-remove-drupal_add_css.patch queued for re-testing.
Comment #14
adammaloneReroll of #11
Comment #15
Wim LeersCan you fix the typo in this line? I'll RTBC then — this is good to go. Thanks! :)
Comment #16
adammaloneThanks Wim - removed the '-' in the latest patch.
Comment #17
Wim LeersGreat, thanks! :)
Comment #18
catchThose aren't added from system_init() any more.
Comment #19
adammaloneRight - they're added in system_page_build()
Rerolled...
Comment #20
catchComment #21
alexpottCommitted 862b4d6 and pushed to 8.x. Thanks!
Comment #23
JohnAlbinI was working on #1839318: Replace drupal.base.css library with normalize.css for all themes when I noticed this issue. Unfortunately, when all the CSS files were moved from _drupal_maintenance_theme() to template_preprocess_maintenance_page() (btw, yay!), the drupal.base.css was accidentally left in the former location. It was added just a few days before the last re-roll, so it was probably overlooked.
Also, we need to make the code in template_preprocess_maintenance_page() more closely match system_page_build() because the weights on these system CSS files
nowstill when on regular vs. maintenance pages. That's probably my fault for not adding the weights to the maintenance versions when I added the weights on the normal side. The big chunk of code in this patch after$default_css = array();
is almost an exact copy of the code snippet from system_page_build().Also, I don't believe system.admin.css should be conditionally added for maintenance pages. We should just add it for all maintenance pages.
Comment #24
Wim LeersOh :( Sorry to have overlooked that!
Are those weights truly necessary? The whole weights system is going away anyway, so if we introduce yet more complex weight handling here, it'll be more to clean up later.
I'll ask nod_ to chime in.
Comment #25
sdboyer CreditAttribution: sdboyer commentedif those css files truly need to precede other things, they should probably be added as a library. we're really trying to kill weights (i got distracted last week, but i'm rounding the corner on a patch that does so).
this does smell like one of those places where the library/deps system isn't particularly well-suited to managing the css cascade, but still - if we can avoid mucking with weights like that, we should.
Comment #26
Wim Leers#23: 2008270-23-fix-maintenance-css-weights.patch queued for re-testing.
Comment #27
catch23: 2008270-23-fix-maintenance-css-weights.patch queued for re-testing.
Comment #29
mr.baileysRe-rolled and removed an unnecessary
$default_css = array();
.Comment #31
mr.baileysTest failures seem unrelated to this patch, and pass locally, so requesting a retest.
Comment #32
mr.baileys29: 2008270-29-drupal_add_css-maintenance.patch queued for re-testing.
Comment #33
vijaycs85Coding standard updates...
Comment #34
Wim LeersIt looks like all patches since JohnAlbin's resurrection of this issue, including John's, actually try to attach the
drupal.base
library that was removed, instead of attaching thenormalize
library that we do have since #1839318: Replace drupal.base.css library with normalize.css for all themes.The difference is that
normalize.css
then gets added to e.g./update.php
, whereas otherwise, it's not.#33 updated the patch to better comply with coding standards. But the thing was that it was a 1:1 copy from
system_page_build()
. So then we should update the code there too.Comment #35
Wim LeersFunction name has changed in the mean time.
Comment #36
andypostAny reason to keep
system_page_build()
at all? Why not move this "attached" into appropriate preprocess functions?So Any reason to duplicate this? Suppose better to add files once in preprocess
Comment #37
Wim Leers#36: because
system_page_build()
runs for all pages (and only addssystem.admin.css
on admin paths, and never addssystem.maintenance.css
), except when maintenance mode is on, in which casetemplate_preprocess_maintenance_page()
runs, and hence we must duplicate analogous logic there (but in that case, always addsystem.(admin|maintenance).css
)?I think you might have missed that subtle detail? :)
Comment #38
nod_After getting sidetracked for a while I ended up testing the patch. Works great.
Comment #39
andypost@Wim no, I mean to separate this into
template_preprocess_maintenance_page()
andtemplate_preprocess_page()
and get rid ofsystem_page_build()
because it could not be executed when hooks(database) are not available, so this will allow get rid of the code duplication and this commentComment #40
Wim Leers#39: Ah, I see! That could work, yes. But: 1) AFAIK the Twig folks are trying to get rid of preprocess functions, 2) that's very much out of scope for this issue :)
Feel free to open a new issue for that though!
Comment #41
benjy CreditAttribution: benjy commentedThis looks good to go, +1 for RTBC.
Comment #42
webchickThis looks consistent with what we've done elsewhere for these patches. andypost's point may be worth a follow-up if the Twig maintainers think it's a good idea, but I agree it's not for this issue to solve how preprocess logic is organized.
Committed and pushed to 8.x. Thanks!
Comment #43
vijaycs85yay!!! just test case fixes (#2157085: Remove remaining external drupal_add_css() references from code) and doc (#2157091: Remove remaining drupal_add_css references from documents) clean up pending to get one meta down #2073819: [META] Remove direct calls to drupal_add_css()
Comment #45
Wim Leers