As follow-up to #2273277: Figure out a solution for the problematic interaction between the render system and the theme system when using #pre_render and within discussions of #2226493: Apply formatters and widgets to Node base fields it got clear that drupal_render() usage is very unclear at the moment, especially to the 2nd parameter.
Current comments say to always pass:
drupal_render($x, TRUE);
when calling from preprocess or things will break with #post_render_cache.
I now wonder: If the recursive case is safe by default, but the other case is not, why not make, recursive the default and ensure our ROOT drupal_render call uses FALSE?
That also ensures that if someone really wants to render out-of-band data they could theoretically call:
drupal_render($x, FALSE) to _not_ have it included in cache tags, etc.
which makes a lot of sense IMHO (and was an open question to myself till now).
This would be a follow-up of course as currently we only support only one stack if I see this correctly, but just generating a new stack should work fine for the FALSE case.
- Make $is_recursive = TRUE the default option for drupal_render
- Discuss
Comments
Comment #1
fabianx commentedComment #2
fabianx commentedComment #3
fabianx commentedComment #4
wim leersI've been wondering the same lately.
Hence: +1.
Comment #5
fabianx commented@Wim: For this to create a proper patch, I now wonder:
Where is our root drupal_render() call at the moment in the code base?
Once I know that I will update issue summary and tag this novice, because then it will be trivial to do.
Comment #6
wim leersExcellent question. There is no clear answer because of #2327277: [Meta] Page rendering meta discussion.
But roughly speaking, each page "supra-region" (top, bottom, and … "the rest") gets a root call.
Comment #7
effulgentsia commentedWhat about removing the parameter, and internally equating $is_recursive with !$stack->isEmpty()?
We may have some places where that's incorrect, but I think all such places are bugs that we should fix.
Comment #8
effulgentsia commentedNote that in addition to #6, we also have other root callers scattered around places like authorize.php, contact_mail(), and others. Potentially, all those places should get fixed as part of #6 as well, but in the meantime, I think #7 would be more robust.
Comment #9
effulgentsia commentedOr, we could even leave the parameter in (for now), but instead of defaulting it to TRUE or FALSE, default it to
!$stack->isEmpty(). Then we can hunt down all places that need to override that default for some reason, and figure out what to about those cases.Comment #10
fabianx commentedI like #7.
I still think we should support calling it with FALSE to create a new stack to allow out-of-render rendering, but that is definitely follow-up.
Comment #11
wim leersI'd love that. But it won't work.
… okay. Let's try :) I really hope it will turn out to be feasible. That'd be one very nice clean-up indeed!
I would LOVE to get rid of that second parameter for
drupal_render()— it's atrocious. But I fear this patch will run into old/crazy/wrong usages ofdrupal_render()that will be very, very hard to fix.Let's try it! :)
Comment #12
wim leersWe're now continuing this in #2361681: drupal_render(): invert second argument ($is_recursive_call -> $is_root_call) => more strict, better DX/TX. I was unable to find this issue when I opened that one. This discussion has been taken into account.