Enhance drupal_render() themeing. Return renderable array on tracker page.

I was not happy with the guesswork that the new function did so I explicitly check whether the function we call expects a renderable array or not.

I made a typo, i used 'elements' instead of 'element'.

The only WTF-y part is almost 1:1 comment:code so that's OK too. Eaton liked this patch too in his twitter tho I am not sure how much he read it.

Now that's an amazing patch!

Me likes a lot. This basically makes any Drupal theme function perfectly usable with drupal_render(). And this is great - as moshe said, that makes it really really easy to keep renderable and thus alterable arrays around much much longer.

This even means that in the docs we could deprecate the use of theme() in most of the cases - build an array instead with the #theme function's arguments set and return that. This works by now for page callbacks and blocks, so all our currently common places where output comes from.

RTBC+1 from me!
We can always refine later (e.g. we really should simplify or remove hook_theme implementations for elements defined in hook_elements) but this is a huge step forward.

there's a typo in the book.module hunk: 'attributes' should be '#attributes'

This would sure make writing Field API formatters less confusing.

Wow, you've been faster than me. Attached patch improves the documentation in the theme() function hunk a bit and adds a test for the new behaviour. After the tests pass again, RTBC +1 from me.

Hm. That's a clever little trick. However, this creates a completely hopeless mess for someone who's trying to ascertain which of those #properties relate to the renderable object (#title, #weight, #theme, #attibutes) and which are properties passed into the theme function. (#links, #items, #rows, etc.)

In addition to the DX concern, however, I actually think this approach makes altering harder.

Imagine you are trying to hook_page_alter() the tracker page to change it into a list view instead of a table. Your code would end up being:

// Change the theme function.
$page['tracker']['#theme'] = 'item_list';
// Now unset all of the old stupid properties you don't need anymore.
unset($page['tracker']['#header']);
unset($page['tracker']['#rows']);
// And finally, add the things you do.
$page['tracker']['#items'] = $items;

Would it make sense to move to something more like:

$thingy['#theme'] = 'callback_function';
$thingy['#theme_arguments'] = array($var1, $var2);

In this case, it'd be a simple matter of:

// Change the theme function.
$page['tracker']['#theme'] = 'item_list';
// Replace its arguments.
$page['tracker']['#theme_arguments'] = array($items);

Wow, I saw the #22 solution in my dream and just was to post it -- but webchick was first. Mind-reader!
This makes total sense -- also, what if theme argument name clashes with fAPI propery name?

Is

$thingy['#theme'] = array('callback_function', $var1, $var2);

a step too far? For altering perspective, it is probably messier.

Hm. That's a clever little trick. However, this creates a completely hopeless mess for someone who's trying to ascertain which of those #properties relate to the renderable object (#title, #weight, #theme, #attibutes) and which are properties passed into the theme function. (#links, #items, #rows, etc.)

IMO, it's no more difficult than someone trying to figure out what properties a theme function has now. Quick! What parameters does theme_table() accept? Unless you memorize api.drupal.org, calling theme() is mystery meat at present: you have to know all the properties already. In addition, the more params there are the more ordering becomes tricky. In a lot of our utility functions like l() we already see a migration towards the use of a catch-all $options parameter with keyed items inside an array.

I would actually say this improves consistency since it uses the same names we define for tpl.php variables. IMO, it's a worthy compromise between the legacy approach of straight-up theme() functions with parameters, and the object/property paradigm of renderapi.

I could be wrong, but this is an approach moshe, chx and I discussed some time ago when the initial attempts at theming node_links() came up. We were facing a really ugly dilemma: there was no good way to make a given theme function 'smart enough' to deal with theme() and drupal_render() at once, because of the mismatch. We considered mapping #properties then, but were pulled onto other projects. Moshe has taken it another step forward and it's a real step towards consistency, IMO.

Still, I do not see how possible namespace conflicts are handled. Or is this a non-issue?

IMO it's a non-issue that can be fixed relatively easily when it's encountered. Up until this point the actual naming of arguments for a given theme function has been meaningless unless it mapped to a tpl.php file. Without a tpl.php file, they are purely "decorative" and never used in D6 and D7 presently.

This just sets up an automatic mapping, giving them actual meaning. If there are collisions, renaming the theme function arguments is straightforward for developers. Our existing commonly used #properties don't share much overlap.

It's also important to keep in mind that this move would prevent a lot of function namespace bloat. In early versions of the node links rendering patch, we struggled with the need for 'theme_links()' as well as 'theme_links_element()' -- the latter being nothing but a wrapper that extracted #properties and mapped them to theme_links(). As we gain more renderable elements this will occur more and more often. This patch allows a single theme function to do double-duty.

[Edit] Is is still hard to distinguish in first glance what are properties and what are theme parameters.
One way would be namespacing theme arguments, but this likely will not fly.

$thingy['#theme'] = 'item_list';
$thingy['#theme_header'] = ...
$thingy['#theme_rows'] = ...

