Links in renderable arrays and forms (e.g. "Operations") are not alterable

A similar issue is here: #318636: Make l() themable, but I'm not marking this one a duplicate. I have an alternate idea for how to handle operations links specifically and am working on it. Will post again here when I have something worthwhile to share.

I'm fine with this patch, but whats the problem with "forking the whole l() call"? You just build the link your need. This does add a lot of theme() calls to a node/comment/user listing. A listing of 50 items often has 200 operations links.

How about this?

BTW: still working through the other places...

Here it is for all places that were already setting #markup. Specifically:
------------------------
_locale_languages_configure_form_language_table
block_admin_display_form
filter_admin_overview
field_ui_field_overview_form
image_style_form
_menu_overview_tree_form
profile_admin_overview
system_themes_form
taxonomy_overview_vocabularies
taxonomy_overview_terms
------------------------

Next up: figure out what to do in places that use tableselect #options
------------------------
comment_admin_overview
user_admin_account
------------------------

And then there's all the places that just call theme('table') or set '#rows' on an element with #theme='table' with hard-coded links already in $rows:
------------------------
theme_locale_languages_overview_form
_locale_translate_seek
aggregator_view
book_admin_overview
theme_book_admin_table
contact_category_list
dblog_overview
dblog_event
theme_image_style_list
menu_overview_page
node_overview_types
node_revision_overview
openid_user_identities
path_admin_overview
statistics_recent_hits
statistics_top_visitors
statistics_node_tracker
statistics_user_tracker
system_ip_blocking
system_actions_manage
translation_node_overview
theme_user_admin_new_role
------------------------

Oh boy :)

OMG! There was a much simpler way. Simply replace #markup with #link and have drupal_render() deal with it inline. Not only does this avoid the theme() overhead, this patch should be FASTER than head, because #markup DID go through the theme() overhead, so we now save ourselves an extra theme() call for every link that without this patch was being put into #markup.

This patch also includes the 2 tableselect use cases, but I had to expand the capabilities of _theme_table_cell() to do it.

So, my thought is that if testbot passes #8 and if it passes code review, it should be fast-tracked to commit, so that whoever is working on overlay can keep going. As far as the last list of functions in #6 (the ones that currently call theme('table') directly on hard-coded data), seems like they should all be refactored so that the form constructor / page callback builds and returns a more flushed out FAPI array / page array (i.e., change these to work like the ones that this patch was able to convert). That way overlay can do all it needs in hook_form_alter() / hook_page_alter(). I'm not up to that task ATM. Is there anyone else who can take that on?

testbot rocks!

No wonder testbot wasn't testing it. CNR. Clearly, I'm way too tired and need to go to sleep now.

That patch is much simpler. But I think I have improved even further. The attached patch only converts one link to the new system so you can see whats happenning.

1. introduced a new render element #link which transforms the link parts during a #pre_render call. the output of the pre_render is a #markup containing the built link. i did not like the special case added to drupal_render() in #11.
2. i removed theme_markup(). this saves us a lot of theme() calls. instead, we simply move #markup contents to #prefix during #pre_render. then drupal_render() does its usual drupal_render_children. this speeds up all of drupal, not just link building

Lets see what the bot says.

#11 and #19 merged!

Added #access there.

Also switched to #post_render for processing #markup. Is better this way. Now #pre_render callbacks are free to transform stuff into #markup and it will get processed later. No need to shift themselves to the beginning array, for example. Also, we don't have to change that $elements['#children'] code at all. To a certain extent this is bikeshedding, but I prefer it a little bit more.

