Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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?
Comment | File | Size | Author |
---|---|---|---|
#22 | theme-filter-tips-1778624-20.patch | 4.1 KB | hussainweb |
Comments
Comment #1
jwilson3I'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 :)
Comment #2
jwilson3To backup what i mentioned in #1, here is the patch.
Comment #3
jwilson3Comment #4
Fabianx CreditAttribution: Fabianx commentedI think this should use a __suggestion for item_list__filter_tips or so ...
Overall: Nice cleanup. Leaving for other to review more.
Comment #5
jwilson3#4 makes sense to me.
Comment #6
jenlamptonThis 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()
Comment #7
sunPostponing 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.
Comment #8
jenlamptonSince that issue is now fixed, reopening this one :)
Comment #9
budda.
Comment #10
buddaThis patch to d8 core head outputs:
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).
Comment #11
Fabianx CreditAttribution: Fabianx commentedSee 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 )
Comment #12
jwilson3Comment #13
steveoliver CreditAttribution: steveoliver commentedFollowing on @jwilson3's work, I've
'filter_tips'
entry fromfilter_theme()
theme_filter_tips
applies only to the Compose tips page (at /filter/tips), merged everything fromtheme_filter_tips
intotheme_filter_tips_long
--the page callback for "Compose tips", usingtheme('item_list__filter_tips_long')
instead of building our ownul
.theme('filter_tips')
(and only using a small part of it), calledtheme('item_list__filter_tips_guidelines')
fromtheme_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.
Comment #14
jwilson3item_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.
Comment #15
jwilson3Comment #16
jenlamptonThis 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
Comment #17
hussainwebIt 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.
Comment #19
hussainwebAfter 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:
New code:
I am going to just use an !empty() call to check, which will stop the exceptions. There were no other failures.
Comment #20
hussainwebAttaching the fix as per #19 and an interdiff.
Comment #21
hussainwebI 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.
Comment #22
hussainwebI 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.
Comment #23
star-szrUnfortunately while this looks to be a nice refactor, it breaks /filter/tips. That needs to be addressed.
Comment #24
jwilson3Filter 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.Comment #25
Fabianx CreditAttribution: Fabianx commentedYes, we want to remove as many theme functions templates as possible that just duplicate things like e.g. a list and use suggestions instead.
Comment #28
mitokens CreditAttribution: mitokens as a volunteer commented