This just sets up an automatic mapping, giving them actual meaning. If there are collisions, renaming the theme function arguments is straightforward for developers.

Um. Really, though?

Say I have the following theme function:

function theme_crazy_title($title) {
  return '<marquee>' . check_plain($title) . '</marquee>';
}

At what point is it going to become obvious to me as the developer that the reason my page element's #title attribute is being rendered in marquee tags is because I named my theme function parameter $title? And at what point would it be obvious to me that I should name it "$the_title" instead? Particularly given that there is no central documentation that lists all possible #properties for "reserved" names, since I raised this concern in the initial hook_page_alter() issue and was told "Don't worry, let's cross that bridge when we get to it." ;)

I agree that renaming a function parameter is easy. I don't agree that arriving at that conclusion is at all intuitive for someone bitten by a property name clash, unless I'm missing a big cluestick.

kika's approach means having to perform regex in order to find a match, but maybe something like:

$thingy['#theme'] = 'crazy_title';
$thingy['#theme']['#args']['#title'] = 'Your mom!';

At what point is it going to become obvious to me as the developer that the reason my page element's #title attribute is being rendered in marquee tags is because I named my theme function parameter $title?

That would only happen if you implemented a custom theme function that takes a 'title' argument, then assigned that theme function to the page array using its #theme property, and that page array used #title for something else. This patch is subtle, but relatively simple: if an element is passed into drupal_render(), and that element's #theme function is set, and that theme function is NOT expecting a renderable array, we attempt to map the element's named #properties to the theme function's named arguments.

The fundamental issue is that most of our existing theme functions are built around the assumption that they will receive a list of explicit parameters. At the same time, our flexible renderable arrays require that their theme functions accept a single parameter: the renderable element itself. Many of our renderables contain the same information we would manually pass into the old-style theme function, buried in their #properties, but there is no way to map between the two. So we start accumulating 'old-style' theme functions for people manually building HTML, and 'new-style' theme functions for our renderable elements that do nothing but extract #properties from the object handed to them, and call the old-style theme function. That's inefficient and clunky, and risks more naming collisions in our already overloaded namespace.

This approach means that we can use renderables and oldschool theme functions almost interchangably: a renderable element can be a way of simply capturing all the 'stuff' that a theme() function wants to be given, then putting it into a larger structure. Take these two example:

$node->content['extra_picture'] = array(
  '#markup' => theme('image', 'http://foo.com/test.jpg', NULL, t('A title'), array('class' => 'external-image'), FALSE),
  '#weight' => 5,
);

$node->content['my_extras'] = array(
  '#theme' => 'image',
  '#path' => 'http://foo.com/test.jpg',
  '#title' => t('A title'),
  '#attributes' => array('class' => 'external-image'),
  '#getsize' => FALSE,
  '#weight' => 5,
);

In the first example, we're using a traditional theme_image function so we have to stick the results into a #markup element. In the second example, we're essentially delaying the call to the theme_image function and turning the arguments into useful actual data on the renderable element. It allows us to reuse our existing theme functions without creating bizarro wrappers like theme_image_element() and so on.

I agree that renaming a function parameter is easy. I don't agree that arriving at that conclusion is at all intuitive for someone bitten by a property name clash, unless I'm missing a big cluestick.

Unless I'm confused about something, there will be no biting unless they are explicitly setting up custom theme functions and sticking them onto renderable arrays without examining the properties of those arrays. And because it increases consistency between different parts of Drupal (theme() and drupal_render()) that have historically been been closely related but very different, I think it's a net win.

Maybe Moshe can explain a bit more about some of the motivations, if I've gotten something wrong?

Eaton helped me with the cluebat in IRC. This example helped me understand.

Let's take the example of a textarea. It's a renderable element, so is a collection of hash properties. Like so:

$form['mytext'] = array(
  '#type' => 'textfield',
  '#title' => 'Bananas',
  '#description' => 'Mmm... lovely, lovely bananas.',
  '#default_value' => 'Bananas are awesome because...';
  '#rows' => 27,
  '#cols' => 60,
);

It's themed by theme_textfield by default, which takes one parameter which is the $element array:

function theme_textfield($element) {
  // do a bunch of stuff.
}

By default, this outputs a textfield with some basic divs and stuff around it. Now let's say that you wanted to be able output fancier textboxes with character counting, jQuery UI swoosh effects, and whatever else. For argument's sake, let's say you wrote a theme function like this, since you wanted it to be useful even outside of Form API:

function theme_fancy_textfield($title, $description, $default_value, $rows, $cols, $fancy_effects = 'swooshy') {
  // ...
}

In such a case, where the alternative theme function maps closely to the element being rendered, the amount of code you have to write is:

$form['#theme'] = 'fancy_textfield';

In the case where we used name-spacing of some kind, the amount of code you'd have to write is:

