The patch to commit is: #891112-57: Replace theme_item_list()'s 'data' items with render elements

Problem

  • theme_item_list() uses a completely custom, awkward syntax/structure for all items that 1) need list item attributes, and/or 2) nested lists.

Goal

  • Harmonize theme_item_list() with current core practices.

Proposed solution

  1. Change theme_item_list() support 1) a simple string value or 2) a render array as item.
  2. To specify custom list item attributes, use the custom #wrapper_attributes property, which has been discussed in a range of other issues in the past already.
  3. To avoid having to specify the full theme/render properties all over again for nested lists, implement a theme variable preprocess function that takes over the properties of a parent list to nested child lists.

Follow-ups


The documentation for theme_item_list() shows that the 'data' element of each item passed into the items variable must be a string. Considering the push to make D7 chock full of render elements, its a bit of a WTF that the most commonly used theme hook (theme_item_list) to create a generic-ish container (unordered list of items) will not accept a render element in its data element.

I know its extremly late in the d7 cycle, but since a plain string is also a valid render element, even if we make this change, it won't break any modules that have been ported; the plain strings will continue to work as usual. But we'll have added the ability to include complex-markup-as-render-element with this theme hook, instead of the considered-bad complex-markup-as-plain-string that we are currently forced to endure by theme_item_list()'s current implementation.

I'll leave it up for discussion about whether this is too late for d7, or if, because of the inherit backwards-compatibility of the patch that this is acceptable to fix a WTF for an often-used theme function.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mlncn’s picture

It would be great to see this in 7.0 or 7.1. Unfortunately i can't see a way to distinguish between $item arrays that use the 'data' key and $item arrays that are straight render arrays that isn't very ugly.

buckley’s picture

subscribing

stefgosselin’s picture

Category: task » feature

Subscribing, also bumped into this and found it to be a (minor) oversight.

amitaibu’s picture

Status: Active » Needs review
FileSize
789 bytes

Patch adds render() function on the value.

The result of this code + patch

  $items = array();
  $items[] = 'foo';
  $items[] = array('data' => 'bar');
  $items[] = array('#type' => 'link', '#title' => 'foo-link', '#href' => 'node');
  $items[] = array('data' => array('#type' => 'link', '#title' => 'bar-link', '#href' => 'node'));

  $element = array('#theme' => 'item_list', '#items' => $items);
  debug(render($element));

is as expected:

'<div class="item-list"><ul><li class="first">foo</li>
<li>bar</li>
<li #type="link" #title="foo-link" #href="node">bar</li>
<li class="last"><a href="/d7_dev/node">bar-link</a></li>
</ul></div>'

Status: Needs review » Needs work

The last submitted patch, 891112-theme-item-list-4.patch, failed testing.

mlncn’s picture

Status: Needs work » Needs review

The third list item is as expected? I'll grant this is an improvement and would recommend it going in, but by these results render elements only work for list items using the 'data' ... and, actually, no, i'm pretty sure it's going to break other things too. There's no getting around the need to have a way to disambiguate a normal render array from an item-list special data/attributes array.

amitaibu’s picture

Status: Needs review » Needs work

Indeed the test fail, I'll have a look.

> The third list item is as expected?

Yes, it's passing the value without 'data', but with the patch you can pass a render able array.

mlncn’s picture

<li #type="link" #title="foo-link" #href="node">bar</li> is not a rendered array. It is theme_item_list() treating properties of a renderable array as attributes which produces somewhat nonsense HTML :-/

amitaibu’s picture

> which produces somewhat nonsense HTML :-/

Ouch ;)

siharris’s picture

Patch adds render() on $item['data'] only

This patch produces the same output as the patch @4, which only failed because it tries to render $item['children'] which is a list item array, not a render array.

The original issue only requested that the 'data' value of a list item could be a render array, rather than the whole list item being a render array. If that's the behaviour we want then the garbage output for the third list item is expected.

Since it's only rendering 'data' which is supposed to be a string anyway, it definitely shouldn't break anything.

