rework theme_filter_tips to use the new Attributes, and call theme('item_list) while we're at it

I've got a small piece of code now that replaces the custom LIs with a call to theme('item_list') but I don't understand the overarching goal of the rest of this.

Would you like to rework this into a preprocess and a template file, in preparation for migration to twig templates? Or just rewrite the lines that add the wrapper divs , in order to take advantage of the new Attributes class? If its the later, then I don't understand how rewriting a simple line of code like: $output .= '<div class="compose-tips">'; into something that uses the Attributes class, a la $output .= '<div' . new Attribute(array('class' => array('compose-tips')) . '>'; would make this any cleaner for themers.

IMHO, the obvious problem with this theme function that makes it really ugly and hard to read and comprehend are the various conditions ($multiple, etc) wrapped inside nested foreach loops.

PS. I'm new to the Drupal 8 retheming movement and twig movement, but exited to get started helping, so go easy on me :)

To backup what i mentioned in #1, here is the patch.

+++ b/core/modules/filter/filter.pages.incundefined
@@ -67,11 +67,17 @@ function theme_filter_tips($variables) {
         }
-        $output .= '</ul>';
+        $output .= theme('item_list', array(
+          'items' => $items,
+          'attributes' => array('class' => array('tips'))

I think this should use a __suggestion for item_list__filter_tips or so ...

Overall: Nice cleanup. Leaving for other to review more.

#4 makes sense to me.

Postponing on #891112: Replace theme_item_list()'s 'data' items with render elements

That patch is pretty much ready and just needs a commit. Afterwards, we'll convert the $items here into the new structure.

.

This patch to d8 core head outputs:

<div class="item-list"><ul class="tips"><li class="odd first"><div class="item-list"><ul><li class="odd first last">filter-filter-url</li></ul></div></li><li class="even"><div class="item-list"><ul><li class="odd first last">filter-filter-html</li></ul></div></li><li class="odd last"><div class="item-list"><ul><li class="odd first last">filter-filter-autop</li></ul></div></li></ul></div>

Which when rendered in the admin overlay looks wrong to the end user too.

An error message is returned a number of times:

User error: "data" is an invalid render array key in element_children() (line 6022 of core/includes/common.inc).

See here for the new syntax for item lists:

http://drupal.org/node/1842756

( there is also some code that can be used for item-list.twig )

Following on @jwilson3's work, I've

1. removed 'filter_tips' entry from filter_theme()
2. since most of the logic in theme_filter_tips applies only to the Compose tips page (at /filter/tips), merged everything from theme_filter_tips into theme_filter_tips_long--the page callback for "Compose tips", using theme('item_list__filter_tips_long') instead of building our own ul.
3. instead of calling theme('filter_tips') (and only using a small part of it), called theme('item_list__filter_tips_guidelines') from theme_filter_guidelines--the theme callback that produces the tips for each format below textarea fields.

This is a general theme sytem cleanup task that will also benefit Twig by allowing us to re-use item-list templates when we convert these to preprocess function + twig files.

item_list__filter_tips_guidelines reads and sounds a little weird.

Couldn't we keep it as analog to the old format as possible: item_list__filter_tips and item_list__filter_guidelines. Other than that, the patch looks good, thanks for forward movement steve.

It is quite an old patch and several changes were needed.

As of HEAD, the filter.module was updated to use the newly recommended drupal_render() method. This still used the theme hook which was removed in the previous commit, though.

To merge, I have removed the theme implementation and replaced with drupal_render(). But, I have also removed theme_filter_tips() as per #13. Further, I have replaced theme() with drupal_render() in filter_tips_long() as well.

Lets see how it goes with the testbots.

After lots of analysis, I found that the exceptions are thrown when a text format does not enable any filters, which is common in tests. This did not occur earlier because there was a foreach loop to pick up all text formats, there was no direct indexing:

Old code:

    foreach ($tips as $name => $tiplist) {
      if ($multiple) {
        $output .= '<div class="filter-type filter-' . drupal_html_class($name) . '">';
        $output .= '<h3>' . $name . '</h3>';
      }
    ...
    }

New code:

  $tips = $filter_tips[$format->name];

I am going to just use an !empty() call to check, which will stop the exceptions. There were no other failures.

Attaching the fix as per #19 and an interdiff.

I have no idea how the 'Twig' tag got removed. Adding it back (even though we are not doing anything about twig here, except preparing for it)...

Also, FWIW, I ran one of the earlier tests locally and it came out clean - no exceptions and all passed.

I am attaching the exact same patches with the issue ID in the filename. I am not sure how important it is but I have forgotten it in my last few patches.

Unfortunately while this looks to be a nice refactor, it breaks /filter/tips. That needs to be addressed.

Filter tips now use the Attributes object. The twig templates make use of hardcoded <ul> and <li> tags, however, I'm not sure this is a Bad Thing anymore, unless it really makes sense to rework this back into a preprocess to set it up to use '#theme' => 'item_list' in the preprocess. If this does still make sense to someone, feel free to reopen and continue work.

Yes, we want to remove as many theme functions templates as possible that just duplicate things like e.g. a list and use suggestions instead.

Thank you for creating this issue to improve Drupal.

We are working to decide if this task is still relevant to a currently supported version of Drupal. There hasn't been any discussion here for over 8 years which suggests that this has either been implemented or is no longer relevant. Your thoughts on this will allow a decision to be made.

Since we need more information to move forward with this issue, the status is now Postponed (maintainer needs more info). If we don't receive additional information to help with the issue, it may be closed after three months.

Thanks!

The current parent meta issue assigned in comment #28 seems irrelevant at this point, since the filter-tips.html.twig that ships with Drupal 11 has appropriate use of Attributes and provides no default classes anymore.

#25:

we want to remove as many theme functions templates as possible that just duplicate things like e.g. a list and use suggestions instead.

This may be a valid point and relevant to this issue, but this seems extremely low-priority. The community has embraced Twig templates everywhere. Having this template isn't causing anyone issues. And is arguably more flexible allowing themers to completley swap out the simple nested list for a <table>, or a <details> or another component for improved readability of long filter tips.

I suggest closing this, but defer to more active core contributors for consensus.

Since there's been no follow up going to go with #43