+++ includes/common.inc	14 Oct 2009 02:39:57 -0000
@@ -4333,6 +4333,47 @@ function drupal_set_page_content($conten
+function drupal_pre_render_link($elements) {
+  $options = isset($elements['#options']) ? $elements['#options'] : array();
+  $elements['#markup'] = l($elements['#title'], $elements['#href'], $options);
+  return $elements;
+}

This might make #318636: Make l() themable obsolete.

+++ includes/common.inc	14 Oct 2009 02:39:57 -0000
@@ -4333,6 +4333,47 @@ function drupal_set_page_content($conten
+function drupal_pre_render_markup($elements) {
+  $elements['#children'] = $elements['#markup'];
+  return $elements;
+}

Just a quick question here... If an object type of markup has child elements, what happens?

$form['mystuff'] = array(
  '#type' => 'markup',
  '#markup' => 'I am amazing.',
);
$form['mystuff']['myotherstuff'] = array(
  '#markup' => 'I am not so awesome.',
);

+++ modules/profile/profile.admin.inc	14 Oct 2009 02:31:14 -0000
@@ -51,6 +51,7 @@ function profile_admin_overview($form) {
 
+  // @todo: Any reason this isn't done using an element with #theme = 'links'?

Either we stick it in this patch, or remove the comment and make a follow up issue. @TODO's are ugly ;-) .

This review is powered by Dreditor.

Meh, we can make a follow up for the @todo :-) .

webgrind screenshots. This adds some overhead (approx 100 additional calls to drupal_render() on admin/content/content with 50 nodes listed. But it's admin pages only, and shouldn't hold the patch up. It's also likely less overhead than theme_link() would be - since that has to be called every time l() gets called anywhere.

@#25: See http://drupal.org/node/318636#comment-2148688 for why I don't think this makes that issue obsolete, unless we think we can replace all calls to l() with the use of a renderable element.

Ah, you're right, effulgentsia. This only hits up links that are rendered with #type=link, and applies that type to the renderable links in hook_link. #318636: Make l() themable is for all links passed through l(). That makes sense, thanks.

Ok, this one is RTBC. Nice team effort.

Re-rolled.

Fixed 1 trivial line of code (new code in HEAD assigned 'markup' as a theme function, which is replaced in this patch by 'drupal_pre_render_markup').

Re-rolled.

Re-rolled.

I see catch's webgrind output in #28, but that's percentage-based. Could we also get some benchmarks about the overall time on the page and how it compares before/after this patch? There are lots of comments here that say it "should" be faster, but it'd be nice to have confirmation. If I missed them somewhere just hit me with a reply #.

Since this is an API change (which was ready by code freeze), I'd like to get this taken care of relatively quickly.

A good test case for this benchmark would probably be something like admin/content/node with 50 nodes.

200 nodes, 50 on that page, anonymous user with administer nodes permission. Ran these twice before/after - posting one result but very consistent:

HEAD:
Concurrency Level:      1
Time taken for tests:   127.679 seconds
Complete requests:      1000
Failed requests:        0
Write errors:           0
Total transferred:      34594000 bytes
HTML transferred:       34129000 bytes
Requests per second:    7.83 [#/sec] (mean)
Time per request:       127.679 [ms] (mean)
Time per request:       127.679 [ms] (mean, across all concurrent requests)
Transfer rate:          264.59 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:   113  128   9.6    125     204
Waiting:      106  120   9.0    117     195
Total:        113  128   9.6    125     204

Patch:

Concurrency Level:      1
Time taken for tests:   133.966 seconds
Complete requests:      1000
Failed requests:        0
Write errors:           0
Total transferred:      34594000 bytes
HTML transferred:       34129000 bytes
Requests per second:    7.46 [#/sec] (mean)
Time per request:       133.966 [ms] (mean)
Time per request:       133.966 [ms] (mean, across all concurrent requests)
Transfer rate:          252.18 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:   117  134  14.5    129     248
Waiting:      110  126  13.8    121     231
Total:        117  134  14.5    129     248

A bit slower, but since we're not using this on 'public facing' pages, and there's other things we can do on these pages (like fixing slow queries etc.), marking back to RTBC.

Note that we also run check_plain() on all those node titles. With this patch and #523058: Optimize check_plain() I get, which makes this a no-op - and that has effects on every page, not just admin tables.

Concurrency Level:      1
Time taken for tests:   128.993 seconds
Complete requests:      1000
Failed requests:        0
Write errors:           0
Total transferred:      34594000 bytes
HTML transferred:       34129000 bytes
Requests per second:    7.75 [#/sec] (mean)
Time per request:       128.993 [ms] (mean)
Time per request:       128.993 [ms] (mean, across all concurrent requests)
Transfer rate:          261.90 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:   115  129  14.1    124     211
Waiting:      107  121  13.6    116     202
Total:        115  129  14.1    125     211

Forgot to mention, the 'should be faster' was a comparison between renderable arrays on pages like this, and a theme('link') function running on every link everywhere - not this patch vs. HEAD.

webchick: This patch covers two types of conversions. With the following functions (with paths in parens next to them), an existing #markup is replaced with a #type='link':

_locale_languages_configure_form_language_table (admin/config/regional/language/configure)
block_admin_display_form                        (admin/structure/block) 
field_ui_field_overview_form                    (admin/structure/types/manage/*/fields)
filter_admin_overview                           (admin/config/content/formats)
image_style_form                                (admin/config/media/image-styles/edit/%image_style)
_menu_overview_tree_form                        (admin/structure/menu/manage/%menu)
profile_admin_overview                          (admin/config/people/profile)
system_themes_form                              (admin/appearance)
taxonomy_overview_vocabularies                  (admin/structure/taxonomy)

These are the ones for which my comment #8 (should be faster) was more applicable to. However, it turns out I was wrong. I did a benchmark for a admin/structure/taxonomy with 30 vocabularies (so 90 operations links, because each vocabulary has 3 operations links), and the result was HEAD at 112ms and Patch at 113ms. The difference is small, and if necessary for this patch to be accepted, can be optimized to be even smaller. For example, we now have #type=link invoking 2 pre_render functions, and this can be optimized to 1 if necessary. Also, while looking at #601806: drupal_render() should not set default element properties when #type is not known, I discovered that it takes surprisingly long to call element_info() (which is called when there's a #type, but not when there isn't, so again, if necessary, some further optimization can be performed here). Please let me know if these are optimization you're not interested in (given that these are admin pages), optimizations you're interested in as a follow-up, or optimizations required for this patch to be accepted.

The second set of conversions involve adding a full new render() element where there wasn't one previously. These are:

comment_admin_overview                          (admin/content/comment)
node_admin_nodes                                (admin/content)
user_admin_account                              (admin/people)

For these, there's the much larger performance penalty (5% on the admin/content page that catch benchmarked). If necessary, these can also be optimized to be smaller (but not to 0, and with being more aggressive by inlining the handling of #type=link into drupal_render() in order to avoid all sorts of overhead involved with that function). Again, given that these are admin pages, how important is it to aggressively optimize (either as a follow-up, or as a prereq to having this patch land)?

I think this patch is important because it is just wrong to not provide any mechanism by which operations links can be altered. So please let me know how aggressively you would like me to optimize the drupal_render() code for it to meet your performance needs.

Ok, after banging my head against the wall for a few hours, I found the reason why the forms where there was a conversion from #markup to #type=link resulted in slightly worse performance despite the fact that micro-benchmarks indicated that just calling drupal_render() on elements with #markup or with #type=link were both slightly faster with this patch than calling drupal_render() on elements with #markup is currently in HEAD. The reason was because form_builder() adds default properties on elements with a #type, but not on elements without, and then later calls element_children(), and element_children() takes longer with each property that the element has.

So, to compensate, this patch includes an optimization that introduces a new property, #leaf, which is set to TRUE for the 'link' type. And then element_children() early returns for these elements (instead of wasting time looping on each property and checking if it starts with '#'). As a follow-up to this issue, we may want to apply this optimization on other element types (such as 'markup' and 'hidden'), but let's figure that out separately. In the meantime, it seems totally reasonable that links should not have children (and if in some cases they need to, there are ways to set the #leaf property to FALSE for those use-cases).

With this change, I apache benchmarked the /admin/structure/taxonomy/2/list page (where vocabulary 2 had >100 terms in it, 100 of which were shown on the page), and I got equivalent results between HEAD and this patch (211.96ms for HEAD and 211.97ms for the patch, but the standard deviation of the mean was 0.1 in both cases, so the difference is not statistically significant). This is for 200 links (a view link and an edit link for each of 100 terms) having been converted from #markup to #type=link.

I removed from this patch the conversions of comments, content, and users (see #44), because these still have performance overhead due to converting from cell data that doesn't result in render() being invoked at all to cell data that requires a render() call, and render() still has overhead. I'll keep working on further optimizations to render() on separate issues, and if we ever get it fast enough, we can revisit converting these 3 forms. Or, if we decide that it's okay for these 3 forms to become a little slower (see #42), we can create a follow-up issue to convert them. In the meantime, by removing them from this issue, this patch introduces no performance regression of any kind, and so I hope can land quickly.

Setting it to "needs review" due to the code changes described. Hopefully, it can move back to RTBC quickly.

@webchick: please confirm sun's comment in #47. Patch 46 has no performance regression. Are you ok with adding a 4% regression (see #42) to the three other admin pages, in which case, I'll re-roll this patch to re-include them. Otherwise, can we get this committed, and then debate those three pages? Your comment in http://drupal.org/node/318636#comment-2196578 makes me wonder where you stand.

The '#leaf' property might speedup field rendering a small bit, because those render elements have quite a few #properties...

Ok. After much internal deliberation, and given that it has thumbs up from Render API Master Moshe and even Lead Performance Guru Catch is ok with the performance regression on admin pages that this patch causes (as a trade-off in flexibility), I've decided to commit #50 to HEAD. I agree with sun that #leaf is an extra layer of abstraction that we don't need, and see no reason to introduce this pattern into core if we're not going to make use of it. I appreciate all of the work that went into making this as performant as possible; lots of people will be really happy to see this change.

Please document this in the module upgrade guide ASAP so that those who are creating their own admin panels in their D7 modules can make use of this new link structure.

That's great! Can we do something even greater that would allow us not to call render in _theme_table_call and getting that data in the page array?

The entire topic of renderable arrays needs to be documented. What can be there, how can it get there, etc. That's part of the documentation that's required here. If anybody will give me a quick tutorial on renderable arrays or a pointer to some code that will help me out, I can write something.

I'm not sure what documentation already exists, but here's a quick hit of what I think to be most important about renderable arrays (this list is likely incomplete):

- In D6, page callbacks returned HTML strings. In D7, they can return HTML strings as well, but it's recommended that they return renderable arrays instead. That exposes the content to more hooks that can alter it.

- For anyone familiar with FAPI arrays, renderable arrays are similar. They are associative arrays than can have properties (keys starting with "#") and children (keys not starting with "#"). The "#type" property determines the default values of other important properties that get merged onto the array, and therefore determines how the array gets rendered.

- Renderable arrays are rendered to HTML strings by calling drupal_render() on them. There are helper function, show(), hide(), and render(), that theme templates can use to control what gets rendered where.

- Step through drupal_render() to see the properties involved in the rendering process ("#cache", "#pre_render", "#theme", "#theme_wrappers", "#post_render", "#prefix", "#suffix", and "#attached").

- The longer content remains as a renderable array, the better. In other words, the later in the page execution that render() or drupal_render() is called, the better. It means more hooks can alter the content without resorting to reg-ex'ing a large flattened HTML string. About as late as you can get in the process is a theme function that renders the direct children of the element passed into it. So, theme functions and template files that call render() are doing it right. Functions that aren't theme functions that call render() or drupal_render() or generate an HTML string through some other way aren't taking as much advantage of the render system as possible. There could be good reasons for this, but it should at least be looked at.

- A renderable array with "#type" = "markup" or without a #type, but with a #markup property, is rendered simply by returning the #markup property. This allows generic HTML that isn't covered by one of the other types to still exist within a renderable array tree. However, it's easier for hooks to alter content that exists in a more structured way rather than content that is already HTML'ified in a #markup property. This issue, specifically, was about introducing a 'link' type, so that links could exist in this more structured way. So, the recommendation is to minimize using #markup, and instead use one of the appropriate types where possible.

- FAPI arrays are a special case of renderable arrays, so functions returning FAPI arrays are already returning renderable arrays.

Hope this helps get you started.

@chx - see #80855: Add element #type table and merge tableselect/tabledrag into it.

When this issue started, it was targeted to Operations links, so the way I located what needed changing was by grepping for 'Operations'. However, the issue became a generic #markup -> #type='link' conversion. This patch includes some stragglers that weren't found when grepping for 'Operations'.

Just a note we didn't build anything in here for allowing more than one operations link in the operations column - see http://drupal.org/node/301902#comment-2228116

That means either those stay unalterable, or there has to be two columns, or something - thoughts welcome.

Committed #58 to HEAD.

Looks like this is still needs work for docs.

#653068: Update documentation is incomplete

http://drupal.org/update/modules/6/7#operation_links

Looks like the more generic render array information from #56 has also already been added to that page (thank you, rfay, or whoever did that).

FYI: There is a suggestion to rename #href to #path in #656614: Rename #href to #path.