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.There are still some situations where modules will call drupal_add_js() and drupal_add_css() in hook_init(). I think the current guidelines say use #attached when you only want to add css/js to a specific element, use .info for things that appear on nearly every page, and use hook_init() for something like css that will only appear for a certain role.
However, hook_init() is still not a good choice here, since it is called during every full bootstrap, so we're adding css to 'pages' like autocomplete, xmlrpc and image derivatives which will never actually be rendered. These are all callbacks where performance is critical, and where they're only usually 1-200ms.
Instead we should find a way so that this code only runs when actually serving a full page - either by adding #attached in hook_page_alter() instead, or some other hook. Would then need to update core modules that still do this to use that, along with updating the documentation.
| Comment | File | Size | Author |
|---|---|---|---|
| #13 | drupal8.init-assets.13.patch | 6.83 KB | sun |
| #7 | drupal8.init-assets.7.patch | 4.97 KB | sun |
Comments
Comment #1
mattyoung CreditAttribution: mattyoung commentedSub
Comment #2
moshe weitzman CreditAttribution: moshe weitzman commentedI think we need a $page object in D8 and it will have #attached for this. The $page object could come along with Butler or perhaps in some other effort if Butler never materializes.
Comment #3
Anonymous (not verified) CreditAttribution: Anonymous commented@moshe - i'd prefer $request and $response, as there are types of requests like catch lists above where $page is not a useful concept.
Comment #4
idflood CreditAttribution: idflood commentedsubscribing
Comment #5
nod_tag
Comment #6
sunRelated: #1861676: Remove stylesheets[] and scripts[] .info file property support for modules
Comment #7
sunTechnically, the most appropriate place would be
template_preprocess_html(), because JS/CSS only applies to HTML pages.That would make the assumption that everything that goes through
drupal_render_page()results in HTML output, and everything that does not preemptively ends the page rendering process and does not go through that function.Page/block Response objects most likely do not really help here, since the cases we're talking about are unconditionally adding JS/CSS to all page responses (i.e., they are not the controllers/handlers that produce a particular page response; they want to hook into all html responses).
Thus, I just wanted to propose to move all of the calls from
hook_init()tohook_preprocess_html()as a simple first step, but the callers to drupal_add_js() and drupal_add_css() reveal that there are only two of such calls in dblog_init() and system_init(). All others have been converted to system_add_module_assets() already, and in #1861676: Remove stylesheets[] and scripts[] .info file property support for modules, I propose to convert almost all of them into contextually/conditionally added CSS.Anyway, attached patch performs this change as a simple step forward.
Comment #8
moshe weitzman CreditAttribution: moshe weitzman commentedYour patch actually moves the attaching to system_page_build() which is exactly where I wanted it! I think thats totally fine. Is RTBC IMO when bot comes back green.
Comment #10
sunHm. Not sure whether that's a random test failure or not, trying again.
Comment #11
sun#7: drupal8.init-assets.7.patch queued for re-testing.
Comment #12
nod_Green and real nice patch :) I guess the docs needs updating/change notice?
Comment #13
sunGood point — updated the API docs accordingly.
I was not able to find a sensible replacement example code snippet for
hook_init()that wouldn't inherently suggest/promote bad coding practices, so I left it empty. I looked through the remaining, existing hook_init() implementations in core, but pretty much all of them should actually use a better place/hook to achieve what they are doing in D8. AFAICS,hook_init()only remains for exactly what it documents, "set up global parameters that are needed later in the request." - in other words: it's the entry point to apply and execute hacks. (or "customizations", if "hacks" sounds too offending)Comment #14
moshe weitzman CreditAttribution: moshe weitzman commentedReady to fly.
A related issue that I'd love to see go in is #1762204: Introduce Assetic compatibility layer for core's internal handling of assets
Comment #15
catchOn hook_init(), I've got #1833442: Remove hook_boot() and #1860026: Remove hook_exit() for cached page requests open already. We might need at least a clarification issue for hook_init() as well.
Patch looks good to me.
Comment #16
catchCommitted/pushed to 8.x, thanks!
Comment #18
Wim LeersOMG OMG OMG! How did I *not* know about this?
Thank you, sun, for working on this. I'm so glad that this is done!
Next up: only add filter modules' CSS files if the filter is actually used on the page? :)
Comment #19
moshe weitzman CreditAttribution: moshe weitzman commentedWim - that optimization can be harmful. It causes a new aggregated style sheet to be downloaded as you probably know.
Comment #20
Wim Leers#19: Great remark. But IMO the real answer is to fix our aggregation, not to add bloat on every single page load because our aggregation system isn't smart enough.
Comment #21
moshe weitzman CreditAttribution: moshe weitzman commentedI was thinking of http://drupal.org/project/bundlecache when I wrote my comment and now I see that Wim is the current maintainer. Fix core for us, Wim!
Comment #22
Wim LeersI'm the maintainer, I intended to maintain it and push it forward, then the Facebook internship happened. That being said, it's being addressed in the Assetic and "out of band assets" etc. issues. There's so many of these resource/asset handling issues now. I hope it will still happen — I don't have the bandwidth for it while working on Spark :/