$form['#theme'] = 'fancy_textfield';
$form['#theme_arguments'] = array(
  // Purposely out of order to show this can be done.
  '#rows' => $form['#rows'],
  '#cols' => $form['#cols'],
  '#title' => $form['#title'],
  '#description' => $form['#description'],
  '#default_value' => $form['#default_value'];
);

I suppose you could write a helper function that would extract the non-theme property out of an element and use that. But at any rate, there's duplication there. So in this case, the name-space collision works greatly in your favour, because you only need to set the properties re-used in the theme function once.

I need to mull this a bit more, I think, but at least I understand it a bit better now.

Yeah, that's pretty much it.

Although I'd probably say that the biggest use case is not theme functions for things that you know are a renderable element, but the ability to use a renderable element to store data for an existing theme function. The 'image' example above is along those lines. Basically, it could replace a lot of the locations where we're currently calling the theme system in order to populate a #markup element, delaying the theming until later, and making the structures a lot easier to alter.

Another useful example is the Amazon module for D6. It can take an amazon ASIN product ID, and a 'style' parameter, and spit out a nicely formatted product record in a variety of ways. In the module, I need to be able to directly theme that stuff, OR put it into renderable arrays depending on the circumstances. That means I need two theme functions:

theme_amazon_item($asin = '', $style = 'normal');

theme_amazon_item_element($element);

the latter function looks for $element['#asin'] and $element['#style'] internally, and just hands things off to theme_amazon_item() after extracting the properties. This patch would eliminate the need for the intermediate step -- my 'theme_amazon_item' function could handle both cases, eliminating a lot of redundant code.

Ok. Read over this patch one more time.

- The namespace collision that both kika and I picked up on is actually one of the features of this patch, rather than a detriment. The theory is that although it may cause some DX headaches once in awhile, for the most part the theme function you'd replace on a renderable object would take the same properties anyway, so you'd end up with less work to do.

- We get to get rid of icky, nasty stuff like having both theme_list() (for renderable arrays) and theme_item_list() (for non-renderable arrays). In the process, this fixes a DX concern that's going to bite each and every person at one point or another. ("Ok, I want a list. Oh, there we go! theme_list. OH GOD THE ERRORS, THE ERRORS... Oh. theme_item_list, I guess...")

- It also comes with tests to prove it works. Hooray! ;)

Therefore, I've committed this to HEAD. Leaving "needs work" for documentation.

Umh.

http://cvs.drupal.org/viewvc.py/drupal/drupal/modules/simpletest/tests/c...

    dd(drupal_render($e), 'defaults'); $element = array(
       '#theme' => 'common_test_foo',
       '#foo' => $this->randomName(),
       '#bar' => $this->randomName(),
    );

What is that dd function call supposed to do/be?

Running the tests manually gives me:

Fatal error: Call to undefined function dd() in /home/berdir/workspace/drupal7/modules/simpletest/tests/common.test on line 654

Oh, and the testbot did ignore this one because he does not report fatal errors back...

Edit: Oh, I figured out that dd() is a function from devel.module. Well, that's not part of core... :)

node_links was still used in node_build_mode, replaced it...

Tests should now pass.

Committed to CVS HEAD. Thanks!

I am reopening this for doc. The doc at http://drupal.org/node/224333#poundtheme needs an example or more explanation, because at least two of us cannot figure it out from that doc. Please provide an example, explaining what the property names should be, etc.

See #602556: Clarification to "Upgrading", which I'm closing as a duplicate of this issue.

I had to study the example for a little while to finally understand it. Perhaps something like "by providing #-style properties" would be clearer. But I can agree with jhodgdon that adding the theme declaration (from hook_theme) might help too.

I don't personally think "providing #-style properties" illuminates it much for me.

If I'm understanding things correctly, you have to put in properties whose name is # + name of the theme function arg as defined in hook_theme (e.g. theme function arg is "foo", and you put in #foo to get that value passed to the theme function)?

And if that above is correct, what happens if you don't have all the parameters defined -- e.g. if the theme function args are array( 'foo', 'bar', 'baz') and all I provide is 'bar'? or if I provide none of the named parameters? Because normally I think the form element itself used to go in as the first/only parameter of the theme function for form elements, didn't it? Does that still happen, or is it completely bypassed always, or just if you provide named properties?

Inquiring minds want to know... and it should be made clear in the upgrade doc. In my opinion.

Bump... This still needs a documentation upgrade. Can anyone clarify this for me, or better yet update the page?

OK, I added a bit to the #poundtheme section.

theme_list() was removed above, but the corresponding 'list' element type was not removed from system_element_info().

This bit me when constructing an example render element. I picked the 'list' element and discovered it's totally broken. While a "fix" is possible, I can see the intention of this issue was to remove it. Which is what the attached patch does.

Committed to HEAD.

reverting to old issue meta info.

Hmm… found another wrapping theme function that should be removed. Following up over here: #767852: Remove no-longer-needed theme function wrappers