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.Follow-up from #2073823: [META] Remove drupal_add_js() calls and #2073819: [META] Remove direct calls to drupal_add_css().
When drupal_render() is called, it puts assets into a static. That static is then checked during final page rendering to see which assets should get rendered.
While most places use #attached, any code that calls drupal_render() immediately breaks the #attached chain if there's a call further up the stack.
Couple of things we can do:
1. Add an $attached argument to drupal_render(), passed by reference, that hands back all the $attached assets to the caller filled in. This is possible without an API change, although it's a bit ugly for that reason.
2. drupal_render() could internally collect assets added into a static until that $attached argument is set, at which point it'd clear out the storage. This was suggested by FabianX in prague to allow drupal_render() calls in preprocess without actually breaking render caching. This also would not be an API change.
Comments
Comment #1
amateescu CreditAttribution: amateescu commentedI believe a third option was also discussed: make drupal_render() return an array instead of a string. This would be a pretty big API change though.
Comment #2
catchYes that's also an option, but I'm not sure about it for two reasons:
1. It's a big API change for a relatively small implementation change.
2. It doesn't give us "I'm calling drupal_render() and handling assets" vs. "I'm calling drupal_render() because I didn't port from 7.x properly yet/preprocess is still weird" vs. "I'm calling drupal_render() because I'm drupal_render_children()" - and we'd need to add an optional argument for that.
If we can assume that anything passing $attached as an array is the first of these, we don't need a second optional argument to handle #2.
Comment #3
amateescu CreditAttribution: amateescu commentedI think that's what we were aiming for, but I also agree that this third options is not the most desirable one, I just mentioned it for posterity :)
From the OP, I like option 2) best. But another question comes to mind: do we need to care about the future of (a possible OO) Render API?
Comment #4
catchI don't think we can do #2 unless we do #1 too (at least without changing the return value).
If we fix the preprocess issue, then there should really only be a couple of places where drupal_render() gets called, and we'd not need to store anything - we could more or less stop people from calling drupal_render() anywhere then (or if they do, they're responsible for sorting everything out), then #2 isn't necessary, but right now it's a stop-gap.
For OOP render API, there's two parts of this:
1. Converting drupal_render() itself to a service, I'd be fine with that in 8.3.0 even - we can leave bc layer in but not use it ourselves. I don't think that changes anything here - except where the intermediary storage for $attached might be.
2. Converting render arrays to objects, that means converting FAPI to objects too, whenever that happens there's going to be enough to be concerned about, so I'd not worry about it here.
Comment #5
Crell CreditAttribution: Crell commentedOOPifying the render array definitions doesn't buy us much if anything at this point, and as catch said the knock-on effects would be dramatic. Let's not go there.
Service-ifying the render engine is a generally good thing to do period; it could be done as part of this cleanup or not, depending on what makes life easier.
Another thought: we have HtmlFragment now, which is intended to be able to carry a string of HTML plus associated out-of-band data. (Links, meta tags, css, js.) Why not just use that as the result of a "rendered render array", instead of another array or a split output?
We could, potentially, add a _toString() to it for BC purposes that returns the string body *and* queues up its assets into whatever static we have now?
Also, "collect stuff into a service" is logically impossible since then it's not a service, it's an over-engineered global variable. Please let's not do that.
Comment #6
catchJust using drupal_static() and clearing it out works for collecting internally. However if we end up converting drupal_render() itself to a service, then I'd rather than 'over-engineered global variable' in the form of a class property compared to calling out to drupal_static(), it's not as if we don't have plenty of usages like that already (like all the stuff that ends up on $request).
Either way I don't think we should do any class conversion in this issue - this ought to be a relatively small change that shouldn't get mixed up with a wider refactoring. Will edit the OP to remove any reference to anything modern ;)
Comment #7
catchComment #8
larowlanWould #1762204: Introduce Assetic compatibility layer for core's internal handling of assets help here?
Comment #9
catchI don't think it solves this specific problem, but sdboyer mentioned he's got a proof of concept doing something similar in the removal issues. afaik this issue is exactly in between those two.
Comment #10
catchComment #11
sun@Crell's suggestion of returning a HtmlFragment and adding a __toString() makes most sense to me.
For consistency, the same could be done for the return value of theme().
The only potential drawback I can see is that HtmlFragment would name-wise/semantically hard-code a HTML output format, whereas I believe there are theme functions that do not format HTML. But that's a minor detail, which could be ignored.
Comment #12
steveoliver CreditAttribution: steveoliver commented@sun,
What about calling it
RenderFragment?Comment #13
Crell CreditAttribution: Crell commentedI'd prefer to keep the Render name out of it, because I don't see the HTML-level domain objects (HtmlPage, HtmlFragment, and likely a *small* number of others that we'll be adding) as part of the Render API. I want to keep them distinct, by design. RenderFragment might imply that it's still carrying around a render array, which is absolutely not the intent.
sun: In practice, I don't think we're using render arrays for anything other than HTML. Or if we are it's in really really edge-case situations that we likely could do without. Non-HTML output at this point is mostly handled through the serializer module, which IIRC doesn't use render arrays.
I'm going to try a skunkworks of #5 and see what happens.
Comment #14
sdboyer CreditAttribution: sdboyer commentedComment #15
Crell CreditAttribution: Crell commentedMight this be deprecated, sort of, by #2256365: Factor render->fragment code out to a service? That's basically where my skunkworking from way back ended up...
Comment #16
Wim Leers#2273277: Figure out a solution for the problematic interaction between the render system and the theme system when using #pre_render fixed this! :)
Read #2273277-36: Figure out a solution for the problematic interaction between the render system and the theme system when using #pre_render for details.