| Project: | Drupal core |
| Version: | 7.x-dev |
| Component: | base system |
| Category: | feature request |
| Priority: | normal |
| Assigned: | moshe weitzman |
| Status: | closed (works as designed) |
Issue Summary
drupal_render() currently expects all its passed in elements to have already done their hard work to set #value and optionally #theme. If a later stage of page building unsets that element or changes #access to FALSE then the hard work was for nothing. This patch adds #value_callback support to drupal_render(). This is not a new property - fapi already supports it but drupal_render() curiously does not. Lets fix that.
This patch illustrates the feature by changing #markup=comment_render() to #value_callback='comment_render'. Thus, comment queries are not performed when we are building the node but rather when we are rendering it. This is a big difference once #351235: hook_page_alter() lands. Further, we can safely remove the if() condition around the element and add its logic to #access. Thus we always include this element since it no longer incurs any performance penalty. Including the element is nice as page builders can more easily alter something that exists!
Obviously we'll want to use #value_callback in many more places in the codebase.
With this patch, page building becomes a time when we setup the instructions for building and rendering the page. We do minimal real work here. It is not until the final drupal_render() call when we incur DB queries and unserializes and so on. This gives maximum flexibility to page builders and themers while preserving performance.
Another benefit of not performing real work until the preprocess layer (assumes #351235: hook_page_alter() has landed) is that #value_callbacks can be altered depending on the final output format for the page (JSON, RSS, etc.). This benefit is not fully cooked in my head but I think it is promising.
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| value_callback.patch | 2.75 KB | Idle | Passed: 9326 passes, 0 fails, 0 exceptions | View details | Re-test |
Comments
#1
While I'm a big fan of migrating to callback based systems that delay the hard work until as late as possible, I'm concerned about this one. #value_callback isn't really the same as a rendering tool -- the only reason that it appears to be applicable for form elements is the blurring between #value and #markup that was actually eliminated in the D7 codebase.
In the case of FormAPI, we need to ensure that the #value stuff happens way before the rendering process, because in some cases the value must be processed and used in processing when the form will never even be rendered.
Maybe I'm misunderstanding where this happens?
#2
I like this idea a lot, if we want to keep the separation between #value and #markup then would #markup_callback work? (or #render_callback or whatever).
#3
if (isset($elements['#value_callback']) && drupal_function_exists($elements['#value_callback']))huh that is not enough by far...if (!isset($elements['#value'])) && empty($elements['#input']) && isset($elements['#value_callback']) && drupal_function_exists($elements['#value_callback']))is what I would like too see. Then I will like this one.#4
Great feedback. I have renamed ot #markup_callback and expanded the if() as chx suggests. #markup callback gets rid of the confusion that Eaton highlights so I think all concerns are addressed in this patch.
Block bodies are good candidate for #markup_callback after #351235: hook_page_alter() lands.
#5
#6
no patch. Will review soon.
#7
Woops. Patch attached.
#9
a few more nitpicks,
&& (bool)menu_get_object()the&&already casts to bool. Second, you want this as the last to avoid none less than a function call during node preview. :) (told you: nitpick)#10
I'm looking over the details of the patch, and while I do support the goal -- delaying potentially costly information retrieval until the #access callback is evaluated -- I'm trying to consider the impact that it has on the complexity of our already tangly workflow.
Wouldn't the #pre_render callback give us the same effect? Also, it might make sense to try to piggyback this onto #355236: Refactor drupal_render theming - docs -- that patch simplifies drupal_render's internals considerably and might make some of these optimizations a bit less special-casey...
#11
I'm withdrawing this patch for now. Comment thread rendering should move to hook_page_alter() anyway.
#12
I haven't reviewed this patch very closely yet, but I am not sure if this is really what we want. The general trend is moving away from hard-coded #markup properties towards keeping as much information as possible in the elements themselves and moving markup generation to a later stage (theming layer). So while this patch could improve some things in the short term, once we e.g. make comment rendering return a structured array of comments instead of a rendered #markup string, it couldn't take any advantage of this patch anymore, right?
So moving stuff to hook_pager_alter sounds great. Another idea I got when skimming this patch is something like a #render_callback that gets called by drupal_render with the element as an argument, giving the callback the possibility to add the actual data at render time. I am still not sure about that, though. Moving things to hook_page_alter or other appropriate later-stage alter callbacks, but keeping the final array to a state a la "what's in here should be printed" might make more sense and keep things easier and simpler.
#13
@Frando, #pre_render basically is a 'render_callback' as Eaton mentioned. I agree that this patch is not a good move right now.