siharris’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, theme-item-list-891112-10.patch, failed testing.

Eric_A’s picture

Version: 7.x-dev » 8.x-dev
Status: Needs work » Needs review
Eric_A’s picture

#10: theme-item-list-891112-10.patch queued for re-testing.

mlncn’s picture

> The original issue only requested that the 'data' value of a list item could be a render array, rather than the whole list item being a render array.

Good point! For 8.x it would be better (in a follow-up) to eliminate the confusion and allow/use render arrays in either case, but i'll gladly take render or data only as a first step and in D7.

andymantell’s picture

Subscribing

geek-merlin’s picture

+1 for having renderable children too

mrfelton’s picture

Definitely +1 for this. Patch in #10 is working well for me.

txx28’s picture

#10: theme-item-list-891112-10.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, theme-item-list-891112-10.patch, failed testing.

star-szr’s picture

Status: Needs work » Needs review
FileSize
479 bytes

theme_item_list() was updated quite a bit over in #256827: Various bugs in theme_item_list(), here's an untested update of the patch in #10 for 8.x.

Berdir’s picture

#21: 891112-21.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 891112-21.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
6.7 KB

Ok, here is a patch that actually makes items a list of renderable arrays. This means:

- No more 'data', no more special handling for 'children'. If you want to to nest item lists, you can just pass in a renderable array.
- However, I had to trick a bit to allow attributes on the li argument, by introducing a '#li_attributes' property. Suggestions on how that could be nicer are welcome.

Berdir’s picture

Ups, introduced a bug while fixing another one. This should be better.

Status: Needs review » Needs work

The last submitted patch, item-list-renderable-array-891112-25.patch, failed testing.

Berdir’s picture

Converted the test. While the array structure is actually a bit more complicated now, it's no longer specific for this theme function but standard render array, except of the #li_attributes thing. So I think this makes sense.

Also shows that it's possible to create the exact theme markup.

jwilson3’s picture

Berdir’s picture

Status: Needs review » Needs work
Issue tags: +theme system cleanup, +Theme Component Library

The last submitted patch, item-list-renderable-array-891112-27.patch, failed testing.

sun’s picture

Status: Needs work » Needs review

Wow, this looks really really lovely. Great job, @Berdir!

As for the property to specify attributes for the wrapping element, I'd strongly recommend #wrapper_attributes. Removes the inherent assumption that the wrapping element would be a LI element.

That property name was actually suggested in discussions some time ago already, but didn't make it into an issue in the queue yet, AFAICS.

Berdir’s picture

#wrapper_attributes sounds great, re-roll after the theme_pager() patch went in.

