theme_filter_tips currently does this janky thing to add it's own classes to both the containing div for each filter, and the LI tags for each tip.

Can we re-work this theme function to use the new Attributes as per
#1290694: Provide consistency for attributes and classes arrays provided by template_preprocess()

And while we're at it, can we please call theme('item_list) instead of writing our own UL and LI tags in this theme function?

Files: 
CommentFileSizeAuthor
#22 theme-filter-tips-1778624-20.patch4.1 KBhussainweb
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch theme-filter-tips-1778624-20.patch. Unable to apply patch. See the log in the details link for more information. View

Comments

jwilson3’s picture

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 :)

jwilson3’s picture

FileSize
947 bytes
PASSED: [[SimpleTest]]: [MySQL] 42,068 pass(es). View

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

jwilson3’s picture

Status: Active » Needs review
Fabianx’s picture

+++ 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.

jwilson3’s picture

FileSize
960 bytes
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed during invocation of run-tests.sh. View

#4 makes sense to me.

jenlampton’s picture

This looks better, but does item_list__filter_tips actually work? I think theme_item_list will need to support this suggestion before this will work.
See #1751194: Introduce hook_theme_suggestions[_HOOK]() and hook_theme_suggestions[_HOOK]_alter()

sun’s picture

Status: Needs review » Postponed

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.

jenlampton’s picture

Status: Postponed » Active

Since that issue is now fixed, reopening this one :)

budda’s picture

Status: Active » Needs review

.

budda’s picture

Status: Needs review » Needs work

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).

Fabianx’s picture

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 )

jwilson3’s picture

Status: Needs work » Needs review
FileSize
1004 bytes
PASSED: [[SimpleTest]]: [MySQL] 48,898 pass(es). View
steveoliver’s picture

FileSize
4.08 KB
PASSED: [[SimpleTest]]: [MySQL] 48,636 pass(es). View

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.

jwilson3’s picture

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.

jwilson3’s picture

Status: Needs review » Needs work
jenlampton’s picture

Issue tags: +Needs reroll

This is going to need a reroll. It looks like the first part in filter.module got in as part of #2009018: Replace theme() with drupal_render() in filter module

and the second part is TOTALLY different. :/

See also: https://drupal.org/node/1898416

hussainweb’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
4.05 KB
FAILED: [[SimpleTest]]: [MySQL] 56,496 pass(es), 0 fail(s), and 27 exception(s). View

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.

Status: Needs review » Needs work

The last submitted patch, theme-filter-tips-17.patch, failed testing.

hussainweb’s picture

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.

hussainweb’s picture

Status: Needs work » Needs review
Issue tags: -Twig
FileSize
214 bytes
4.1 KB
PASSED: [[SimpleTest]]: [MySQL] 56,447 pass(es). View

Attaching the fix as per #19 and an interdiff.

hussainweb’s picture

Issue tags: +Twig

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.

hussainweb’s picture

FileSize
214 bytes
4.1 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch theme-filter-tips-1778624-20.patch. Unable to apply patch. See the log in the details link for more information. View

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.

Cottser’s picture

Issue summary: View changes
Status: Needs review » Needs work

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

jwilson3’s picture

Status: Needs work » Closed (won't fix)

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.

Fabianx’s picture

Status: Closed (won't fix) » Needs 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.

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 22: theme-filter-tips-1778624-20.patch, failed testing.

mitokens’s picture

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.