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:

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

Files: 
CommentFileSizeAuthor
#6 1595614-6-consolidate_on_theme_link.patch2.24 KBeojthebrave
FAILED: [[SimpleTest]]: [MySQL] 47,853 pass(es), 5 fail(s), and 0 exception(s).
[ View ]

Comments

sun’s picture

I'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().

sun’s picture

andypost’s picture

Suppose we actually need some wrapper element to help themers make their magic

eojthebrave’s picture

I think we could just use theme_link with some additional suggestions so that if someone wanted too they could easily override theme_link__more or theme_link__more_help and add whatever wrappers they needed.

jenlampton’s picture

Issue summary:View changes

added another

jenlampton’s picture

Issue summary:View changes

added related issues

jenlampton’s picture

Issue summary:View changes

added more funcs and issues

jenlampton’s picture

Issue summary:View changes

more funcs and issues

jenlampton’s picture

Issue summary:View changes

added pager link

jenlampton’s picture

Issue summary:View changes

added book title api link

jenlampton’s picture

Issue summary:View changes

add theme_system_compact_link

jenlampton’s picture

Issue summary:View changes

added toolbar toggle

jenlampton’s picture

Issue summary:View changes

add pager first

jenlampton’s picture

Issue summary:View changes

added theme_pager issues

jenlampton’s picture

Issue summary:View changes

remove file

jenlampton’s picture

Issue summary:View changes

remove dupe issue

jenlampton’s picture

Title:[meta] Remove all the theme functions in core that simply output a link. Replace with l()» Remove or consolidate many similar link functions in core

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

jenlampton’s picture

Issue summary:View changes

theme link link

jenlampton’s picture

Issue summary:View changes

summary thingy

eojthebrave’s picture

Title:theme_more_link() vs. theme_more_help_link()» Remove or consolidate many similar link functions in core
StatusFileSize
new2.24 KB
FAILED: [[SimpleTest]]: [MySQL] 47,853 pass(es), 5 fail(s), and 0 exception(s).
[ View ]

Here'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.

eojthebrave’s picture

Issue summary:View changes

Fix links.

jenlampton’s picture

Issue summary:View changes

added one for book title link

jenlampton’s picture

Issue summary:View changes

added another

jenlampton’s picture

Issue summary:View changes

remove dupe

jenlampton’s picture

Issue summary:View changes

remove other dupe

jenlampton’s picture

Title:Remove or consolidate many similar link functions in core» [meta] Remove all the theme functions in core that simply output a link. Replace with l()

updating title

jenlampton’s picture

Issue summary:View changes

add more

jenlampton’s picture

Issue summary:View changes

remove list

jenlampton’s picture

Issue summary:View changes

added link to issue

jenlampton’s picture

Issue summary:View changes

add another related

jenlampton’s picture

Issue summary:View changes

move issues to match theme funcs

jwilson3’s picture

Title:Remove or consolidate many similar link functions in core» [meta] Remove all the theme functions in core that simply output a link. Replace with l()

I'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.

Fabianx’s picture

Title:[meta] Remove all the theme functions in core that simply output a link. Replace with l()» Remove or consolidate many similar link functions in core
Status:Active» Needs review

As #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.

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.

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.

Status:Needs review» Needs work

The last submitted patch, 1595614-6-consolidate_on_theme_link.patch, failed testing.

Anonymous’s picture

Issue summary:View changes

linky

jenlampton’s picture

Issue summary:View changes

remove menu link since that's not quite the same thing

jenlampton’s picture

Issue summary:View changes

move menus to related not remaining

catch’s picture

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.

This 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?

jenlampton’s picture

Title:Remove or consolidate many similar link functions in core» [meta] Remove or consolidate many similar link functions in core
jenlampton’s picture

Issue summary:View changes

update theme link issue

jenlampton’s picture

Issue summary:View changes

move link to related

ParisLiakos’s picture

thedavidmeister’s picture

thedavidmeister’s picture

Title:[meta] Remove or consolidate many similar link functions in core» [meta] Remove all the theme functions in core that simply output a link. Replace with #type 'link' render arrays

this seems like a better issue title based on the discussion here.

thedavidmeister’s picture

Status:Needs work» Active

meta issues are just "active" until they're closed, I believe.

thedavidmeister’s picture

Issue summary:View changes

add more related issues

thedavidmeister’s picture

Issue summary:View changes

Updated issue summary.

thedavidmeister’s picture

Issue summary:View changes

Updated issue summary.

thedavidmeister’s picture

Issue summary:View changes

Updated issue summary.

thedavidmeister’s picture

Issue summary:View changes

Updated issue summary.

thedavidmeister’s picture

Issue summary:View changes

Updated issue summary.

thedavidmeister’s picture

Issue summary:View changes

Updated issue summary.

thedavidmeister’s picture

Issue summary:View changes

Updated issue summary.

thedavidmeister’s picture

Issue summary:View changes

Updated issue summary.

thedavidmeister’s picture

Issue summary:View changes

Updated issue summary.

jenlampton’s picture

Issue summary:View changes

related

jenlampton’s picture

Issue summary:View changes

remove self

thedavidmeister’s picture

Issue summary:View changes

Updated issue summary.

Cottser’s picture

Issue summary:View changes

Strike through theme functions that are gone

jenlampton’s picture

Issue summary:View changes

update

jenlampton’s picture

Issue summary:View changes

update

thedavidmeister’s picture

Issue summary:View changes

Updated issue summary.

thedavidmeister’s picture

thedavidmeister’s picture

Issue summary:View changes

Updated issue summary.

thedavidmeister’s picture

Title:[meta] Remove all the theme functions in core that simply output a link. Replace with #type 'link' render arrays» [meta] Remove all the theme functions and templates in core that simply output a link. Replace with #type 'link' render arrays
thedavidmeister’s picture

Issue summary:View changes

Updated issue summary.

thedavidmeister’s picture

Since #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:

  $types['link'] = array(
    '#pre_render' => array('drupal_pre_render_link'),
  );

  // Links that lead directly to foo.
  $types['foo_link'] = $types['link'];
  $types['foo_link'] += array(
    '#theme' => array('link'),
    '#theme_wrappers' => array(
      'container' => array(
        '#attributes' => array('class' => 'foo-link'),
      ),
    ),
    '#title' => t('Foo here!'),
    ...
  );

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

thedavidmeister’s picture

Issue summary:View changes

Updated issue summary.

pwolanin’s picture

based 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

thedavidmeister’s picture

based 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

wow, 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.

thedavidmeister’s picture

Issue summary:View changes

Updated issue summary.

ParisLiakos’s picture

Issue summary:View changes
mitokens’s picture