sun’s picture

       if (is_array($item)) {
...
-      else {
-        $value = $item;

We still need to support string values though? (how did the patch pass tests?)

-          $value .= theme('item_list', array(
-            'items' => $item['children'],
-            'type' => (isset($item['type']) ? $item['type'] : $type),
-            'attributes' => $child_list_attributes,
-          ));
...
+        $item = drupal_render($item);

Before rendering, I think we actually should or need to recursively apply the function to child elements; i.e., something along the lines of this:

foreach (element_children($item) as $key) {
  if (!isset($item[$key]['#type']) && !isset(item[$key]['#theme']) && !isset(item[$key]['#markup'])) {
    $item[$key]['#theme'] = $variables['theme_hook_original'];
    $item[$key]['#type'] = $variables['type'];
  }
}

Or not?

Wouldn't you expect this to work?

$build = array(
  '#theme' => 'item_list',
  '#type' => 'ol',
  '#items' => array(
    'Content',
    array('#markup' => 'Structure',
      'Blocks',
      'Contact form',
      array('#markup' => 'Menus',
        'Main menu',
        'Navigation',
      )
    ),
    'Appearance',
    'People',
  ),
);
Berdir’s picture

+++ b/core/includes/theme.incundefined
@@ -2103,38 +2097,13 @@ function theme_item_list($variables) {
+        $item = drupal_render($item);

@@ -2145,7 +2114,7 @@ function theme_item_list($variables) {
-      $output .= '<li' . new Attribute($attributes) . '>' . $value . '</li>';
+      $output .= '<li' . new Attribute($attributes) . '>' . $item . '</li>';

That's how it still supports string. If it's an array, it converts to to a string using the same variable.

Not sure if that is a good idea, saves the otherwise relatively pointless else but you just proved that it is confusing.

I don't quite understand your suggestion yet...

Status: Needs review » Needs work

The last submitted patch, item-list-renderable-array-891112-32.patch, failed testing.

tim.plunkett’s picture

Hm, the

  '#theme' => 'item_list',
  '#type' => 'ol',

part makes 'ol' seem like something from hook_element_info().

Berdir’s picture

Status: Needs work » Needs review
FileSize
8.62 KB

Re-rolled an old version of the patch, here's one with the updated theme function tests.

Todo:
- Re-add $value and the else
- Understand what sun is talking about in #33 ;)
- Rename the type argument to list_type to avoid the confusing #type (?)

Status: Needs review » Needs work

The last submitted patch, item-list-renderable-array-891112-37.patch, failed testing.

Berdir’s picture

Forgot to the change li to wrapper in the test code.

sun’s picture

Title: theme_item_list's 'data' items should be render elements » Replace theme_item_list()'s 'data' items with render elements
Category: feature » task
FileSize
15.03 KB

re: #type should be #tag, yes. I think there's an issue in the queue for that already. If we need to do that change as part of this patch, then I think that would be fine.

What I meant in #33 is:

Currently, you're able to do this:

$build = array(
  '#theme' => 'item_list',
  '#items' => array(
    'Content',
    array(
      'data' => 'Structure',
      'children' => array(
        'Blocks',
        'Contact form',
        array(
          'data' => 'Menus',
          'children' => array(
            'Main menu',
            'Navigation',
          ),
        ),
      ),
    ),
    'Appearance',
    'People',
  ),
);

The expectation is that the top-level list type and theming works "recursively" for children, unless overridden by a child.

The snippet here is essentially the same as in #33 but using the current structure/syntax. The only difference of #33 is that it uses render arrays.

What I suggested in #33 is to adjust the theme function to make a "best attempt/guess" in detecting whether the nested render arrays should take over the list properties of the parent list.

Attached patch shows and implements what I mean.

I've added an example for quick manual testing to node_page_default(), which is of course to be removed.

What do you think?

Status: Needs review » Needs work

The last submitted patch, drupal8.theme-item-list-render.40.patch, failed testing.

Berdir’s picture

I'm not sure. I can see that it could be handy, but it again adds something that is special to this theme function that allows to do things that are otherwise not possible with render arrays. Not really sure but knowing how render arrays work, I wouldn't have expected that this would work like this.

Looking through the usages in core, there are very few cases where we use theme_item_list() with more than two arguments (mostly either items, items + title or items + attributes). A few examples in Views have 3 but only because they can be configured. I don't think I've seen a single case outside of the test were we actually use childrens currently but that usage might increase now that it's more flexible.

Given that, this sounds like a feature that we don't really need and wouldn't save us more than maybe 2-3 lines of code and if there's a case that would build up a structure of multiple item lists, where this could help, then it would also be quite easy to use something a $defaults definition that's appended to each element.

Other opinions?

sun’s picture

Well, yeah, my stance is:

  1. It feels a little bit unnatural to have to specify the entire properties for every child list. This becomes especially an issue if you happen to use a type of OL instead of the default UL.

  2. The inheritance gets really important in light of the theme function consolidations we're doing currently: The top-level or parent list might have been specified with a #theme of list__links__node, and both the code that defines the original list as well as drupal_alter() and template preprocess functions have to ensure that they recurse into the entire list to e.g. change the #theme definition if they wanted to.

  3. It is true that we do not have nested child lists in core yet, and that makes the evaluation of expectations harder. However, we actually do have the goal of converting menu link tree theme functions to a simple #theme 'list__links__menu__main_menu' - entirely eliminating the PITA that menu tree theming currently is. In that case, we face a potentially very large tree of nested lists, and I'd imagine that it would be tremendously helpful if the nested render array list definitions could be kept as small as possible, and that a change to the top-level definition would be inherited to all child lists (unless specifically overridden).


Just to clarify for everyone following what we're discussing: (added a marker)

  $build['list'] = array(
    '#theme' => 'item_list__test',
    '#items' => array(
      // A plain string value forms an own item.
      'a',
      // Items can be fully-fledged render arrays with their own attributes.
      array(
        '#wrapper_attributes' => array(
          'id' => 'item-id-b',
        ),
        '#markup' => 'b',
        'childlist' => array(
          '#theme' => 'item_list',
          '#attributes' => array('id' => 'blist'),
          '#type' => 'ol',
          '#items' => array(
            'ba',
            array(
              '#markup' => 'bb',
              '#wrapper_attributes' => array('class' => array('item-class-bb')),
            ),
          ),
        ),
      ),
----> // However, items can also be child #items.  <----------------------------
      array(
        '#markup' => 'c',
        'childlist' => array(
          '#attributes' => array('id' => 'clist'),
          'ca',
          array(
            '#markup' => 'cb',
            '#wrapper_attributes' => array('class' => array('item-class-cb')),
            'children' => array(
              'cba',
              'cbb',
            ),
          ),
          'cc',
        ),
      ),
      // Use #markup to be able to specify #wrapper_attributes.
      array(
        '#markup' => 'd',
        '#wrapper_attributes' => array('id' => 'item-id-d'),
      ),
      // An empty item with attributes.
      array(
        '#wrapper_attributes' => array('id' => 'item-id-e'),
      ),
      // Lastly, another plain string item.
      'f',
    ),
  );
sun’s picture

Also, I mainly kept the existing render array structure for the automated inheritance, so as to not diverge too much from the regular render array definitions and allow to mix and match them together at will.

However, perhaps it's exactly that bit that is confusing? If so, there would also be an alternative that would essentially work off a custom #items property chain:

      array(
        '#markup' => 'c',
        // This was a 'childlist' sub-element before, now a property:
        '#items' => array(
          '#attributes' => array('id' => 'clist'),
          'ca',
          array(
            '#markup' => 'cb',
            '#wrapper_attributes' => array('class' => array('item-class-cb')),
            '#items' => array(
              'cba',
              'cbb',
            ),
          ),
          'cc',
        ),
      ),

That somehow looks visually more clear to me, but it would inherently introduce a undefined behavior in case there is both an #items property and child render array elements on a nested level. I don't think I like that.

sun’s picture

Assigned: Unassigned » sun
Status: Needs work » Needs review
FileSize
16.62 KB

Took me some time to figure out how to resolve the test failures, but finally found out what's wrong and what the correct logic needs to be.

Hopefully this comes back green.

Since this patch is blocking quite a couple of other theme function consolidation issues, I wonder whether it would be possible to go with it as is for now, and possibly re-evaluate the automatic inheritance of render array properties later on, if desired?

Status: Needs review » Needs work

The last submitted patch, drupal8.theme-item-list-render.45.patch, failed testing.

Berdir’s picture

I'm really not into frontend stuff so I can't neither approve nor block this :)

I just have two things..
- Can we move that complex logic out of the template code and maybe into a preprocess function? So that the template can just do drupal_render() on normal render arrays?
- Similar to that, we should probably check with the twig folks how that should best be done with twig, I guess moving to preprocess should at least get us partially there?

tim.plunkett’s picture

Agreed, moving a lot of that to template_preprocess_item_list() would make a lot of sense.

sun’s picture

Status: Needs work » Needs review
FileSize
21.74 KB

Totally agreed on putting this logic into a variable preprocessor. That would have happened anyway as a final clean-up, and is absolutely in line with the Twig templating efforts. Attached patch does so.

That said. The last patch shows only two failures, but luckily, those two failures are caused by excellent test-coverage for render arrays.

I had to dig deep, very deep into drupal_render(), #pre_render, #post_render, #theme, #theme_wrappers, #type 'item', #type 'markup', #markup, and all possible permutations of these, in order to figure out what exactly goes wrong and what we can or need to do in order to make this usage of #markup with child elements possible. Apparently, Filter module was a good help there.

Attached patch contains a lot of debugging code and comments that are only meant for posterity. I recommend to move forward with the proposed inlining of #markup directly into drupal_render(), and I'll clean up and remove this patch in a bit.

sun’s picture

Same as #49, but without all that debugging code and also without the manual testing code on /node.

sun’s picture

I reviewed #50 once more, and I think the functional change is ready to go.

In hindsight, I was missing some comments that provide some insight into the #markup handling in drupal_render() [the removed drupal_pre_render_markup() had some], and also, the preprocessing in template_preprocess_item_list() was not really explained very well either.

Thus, as a final step, I've heavily improved the code comments.

jenlampton’s picture

Issue tags: +Twig

The long term goal with Twig is to replace all render elements with Twig data objects and their _toString methods (that will do basically the same thing). Tagging this with Twig to get some more eyes on this issue, but we could potentially save some time if we skipped the step of adding new render elements that are going to be removed later anyway.

Of course, this will unblock other important changes, so it may still be worth doing both ;)

sun’s picture

Issue tags: +API change, +API clean-up

It would be great to move forward here, since this patch blocks pretty much all other list + links + etc consolidation/conversion issues.

Time is passing by, and the theme function consolidation issues that are blocked on this are certainly bound to feature freeze, since they are improvements compared to status quo and present API changes for all extensions (of all types).

steveoliver’s picture

+++ b/core/includes/common.inc
@@ -5687,11 +5666,20 @@ function drupal_render(&$elements) {
+  if (!isset($elements['#theme']) && isset($elements['#markup'])) {
+    $elements['#children'] = $elements['#markup'] . $elements['#children'];
+  }

Should we unset($elements['#markup']) after copying it in to #children?

+++ b/core/includes/pager.inc
@@ -237,14 +237,14 @@ function theme_pager($variables) {
+        '#wrapper_attributes' => array('class' => array('pager-first')),

Can we just call this #attributes?

+++ b/core/includes/theme.inc
@@ -2074,19 +2081,60 @@ function theme_mark($variables) {
 /**
+ * Preprocesses variables for theme_item_list().
+ *
+ * @param array $variables
+ *   An associative array containing theme variables for theme_item_list().
+ *   'items' in variables will be processed to automatically inherit the
+ *   variables of this list to any possibly contained nested lists that do not
+ *   specify custom render properties. This allows callers to specify larger
+ *   nested lists, without having to explicitly specify and repeat the render
+ *   properties for all nested child lists.
+ */

@param array $variables could use simplification.

I think the function body comments could be consolidated once we're done thinking through this.

+++ b/core/includes/theme.inc
@@ -2074,19 +2081,60 @@ function theme_mark($variables) {
+/**
  * Returns HTML for a list or nested list of items.
  *
  * @param $variables
  *   An associative array containing:
- *   - items: A list of items to render. String values are rendered as is. Each
- *     item can also be an associative array containing:
- *     - data: The string content of the list item.
- *     - children: A list of nested child items to render that behave
- *       identically to 'items', but any non-numeric string keys are treated as
- *       HTML attributes for the child list that wraps 'children'.
- *     - type: The type of list to return (e.g. "ul", "ol").
- *     Any other key/value pairs are used as HTML attributes for the list item
- *     in 'data'.
+ *   - items: A list of items to render. Allowed values are strings or
+ *     render arrays. In case of a render array, the #wrapper_attributes
+ *     property can be used to specify attributes for the wrapping LI tag.
  *   - title: The title of the list.
  *   - type: The type of list to return (e.g. "ul", "ol").
  *   - attributes: The attributes applied to the list element.

Maybe this instead:
- items: A list of items to render. String values are rendered as is. Render arrays can specify list item attributes as an array of key/value pairs in the #attributes property.

+++ b/core/includes/theme.inc
@@ -2094,6 +2142,7 @@ function theme_mark($variables) {
+  // @todo 'type' clashes with '#type'. Rename to 'tag'.

When do we do this? Any reason not to now?

+++ b/core/modules/system/lib/Drupal/system/Tests/Theme/FunctionsTest.php
@@ -43,39 +43,82 @@ function testItemList() {
+        '#markup' => 'b',
+        'childlist' => array(
+          '#theme' => 'item_list',
...
+      // However, items can also be child #items.
+      array(
+        '#markup' => 'c',
+        'childlist' => array(
+          '#attributes' => array('id' => 'clist'),
+          'ca',
           array(
-            'data' => 'd',
-            'class' => array('dee'),
+            '#markup' => 'cb',
+            '#wrapper_attributes' => array('class' => array('item-class-cb')),
+            'children' => array(
+              'cba',
+              'cbb',
+            ),
           ),

I'm wondering if we want to use 'children' instead of 'childlist'?

sun’s picture

Thanks for the review, @steveoliver! On your review issues:

  1. No, there is no reason to unset #markup. It is not used afterwards. drupal_render() generally does not unset keys during rendering.
  2. No, we cannot rename #wrapper_attributes to #attributes, because that name would clash with #attributes.
  3. Can you clarify what part of the @param $variables phpDoc could use simplification? I think the phpDoc summarizes what this variable preprocessor is currently doing very well, by providing sufficient details on what it is doing and why. Both the bird-level "what" and the human-understandable "why" are the essentials of quality documentation.
  4. I'm afraid your simplification of the phpDoc for 'items' is missing the crucial differentiation between string values and render arrays. There's also no need to re-describe how render arrays are specified, since a single theme function would be the wrong place to do it.
  5. Renaming the 'type' theme variable to 'tag' is a separate issue, since it is irrelevant here. I've created it:
    #1828536: Rename 'type' variable of theme_item_list() to 'list_type'
  6. The keys 'children' and 'childlist' that you've pointed out appear in render arrays, whose child elements can generally use arbitrary key names. There is no way to enforce a certain key name and adding such an assumption to this theme function would be architecturally wrong.
steveoliver’s picture

1. re: unset #markup: cool.
2. Maybe #item_attributes? Or is #wrapper_attributes a standard property? Just seemed a little misleading.
3. it just read a little clunky, but I haven't submitted anything better, so right on :)
4. Yeah I wasn't all cool with the array, key, value nonsense. Maybe "... Render arrays can specify list item attributes in the #item_attributes property." ?
5. re: type -> tag: cool.
6. but of course. I mistakenly thought it was a key we were checking on.

Thanks.

sun’s picture

2) #whatever_attributes vs. #wrapper_attributes was discussed in ~#31 already. The reasoning being that the calling code just defines attributes that are to be applied to the wrapping element, which happens to be a list item here, by default. But if the theme function gets overridden, it might produce a list consisting of DIV > SPANs > rendered content, so what's "item"? #wrapper_attributes ignores the entire presentation logic/decisions and simply says: "Put these attributes on the element that wraps this element, kthxbye."

All the other points seem to be obsolete. Attached patch adjusts the phpDoc for 4) - thanks! :)

sun’s picture

Any further feedback, anyone? Or are we ready?

sun’s picture

Each day spent in indecision here is lost for list-related theme function consolidations.

We have a full stack of possible list consolidations and I'm happy to work on them and push them through, but all of them are blocked on this issue.

Eric_A’s picture

I'm not going to dive into the recent heavy lifting in here, but the concept of #wrapper_attributes seems big enough in itself. If I understand it correctly it would immediately solve issues like #1302482: Ensure render arrays properly handle #attributes and add them there instead of preprocess hooks and #1402250: Allow modules to classify blocks when defining them, so WAI-ARIA roles can automatically be added in the theme layer. I'm not sure if these are still relevant with all the conversions going on, but still: doesn't #wrapper_attributes need its own issue? Currently the summary says: To specify custom list item attributes, use the custom #wrapper_attributes property, which has been discussed in a range of other issues in the past already.

Eric_A’s picture

Ok, I guess there's no automagic in #wrapper_attributes. It's a custom property, needing custom processing. Too bad ;-)

Berdir’s picture

@Eric_A it's custom right now, and it needs to be processed specifically for each type because the implementation differs. But, what we could do is standardize on it for similar things and not use a different name each time.

Fabianx’s picture

Priority: Normal » Major

This is blocking major clean-up efforts and Meta Utility Functions is back to normal, so marking major.

I'll give this a proper review later today.

Fabianx’s picture

Hmmm, Unfortunately the new render array syntax is not directly compatible with Twig, though I can make this work.

I need to think a little more about this.

sun’s picture

I'm not sure I understand — we have render arrays all over the place, how can that not be compatible with Twig?

Fabianx’s picture

The reason is in the specific case to convert theme_item_list to a twig template.

One thing is the foreach ($items as &$item) part as references are supported in Twig via TwigReference, but only if $items is an array itself, so probably it would work ...

I'll try to explain more later, but need to think a little more about it ...

And look at the compiled code of the twig template.

sun’s picture

Can we care for a potential twig conversion later? Also, this is a theme function currently, and AFAIK, twig benchmarks thus far rather left us skeptical whether it will be possible to convert theme functions into twig templates in the first place...

To clarify again, this patch blocks a dozen of other list-related consolidation issues.

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

Tested and looks good to me, I wished we could have made the part with the wrapper attributes more easily accessible, but we can do as followup:

It works for now, passes tests, so RTBC.

This is the relevant TWIG code (after some change to do the new Attribute() within the preprocess function):

 <{{ type }} {{- attributes -}}>
  {% for item in items -%}
    {% set li_attributes = {} %}
    {% if item['#wrapper_attributes'] is defined %}
      {% set li_attributes = item['#wrapper_attributes'] %}
    {% endif %}
    <li class="{{ li_attributes.class }} {{ cycle(['even', 'odd'], loop.index) }} {{ loop.first ? 'first' : '' }} {{ loop.last ? 'last' : '' }}" {{- li_attributes -}}>
      {{- item -}}
    </li>
  {% endfor -%}
  </{{ type }}>

I wish it would look like:

<{{ type }} {{- attributes -}}>
  {% for item in items -%}
    <li class="{{ item.attributes.class }} {{ cycle(['even', 'odd'], loop.index) }} {{ loop.first ? 'first' : '' }} {{ loop.last ? 'last' : '' }}" {{- item.attributes -}}>
      {{- item -}}
    </li>
  {% endfor -%}
  </{{ type }}>

and remove the distinction between render arrays and strings in the preprocess, though that is probably too much of an performance overhead.

So from a TWIG perspective the only problem is the '#wrapper_attributes' being prefixed with the # as themers should always be able to use item.attributes.

A compromise code could then look like this:

 <{{ type }} {{- attributes -}}>
  {% for item in items -%}
    {% set li_attributes = {} %}
    {% if item.attributes is defined %}
      {% set li_attributes = item.attributes %}
    {% endif %}
    <li class="{{ li_attributes.class }} {{ cycle(['even', 'odd'], loop.index) }} {{ loop.first ? 'first' : '' }} {{ loop.last ? 'last' : '' }}" {{- li_attributes -}}>
      {{- item -}}
    </li>
  {% endfor -%}
  </{{ type }}>

or something like it.

Well, it is work-aroundable and renaming / move to other internal data structure can be done as part of TWIG conv => RTBC.

+1 for small steps

Thanks @sun.

Fabianx’s picture

This does not change RTBC status, but are ideas from the todays Twig spontaneous sprint in IRC:

It would be nice to get rid of the #wrapper_attributes already in preprocess and instead change to something like:

array(
  '#theme' => 'item_list',
  items => array(
    'a' => array(
      '#theme' => 'item',
      'item' => 'a',
      'attributes' => array('id' => 'id-a')
    ),
    'b',
    'c'
  ),
  'attributes' => array('id' => 'x')
);

The "#theme => 'item'" is optional as obviously we don't have a theme_item and maybe also should not have ...

But really moving the children of the render element to a structure like this would mean a much more clear and consistent wrapper:

  if (is_array($item)) {
    echo '<li' . $item['attributes'] . '>' . render($item['item']) . '</li>';
  else {
    echo '<li>' . $item . '</li>';
  }

and in Twig

{% if item.item is defined %}
<li{{ item.attributes }}>{{ item.item }}</li>
{% else %}
<li>{{ item }}</li>
{% endif%}

I am just keeping the if / else case for now for performance reasons ...

sun’s picture

sun’s picture

@Fabianx: What you're essentially asking for is to change the per-item render array in a way that forces an extra render array level for the list item itself. That's the only way to change #wrapper_attributes into #attributes.

Doing so would have negative DX consequences though for the majority of use-cases that do not have a need to specify anything special for the list item and which only want to provide the inner contents for a list item.

That said, doing so might make sense, but perhaps also not. I'm very open and happy to discuss this, but pretty please in a follow-up issue to this, since that's an implementation detail, which we can change anytime later on, in case we conclude it makes more sense.

The main change that we need from this patch is the change to render arrays.

If this patch here doesn't get committed in a day or two, then I seriously don't see how we'll be able to get all of the other issues/patches ready before feature freeze. And again, the changes that we're proposing for those theme functions are 100% API changes (improvements), so they do not need any kind of "interpretation" at all — all of them are bound to feature freeze.

Fabianx’s picture

Assigned: sun » Unassigned

#71: This patch is still RTBC - without a doubt. I unassigned you for now, perhaps then a core committer comes along to commit this :).

Fabianx’s picture

Issue summary: View changes

Updated issue summary.

catch’s picture

Title: Replace theme_item_list()'s 'data' items with render elements » Change notice: Replace theme_item_list()'s 'data' items with render elements
Priority: Major » Critical
Status: Reviewed & tested by the community » Active

Committed/pushed to 8.x, thanks!

This will need a change notice.

catch’s picture

Also a general note, this isn't the sort of thing that's blocked on feature freeze. Some API changes we'll probably get stricter on as it gets closer to code freeze itself, but this is straight refactoring and there's still time for that.

sun’s picture

Title: Change notice: Replace theme_item_list()'s 'data' items with render elements » Replace theme_item_list()'s 'data' items with render elements
Priority: Critical » Major
Status: Active » Fixed

Change notice: http://drupal.org/node/1842756

Existing follow-up issue for the contained @todo: #1828536: Rename 'type' variable of theme_item_list() to 'list_type'

@Fabianx: Do you want to create an issue for the detail we discussed and link to it from here?

Fabianx’s picture

#75: Yes, will do. I am first playing around with some sample code for drillable render arrays and the use case for what I proposed.

So I am fine to just open this one once I am ready as it will be touched by Twig conversion anyway. (and I am gonna sync this commit over to the sandbox)

Stellar work! Helps me so much!

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

steveoliver’s picture

I don't want to re-open this, but just wanted to make a note on the #wrapper_attributes 'issue' as it has come up in #1939062-14: Convert theme_item_list() to Twig, but to sun's point in #71:

@Fabianx: What you're essentially asking for is to change the per-item render array in a way that forces an extra render array level for the list item itself. That's the only way to change #wrapper_attributes into #attributes.

#wrapper_attributes may not be ideal (and just a little akward when called out in templates), but I think it's fine, considering that we are grabbing properties from our render arrays when we need them instead of a.) either adding another level or render array as sun pointed out, or b.) mirroring the entire item list structure in preprocess, just to avoid a hash symbol in templates.

jenlampton’s picture

I've opened a follow up issue so we can keep working on this over there
#1963402: Pass variables to Twig in a nicer way (was: Create drillable render arrays)

jenlampton’s picture

Issue summary: View changes

Added patch