We have a lot of frighteningly similar theme functions that only output links, but links don't even need to be run through the theme layer. For more information on what should and what shouldn't be output / overridable via the theme system.
Functions:
- theme_more_link
- theme_system_compact_link
theme_more_help_linktheme_book_title_linktheme_linktheme_pager_linktheme_pager_firsttheme_pager_lasttheme_pager_nexttheme_pager_previous
Templates:
- views-more.html.twig
Remaining tasks
#1833932: Convert theme_system_compact_link() into a #type link
#2031301: Replace theme_more_link() and replace with #type 'more_link'
#2036195: Remove views-more.html.twig and replace with #type link render arrays
#1763964: Use #type => link for theme_aggregator_block_item()
@TODO - use route name and parameters instead of system path
completed Tasks:
#2031305: Remove theme_more_help_link() and replace with a #type link render array
#1778610: Remove the check for a link template from l(), have l() always output just a string.
#1778990: Merge theme_pager_link*() theme functions into theme_link()
#1598886: Clean up pager theme functions
#1222248: Remove theme_book_title_link() use l() instead
#1222260: Remove theme_filter_tips_more_info() from core
#1833934: Remove theme_toolbar_toggle() from core.
Related tasks
#2031341: Move theme_container into theme.inc
#1804614: [meta] Consolidate theme functions and properly use theme suggestions in core
#1812684: [meta] Consolidate all table templates and add theme_hook_suggestions
#1819284: [meta] Consolidate all form element container templates, and add theme_hook_suggestions
#1813426: [meta] Consolidate all item list templates and add theme_hook_suggestions
#1812724: Consolidate all form element templates and add theme_hook_suggestons
Comment | File | Size | Author |
---|---|---|---|
#6 | 1595614-6-consolidate_on_theme_link.patch | 2.24 KB | eojthebrave |
Comments
Comment #1
sunI'd even go one step further and ask:
Do we need the wrapper DIV at all nowadays?
If not, then both of them are just theme_link().
Comment #2
sunClosely related: #1222260: Remove theme_filter_tips_more_info() from core
Comment #3
andypostSuppose we actually need some wrapper element to help themers make their magic
Comment #4
eojthebraveI think we could just use theme_link with some additional suggestions so that if someone wanted too they could easily override
theme_link__more
ortheme_link__more_help
and add whatever wrappers they needed.Comment #4.0
jenlamptonadded another
Comment #4.1
jenlamptonadded related issues
Comment #4.2
jenlamptonadded more funcs and issues
Comment #4.3
jenlamptonmore funcs and issues
Comment #4.4
jenlamptonadded pager link
Comment #4.5
jenlamptonadded book title api link
Comment #4.6
jenlamptonadd theme_system_compact_link
Comment #4.7
jenlamptonadded toolbar toggle
Comment #4.8
jenlamptonadd pager first
Comment #4.9
jenlamptonadded theme_pager issues
Comment #4.10
jenlamptonremove file
Comment #4.11
jenlamptonremove dupe issue
Comment #5
jenlamptonThere are a lot of theme functions in core that only serve to output links. I'm going to update this issue to keep track of them and hopefully we can clean them all out as we move to Twig :) I love the idea of one theme_link function with more theme hook suggestions so each one can be overridden if necessary.
Comment #5.0
jenlamptontheme link link
Comment #5.1
jenlamptonsummary thingy
Comment #6
eojthebraveHere's an example of what this would look like to remove theme_more_link and replace with theme('link__more', $variables).
This is probably just a matter of going through and doing this with all of these functions, and figuring out any special cases.
Comment #6.0
eojthebraveFix links.
Comment #6.1
jenlamptonadded one for book title link
Comment #6.2
jenlamptonadded another
Comment #6.3
jenlamptonremove dupe
Comment #6.4
jenlamptonremove other dupe
Comment #7
jenlamptonupdating title
Comment #7.0
jenlamptonadd more
Comment #7.1
jenlamptonremove list
Comment #7.2
jenlamptonadded link to issue
Comment #7.3
jenlamptonadd another related
Comment #7.4
jenlamptonmove issues to match theme funcs
Comment #8
jwilson3I've just touched on what #4 was pointing out in the parent meta issue: #1833920: [META] Markup Utility Functions. I happen to think the same on this issue as #4: that these theme functions should not be replaced with just
l()
but they should be replaced with link sub-pattern suggestions. However this walks a fine line between what is justifiable sub-pattern or not (see that issue for details).Sidenote, if we could introduce a friendly pattern for all templates to use some sort of standard wrapper syntax, then we could even achieve the div "more-link" wrapper, instead of pushing the class into the Anchor tag as in #6.
Comment #9
Fabianx CreditAttribution: Fabianx commentedAs #7 is going from the wrong assumption that a l() link is changeable in the template outputting it (It is not), re-titling back and marking #6 as CNR.
Yeah, this is only possible with #theme_wrappers property in a render array or mis-using #prefix and #suffix. (don't do that)
I think overall this needs to be decided on a case per case basis.
#6 could be also done as part of the theme_node_recent_content function directly and providing the values as $variables and you then need to overwrite this template to change the more link, which might be acceptable. However then you could also use a render array (brrr) directly.
I think what Jen was thinking of was a Link component, which is a printable object instead of a render array, that allows both complete display and display of all separate parts like in attributes.
Comment #10.0
(not verified) CreditAttribution: commentedlinky
Comment #10.1
jenlamptonremove menu link since that's not quite the same thing
Comment #10.2
jenlamptonmove menus to related not remaining
Comment #11
catchThis sounds a bit like #1213510: A modern t(). It'd have the advantage that you don't need to do most of the processing until __toString() is called, which could be handy. Could be worth opening a new issue for?
Comment #12
jenlamptonI think that would be this one #1836730: Add a renderable object that is equivalent to l()
Comment #12.0
jenlamptonupdate theme link issue
Comment #12.1
jenlamptonmove link to related
Comment #13
ParisLiakos CreditAttribution: ParisLiakos commentedtheme_aggregator_block_item() too
Comment #14
thedavidmeister CreditAttribution: thedavidmeister commentedrelated #1985470: Remove theme_link()
Comment #15
thedavidmeister CreditAttribution: thedavidmeister commentedthis seems like a better issue title based on the discussion here.
Comment #16
thedavidmeister CreditAttribution: thedavidmeister commentedmeta issues are just "active" until they're closed, I believe.
Comment #16.0
thedavidmeister CreditAttribution: thedavidmeister commentedadd more related issues
Comment #16.1
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #16.2
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #16.3
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #16.4
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #16.5
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #16.6
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #16.7
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #16.8
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #16.9
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #16.10
jenlamptonrelated
Comment #16.11
jenlamptonremove self
Comment #16.12
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #16.13
star-szrStrike through theme functions that are gone
Comment #16.14
jenlamptonupdate
Comment #16.15
jenlamptonupdate
Comment #16.16
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #17
thedavidmeister CreditAttribution: thedavidmeister commentedI found another one: views-more.html.twig #2036195: Remove views-more.html.twig and replace with #type link render arrays
Comment #17.0
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #18
thedavidmeister CreditAttribution: thedavidmeister commentedComment #18.0
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #19
thedavidmeister CreditAttribution: thedavidmeister commentedSince #2042285: #theme_wrappers should be able to have a unique set of variables per wrapper hook is in, it would be possible to rebuild a lot of the theme implementations we removed as #type 'foo_link' element types based on #type 'link'.
When I say "based on #type 'link'" I mean something like this:
We could set re-usable defaults for convenience and to help ensure consistency of markup for functionally related elements throughout core, without creating a bunch of duplicate functions/templates to render the markup. Maybe even a tiny, baby step towards #1804488: [meta] Introduce a Theme Component Library?
If contrib/custom code wants to re-implement theme_link() that we removed, it will be automatically apply to all our different link types. Unless of course, the custom/contrib code doesn't want it to, in which case it can simply implement hook_element_info() to pick out the link types it would like to have bypass theme().
Comment #19.0
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #20
pwolanin CreditAttribution: pwolanin commentedbased on #2047619: Add a link generator service for route-based links , we should start using #route_name and #route parameters instead of #href where possible
Comment #21
thedavidmeister CreditAttribution: thedavidmeister commentedwow, that is just *not* what the conclusion of that issue was. According to that issue's summary (and discussion between myself and pwolanin in IRC and the issue queue), we're not planning to convert links to #route_name and #route parameters except where access checks for l() are currently involved. "where possible" is not right.
Comment #21.0
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #22
ParisLiakos CreditAttribution: ParisLiakos commentedComment #23
mitokens CreditAttribution: mitokens as a volunteer commentedComment #24
andypostLooks that fixed, if I missed any function feel free to re-open and file child issue