drupal_render() should not set default element properties when #type is not known

+++ includes/common.inc
@@ -4429,14 +4428,9 @@ function drupal_render(&$elements) {
+    $element['#defaults_loaded'] = TRUE;

s/element/elements/

We need benchmarks for this. I see that it's 1 function call less, but also 1 function call more. ;)

This review is powered by Dreditor.

.

This looks RTBC to me and seems to be totally required for generic alterations/customizations.

I'd like moshe to sign this off. I'll ping him.

Seems reasonable to me too. Ideally, eaton or frando will chime in as well.

Yeah, this is definitely a nice little cleanup, and it moves some unnecessarily twisty special-casing from drupal_render().

I see this is tagged "Performance." Where are the benchmarks?

Here's the same patch as #9, but with the part where #defaults_loaded is set to TRUE commented out. This to better benchmark compare apples to apples with HEAD, which never sets #defaults_loaded to TRUE. I think doing so is a good idea, and should be done as a follow-up issue, but commenting that out for now helps us evaluate performance implications of what this patch is really about separately from evaluating performance implications of that other cleanup.

I'm also attaching a micro-benchmarking script (download to drupal root, rename extension to .php, and visit its url in the browser) that measures the time it takes to invoke drupal_render() when #type is set and when it's not. Here's the results on my computer (running MAMP, so not a production environment) (number is micro-seconds per invocation):

            drupal_render(array('#markup' => 'hello'))    drupal_render(array('#type' => 'markup', '#markup' => 'hello'))
HEAD        44.3685030937 +/- 0.128201156181	             52.4365520477 +/- 0.197194607909
Patch 14    49.6815776825 +/- 0.203881657519	             51.8069005013 +/- 0.159546372122

So, we have a tiny boost in performance when rendering elements with a type and a small loss in performance when rendering elements without a type (as to be expected given what this patch allows for elements without a type). However, what page within Drupal do we have where we're rendering hundreds or thousands of elements without a type, which is the only place where this loss would turn into anything noticeable?

Also, I think this patch is a great idea, and if these numbers are too discouraging, I think it would be good to generate them on a production environment, where the difference might be much smaller.

This looks good to me with one exception. There's no reason to inline element_basic_defaults(). With the static variable we're not calling the function repeatedly but only once per request, which is fine, but with this patch we are no longer able to access the defaults from anywhere except within drupal_render(). That's a feature regression, and nominally API change.

Let's not do that part unless there are benchmarks on that change specifically, but the rest looks good to me.

@Crell: as you point out, there's no performance difference between leaving or losing the element_basic_defaults() function. However, with this patch, even though that function has been removed, you can still get the defaults with element_info('defaults'). The reason element_basic_defaults() has been removed is so that module_invoke_all('element_info') and drupal_alter() could be called from a single place. Would it make sense to have an element_basic_defaults() function that also calls these? If not, would there be a use-case for calling element_basic_defaults() that returns something that hasn't gone through those steps? Set back to CNW if you feel that function is still needed.

+++ includes/common.inc	25 Oct 2009 05:04:08 -0000
@@ -4671,14 +4670,11 @@ function drupal_render(&$elements) {
+  if (empty($elements['#defaults_loaded'])) {
+    $elements += element_info(isset($elements['#type']) ? $elements['#type'] : 'defaults');
+    // @todo: We probably want to set #defaults_loaded here, but need to
+    // evaluate performance impact of doing so.
+    //$elements['#defaults_loaded'] = TRUE;
   }

Can we clarify that comment a bit, please? I don't understand what it's asking for.

+++ modules/image/image.admin.inc	25 Oct 2009 05:04:08 -0000
@@ -818,7 +818,7 @@ function theme_image_anchor($variables) 
-    unset($element[$key]['#title']);
+    $element[$key]['#title'] = '';
+++ modules/simpletest/simpletest.pages.inc	25 Oct 2009 05:04:09 -0000
@@ -139,7 +139,7 @@ function theme_simpletest_test_table($va
-      unset($test['#title']);
+      $test['#title'] = '';
+++ modules/system/system.admin.inc	25 Oct 2009 05:04:09 -0000
@@ -2307,7 +2307,7 @@ function theme_system_modules_fieldset($
-    unset($module['enable']['#title']);
+    $module['enable']['#title'] = '';

Why are these changes required for this patch?

This review is powered by Dreditor.

With sun's feedback. I removed the todo. See comment #14. Setting #defaults_loaded there was something introduced in earlier versions of this patch, and doing that makes sense to me, but should be a separate issue. No need for this patch to even mention it in the code -- let that be a separate issue.

Regarding those other changes, I suspect they are needed if setting #defaults_loaded to TRUE, but since we're not doing that in this patch, let's get rid of them, and keep this patch simple.

I've been looking into why the extra overhead added by invoking element_info() and it's because of drupal_static(). I've been working on an idea to speed that up and will open an issue for that separately, because it affects performance in a few places, not just here. I don't think we should hold this patch up because of that, however, unless someone comes up with a use-case where an extra 5 microseconds per renderable element that has no type would make any noticeable difference.

Ah ok, those extra hunks that were being asked about in #17 was a result of HEAD still adding basic defaults, even if #defaults_loaded has been set by form_builder(). The above patches tried to change that, which seems like a good clean-up to me, but not for this issue. Here's a patch that really minimizes what's done by this patch. I'd like to see follow-up issues resolve the #defaults_loaded situation but I'm trying to remove the bottlenecks from this issue landing. This patch also includes the optimization to drupal_static() I referred to in #18. What do you all think of it? There's other places that can use it. Code that doesn't need this level of micro-optimization can still use the simpler way of invoking drupal_static(), but this patch allows functions that get called hundreds of times per page request to still have resettable statics without needing to suffer an extra function call overhead on each iteration. And as a result, this patch has no performance impact (micro-benchmarking script from #14):

            drupal_render(array('#markup' => 'hello'))    drupal_render(array('#type' => 'markup', '#markup' => 'hello'))
HEAD        44.4458723068 +/- 0.419469128068	             52.654504776 +/- 0.613375405947
Patch 20    44.7358846664 +/- 0.354848815853	             52.4873733521 +/- 0.594166154995

Ugh. No, the global is an absolute no-go. You can read the original drupal_static() issue to learn more.

I'll have a crack at this now.

This already contains the performance considerations from #602522: Links in renderable arrays and forms (e.g. "Operations") are not alterable.

Fixing the notices.

And another notice bites the dust.

Side issue in response to #21: #620452: Optimize drupal_static(): we shouldn't sacrifice simpletest coverage for performance and vice versa

so...? Any objections to #26?

It works for me. As I mentioned in #620452: Optimize drupal_static(): we shouldn't sacrifice simpletest coverage for performance and vice versa, I'm not completely thrilled with non-resettable statics, because they limit the ability of simpletests to change which modules are enabled or what the active theme is between tests within the same page request. However, most simpletests work by invoking a new page request anyway, and we're already implementing non-resettable statics in places where performance matters (theme(), url()), and this is a place where performance matters, so it makes sense. I'll keep working on making the case for #619666: Make performance-critical usage of drupal_static() grokkable, and if that ever lands, then we can replace all non-resettable statics with fast resettable ones. But until then, I think this patch is great.

Great.

In the meantime, I'm _entirely_ questioning those basic defaults for stuff that has no #type. However, that I want to tackle in a separate issue - which, of course, depends on this patch being committed.

This patch resolves the bug stated in the issue title: Default element properties cannot be altered.

This patch may lead to a minor performance improvement, but I suspect that the numbers are so small that my system is unsuitable to provide proper benchmarks.

After all, the intention of this patch is not at all about performance. We only ensured that the performance doesn't stink after applying it, and as mentioned before, the implementation could even have very small performance improvement. But that's totally off-topic. I'm thereby removing the Performance tag.

I would mark this RTBC, but given Moshe's annoyance with the proliferation of static caches, I think it makes sense for him to have a chance to weigh in on this. In this case, it's the replacement of a static variable with a bigger static variable, so maybe he'll be ok with it.

I changed my mind.

We are baby-sitting broken code here. The entire basic defaults are stupid.

If your #type or #theme or #theme_wrappers expect some #title or #attributes, then you should goddarnit define it. The entire logic for "basic defaults for all" is flawed.

@Damien:

function mymodule_element_info_alter(&$types) {
  foreach ($types as $type => $info) {
    $types[$type]['#pre_render'][] = 'mymodule_prerender_global';
  }
}

No.

We are baby-sitting broken code. And, your "use-case" is a total edge-case scenario. I can't even think of a real-world use-case where I'd want to do that. Why would I want to to apply the identical #pre_render or #post_render or #title or #attributes to type "token", "markup", and "form"? That makes zero sense.

Contrary to that, every single #property slows down sorting in element_children(), but also drupal_render(), because we have no clue whether an element is empty or empty. Yes, empty or empty, because basic defaults means: Nothing is empty. We need to recurse to find out.

I'll look into the notices now.

Additionally, 80% of those basic defaults are intended and usable for form elements only. They make _zero_ sense for any other renderable element. They are plain cruft, trash, overhead, senseless, or whatever you want to call it for any other element.

The remaining 20% is #attached, but that doesn't need to be defined by default and is totally special for every element in a structured array.

I could see the use-case for altering the properties for all #type 'block', 'node', 'user', or whatever. But for that, we need a separate patch that makes those use a #type and therefore hook_element_info().

The exceptions were caused by the theme_fieldset() override in Seven theme. (which is a totally useless override, btw)

This one should pass.

Actually - and this is a completely separate issue - some of the required changes unveil some very mad assumptions, e.g. taking over only specific parts of the original element in #process functions.

Fine with me

ok, if no one reviews in-depth, then I'll do myself. :P

+++ includes/common.inc	3 Nov 2009 19:45:30 -0000
@@ -5211,10 +5208,9 @@ function element_info($type) {
     $cache = module_invoke_all('element_info');
     foreach ($cache as $element_type => $info) {
...
+      $cache[$element_type] = $info;

This is plain senseless. ;)

I'm on crack. Are you, too?

+++ includes/common.inc	3 Nov 2009 19:45:30 -0000
@@ -5015,7 +5008,9 @@ function drupal_render_children(&$elemen
-    $output .= drupal_render($element[$key]);
+    if (!empty($element[$key])) {
+      $output .= drupal_render($element[$key]);
+    }

Why in this patch? If it's for performance, are we sure that the savings of a stack call for empty elements is worth an extra if statement for non-empty elements?

+++ includes/common.inc	3 Nov 2009 19:45:30 -0000
@@ -5211,10 +5208,9 @@ function element_info($type) {
     foreach ($cache as $element_type => $info) {
-      $cache[$element_type] = array_merge_recursive($basic_defaults, $info);
+      $cache[$element_type] = $info;

As you found yourself, absolutely senseless.

This review is powered by Dreditor.

Just, checking, this approach basically means we don't get the benefit of #10. No way for modules to apply some property to all elements without a type. I'm ok with this, but wanted to point it out. Elements without a type are often used as generic grouping elements. Can anyone think of a reason why a module would need to set some property on all of these? It's functionality we don't have in HEAD anyway, but something people wanted at the top of this issue.

Why in this patch? If it's for performance, are we sure that the savings of a stack call for empty elements is worth an extra if statement for non-empty elements?

Since elements can be empty now, we need the !empty() check in drupal_render() anyway. Performing the check additionally in drupal_render_children() already should save a lot of function calls to drupal_render(). I have no clue whether this makes any difference in performance, but to me, it looks like cleaner code, because I usually don't invoke something if I don't have any data to pass.

re: #45: See my rant above. It simply makes no sense.

Ok, I'm satisfied. And according to #40 and #42, so are Damien and Moshe.

Ugh. This change makes the calling code absolutely hideous.

This is also an enormous API change that wasn't started until 2 weeks after the 2nd code freeze. I much prefer Damien's initial approach at this stage in the release cycle as it's far less invasive.

The rationale in #36 seems to be "babysitting broken code / the code makes no sense" (which is stuff we can clean up in D8) and also that it should be faster since we're we don't have to recurse for elements_children(). If this patch results in a big performance impact, perhaps we can consider it, but otherwise, I'd far prefer we go back to the simpler, non-API-breaking approach.

So. Benchmarks?

Actually, I think this will have a slight performance impact, especially the larger the (entire) rendered page structure gets.

Additionally, this patch should probably rather considered as required starting point for further render system performance improvements. The render system is one of the bottlenecks in terms of performance currently and as already mentioned elsewhere, we need to streamline the system to figure out the location where we can attack with a sledge-hammer.

Re-rolled against HEAD.

Also attaching benchmarks that show a performance improvement.

sun bugged me about this on IRC again and I took another look.

The patch looks way more horrendous than it actually is because there are code style fixes sneaked in here along with the actual changes. GRRR. Please stop doing that. :\

So the real changes are just:
1. system_element_info() required a #attributes => array() index
2. $element['#required'] turns into !empty($element['#required']) (um. wtf?)
3. #title and #description are not pre-defined.

I do not like introducing 1 and 2 after code freeze. I don't think 3 will have that big of an impact on existing code, since I don't know anything that counts on #title being pre-populated (though I may be wrong...).

sun and I came up with a compromise to set form-only attributes in the form building code, rather than the global render stuff. This should both minimize the API impact, and also ensure that you don't get non-sensical crap like #required for things like blocks. Then in D8, we can go full-blown lack of hand-holding here.

Yes, and this actually makes even more sense.

Let's see what the bot thinks.

So, yeah, let's go with this compromise.

chx already started with a nice attempt to streamline drupal_render() even more in #627038: Make drupal_render iterative

Well. Let's get a review on it first, eh?

Looks awesome to me. #46 was RTBC and this is a cleaner version, so I think it also qualifies as RTBC.

This seems to no longer apply. Could we get a quick re-roll?

Straight re-roll. As long as bot likes it, it's RTBC again.

This one should pass tests. The only difference relative to #58 is this hunk

#58:

--- includes/form.inc	15 Nov 2009 21:36:06 -0000	1.399
+++ includes/form.inc	16 Nov 2009 00:43:15 -0000
@@ -1017,9 +1017,14 @@ function form_builder($form_id, $element
   $element['#processed'] = FALSE;
 
   // Use element defaults.
-  if ((!empty($element['#type'])) && ($info = element_info($element['#type']))) {
+  if (isset($element['#type']) && ($info = element_info($element['#type']))) {
     // Overlay $info onto $element, retaining preexisting keys in $element.
     $element += $info;
+    // Assign basic defaults common for all form elements.
+    $element += array(
+      '#required' => FALSE,
+      '#attributes' => array(),
+    );
     $element['#defaults_loaded'] = TRUE;
   }
 

#60:

@@ -1017,11 +1017,16 @@ function form_builder($form_id, $element
   $element['#processed'] = FALSE;
 
   // Use element defaults.
-  if ((!empty($element['#type'])) && ($info = element_info($element['#type']))) {
+  if (isset($element['#type']) && ($info = element_info($element['#type']))) {
     // Overlay $info onto $element, retaining preexisting keys in $element.
     $element += $info;
     $element['#defaults_loaded'] = TRUE;
   }
+  // Assign basic defaults common for all form elements.
+  $element += array(
+    '#required' => FALSE,
+    '#attributes' => array(),
+  );
 

So, the net change is to apply the '#required' and '#attributes' properties to all elements being processed through form_builder(), even if they don't have a type, which makes sense in case a widget wants to apply a '#theme' => 'fieldset' on an element without setting a type on it. Considering that what we're removing from HEAD is the applying of these defaults ALWAYS, there's no regression introduced by now applying them only on elements routed through form_builder() instead of only on elements routed through form_builder() that have a type.

Given the above, I'm considering this still RTBC, unless webchick sets it to "needs review" in order to get additional confirmation that this change makes sense.

Hm. Looking at the test failures, it appears to be because seven_fieldset() is a copy/paste of the old theme_fieldset(). I would just update it with this patch rather than masking the error.

No, the change to seven_fieldset() is the same as it was in #53. The reason for it failing now and not before is because of the change between 1.9 and 1.10 of file.field.inc, where a multi-valued file widget creates an element with no #type or #attributes, but with a #theme_wrapper of 'fieldset'. Theme functions for form elements should be able to rely on '#attributes' existing and being an array. Otherwise, too much client code needs to change, and that's what you didn't like in earlier versions of this patch.

Fair enough then. :)

Committed to HEAD!

Hm. I am pretty sure that this patch broke HEAD; http://qa.drupal.org/pifr/test/16620 shows a bunch of errors in Forum module related to #attached not being found. As a result, testing bot is now shut off. :(

What's not clear to me is why the testing bot didn't find this until /after/ this was committed. I don't think I committed anything else last night that would have affected this.

Anyway, could someone check to see if they can replicate the test failures before/after reverting this patch?

Looking into it now...

Not sure why bot didn't catch it before HEAD got updated, but here's a fix. The problem is that drupal_render_cache_get() calls drupal_process_attached() without ensuring that #attached exists. It could be fixed there, but I decided instead to make drupal_process_attached() more robust. Since this is fixing a broken HEAD, I'm fast-tracking it to RTBC. People following this thread can suggest alternate solutions after HEAD is working, if they like.

I'd prefer this.

That works too.

Committed #67 to HEAD. Thanks. Let's see if testing bot likes this one better.

Those of you involved in this cleanup from way back may be interested in this: #740834: Form elements cannot be rendered without form_builder().