Problem/Motivation

One of the key problems with the D7 theme system is that there are 5 ways to do anything, yet no one uses all 5. Most people will rarely touch more than 3, or even only 2. Yet there's plumbing and complexity in there to support all 5.

Functions like l(), theme_image(), and theme_image_style() are used because they are shortcuts to write tags. They offer developers more benefit than themers as they are a shortcut to write common markup.

The only point of sending markup through the theme layer is for themers to override that markup. After the sprint, effulgentsia mentioned that something like theme_link() was added out of a blanket policy that "All markup should go through the theme layer." This policy is too general.

Proposed resolution

Generally speaking, we believe if the markup of a specific link or image tag needs to be customized, that element is part of a larger structure (such as a pager or a menu) and the specifics of that link can be addressed via the parent template.

So, we're proposing that what we're currently calling Markup Utility Functions. We would refactor things like theme_link(), theme_image(), theme_image_style() to be more like l().

Remaining tasks

#1778610: Remove the check for a link template from l(), have l() always output just a string.
#1595614: [meta] Remove all the theme functions and templates in core that simply output a link. Replace with #type 'link' render arrays

User interface changes

None.

API changes

TBD.

Original post by c4rl

This is derived from discussion at the BADCamp 2012 Twig sprint: http://groups.drupal.org/node/265858 Please see these notes for the background.

While starting to convert some core theme functions to Twig templates, we found certain existing theme functions simply output a single element. Some of these looked like this:

  {# link.html.twig -- refactoring theme_link() #}
  <a href="{{url}}"{{attributes}}>{{text}}</a>
  {# img.html.twig -- refactoring theme_image() #}
  <img{{attributes}}/>

As we looked at these, they were suspiciously similar and simple. We began to question the use and purpose of these:

  • How are these useful to themers? Are they at all?
  • Why do they go through the theme layer? What was the origin of this?
  • What is a use case where a themer would have to override every image tag, or every link? Keep in mind we're designing for majority use cases.
  • Why do we have theme_link and theme_image when we don't have theme_paragraph, theme_blockquote, theme_heading, etc?
  • What can we learn from l(), a function that produces markup that doesn't go through the theme layer?

Functions like l(), theme_image(), and theme_image_style() are used because they are shortcuts to write tags. They offer developers more benefit than themers as they are a shortcut to write common markup.

The only point of sending markup through the theme layer is for themers to override that markup. After the sprint, effulgentsia mentioned that something like theme_link() was added out of a blanket policy that "All markup should go through the theme layer." This policy is too general.

We couldn't think of a general use case where overriding every single link tag or every single image tag would be useful. Generally speaking, we believe if the markup of a specific link or image tag needs to be customized, that element is part of a larger structure (such as a pager or a menu) and the specifics of that link can be addressed via the parent template.

So, we're proposing that what we're currently calling Markup Utility Functions. We would refactor things like theme_link(), theme_image(), theme_image_style() to be more like l().

Theme functions to be reconsidered

  • theme_link
  • theme_input
  • theme_image
  • theme_image_style
  • theme_html_tag

Related functions

  • l

Comments

c4rl’s picture

Issue summary:View changes

Add a todo to provide links to related theme functions

c4rl’s picture

Issue summary:View changes

Indicate some steps

steveoliver’s picture

c4rl,

This definitely makes sense.

What to we think of theme_html_tag() in light of this?

jwilson3’s picture

Mmmm. This makes a lot of sense, seems tangentially related to the Theme Component Library tag too. Trimming useless baggage and clarifying out our themable components is huge.

The problem I see here, is that, yes, its true that people dont want to necessarily override the top-level themable, but they want to rather implement a subpattern suggestion, for example, to override a specific image style, Eg. theme_image_style__mystylename, or override a pager link theme_link__pager_first()

Would there be an alternate way to do the above, if we didnt have the top-level themable template?

jwilson3’s picture

Issue summary:View changes

added removal and replacement functions

jwilson3’s picture

The question boils down to a key assumption that isn't 100% defined for the component library. Are "magic" (unimplemented) functions (a la template sub-pattern suggestions such as theme_link__pager_first()) preferred versus a long list of one-off custom template widgets (such as theme_pager_first()) that often reimplement similar underlying html structures. It seems like everything is pointing towards magic template inheritance based around basic high-level simple templates, as opposed to rote duplication, but of course, this pattern breaks down and becomes really confusing when a subpattern actually gets implemented by core, and sets up additional variables in preprocessing, such that the subpattern template file stops looking much of anything like the basic/default parent implementation.

Also, now would be a good time to think about how Twig's "block" feature plays into this.

jwilson3’s picture

Issue summary:View changes

Linked to #1833906.

steveoliver’s picture

Maybe the litmus test for a "markup utility function" is that it must be able to be themed sufficiently *within* wrapper templates or theme_ functions.

sun’s picture

I can only get behind this, if we'd say at the same time:

Do not use l() anymore.

That is, because the "wrapper template customizability" you're talking about here is only a given, if you are actually able to customize the link from within the wrapper template before the link is output.

l(), however, renders the passed arguments directly into HTML markup. 0% customizable.

Thus, to achieve actual customizability, we'd replace all of this:

  return l(t('Log in'), 'user/login', array('attributes' => array('class' => array('login')));

with this:

  return array(
    '#type' => 'link',
    '#title' => t('Log in'),
    '#path' => 'user/login',
    '#attributes' => array('class' => array('login'),
  );

The latter would take those arguments and pass it through l(). The difference is that you can do this in your theme override for the wrapper:

function mytheme_user_login_block($variables) {
  $variables['link']['#attributes']['class'] = array('better-login');
  $link = render($variables['link']);
}

Otherwise, we'd be in PCRE/string-matching hell, trying to preg_replace($markup, '/(class="\)(login)/', '$1better-login')

jenlampton’s picture

Want to know what I think of theme_html_tag?
See #1825090: Remove theme_html_tag() from the theme system

One of the underlying principals established at the Twig sprint this weekend is your markup is not special. If we get this component library correct, there should not be any variations in an item list. All item lists in all of Drupal will use the same template - the one provided in core. If a theme developer *needs* to change one of those, there will be a theme_hook_suggestion available so that they can override it, and this will be visible somewhere (inspector in the twig template, perhaps?) for a front-end dev to see it.

We need to get away from the idea that there are one-off custom template widgets at all. Yes, there may be occasions where they are needed, but those should be rare.

If we get into some crazy use-case where a sub-pattern is different than the parent, we should consider that maybe it's not a sub-pattern at all, but a different pattern, and we should include a component for that as well. Once we have these Markup patterns in core, we can use them consistently and everything gets better.

So far, there are not that many patterns that differ. Another principal we decided on was Build from use cases which means that we are not going to put a bunch of stuff in core for the "what-if" scenarios. We're going to solve only real problems, and also provide tools so that contrib can override when it needs to.

By the time any link is output in a template, it can be overridden. Here is some Twig psuedocode for a div with a link in it:

<div>
{{ link }}
{{ content }}
</div>

If you want to override all or some of that link, you just do this:

<div>
<a href="{{ link.href }}" class="{{ link.attributes.class }}"{{ link.attributes }}>{{ link.text }} foo bar</a>
{{ content }}
</div>

We also talked about adding a hook_l_alter() function that allowed l() to be altered by modules, but we decided not to do this unless anyone could come up with a viable use case. And so far, no one has.

jwilson3’s picture

Idea: what if we added a "suggestion" option, and the output of l() returned a render array:

array(
    '#type' => 'link',
    '#suggestion' => 'login',
    '#title' => t('Log in'),
    '#path' => 'user/login',
    '#attributes' => array('class' => array('login'),
  );

That way coders get to keep their shorthand l() notation, and themers could implement link--login.tpl.php

sun’s picture

By the time any link is output in a template, it can be overridden.

This is just simply not true. The twig templates you're suggesting there do not work and cannot work — at least not with functions like l(), which produce a HTML string, not render arrays or twig objects. I've outlined the details in #5.

steveoliver’s picture

#6 re: theme('html_tag'): precisely.

re:

By the time any link is output in a template, it can be overridden.

...not so in the case of links, if we are to *not* use preprocess on them.

Take the example of theme('link_pager'):

It needs to remove the 'active' class.

<a href="{{ link.href }}" class="{{ link.attributes.class|remove('active') }}" {{ link.attributes }}>{{ link.text }} </a>

Since exloding a link object like this is not possible without preprocess, it seems that one task of generalizing the l function would then be to delegate the adding of the 'active' class to the caller(s) of l that need it.

steveoliver’s picture

well, but leaving 'theme_link' can allow what you're talking about in #6 though, Jen.

steveoliver’s picture

Issue summary:View changes

Linked to #1595614

Fabianx’s picture

I just want to give this all a different perspective again:

There are three things that need to be taken into account for this:

  • Performance
  • Flexibility
  • Simplicity

The reason why l() is like it is, is because it is fast.
The reason why l() calls theme_link if a preprocess function or template exists is for flexibility.
The reason why l() is used in code is for simplicity.

Our task now is it to retain those goals and still change the structure.

* l() is definitely a performance critical function, so having it return raw markup makes sense.
* There are cases as @sun pointed out in #5, where users need to overwrite link properties, like adding a class.
* l() is a well known Drupalism and most users love it.

Using render arrays() instead is not gonna solve this problem, but only make it worse, because going forward I will need functions to declare render elements, so having l() return a render array is a no go.

Changing the code from l to the render() array however could work, provided that the code is advertising the array element.

#5: How would a user solve the overriding of the class for _just_ the user-login-block today?

That might shed some light on this ...

Fabianx’s picture

Issue summary:View changes

replaced #1833906 with #1778610

c4rl’s picture

Title:[META] Markup Utility Functions» [meta] Markup Utility Functions

Thanks to everyone for the feedback so far. I'll try to address the concerns at hand.

I want to first mention two of the key principles we established at the BADCamp sprint (see link in summary for principles):

Build from use cases
Don't assume you know what people want or add features based on "What-if?" Think about the 90% of use cases.
Provide tools
Give front-end experts a way to achieve specific goals; goals that apply to the remaining 10% of use cases. Keep in mind that complex problems may require complex solutions.

I also want to clarify that the intention of the proposed change is with some attention to the audience that uses these functions. I am both a themer and a module developer, and I believe the utility of these functions is primarily for module developers to construct common tags easily rather than for themers to override the resulting markup.

Comment #1:

What to we think of theme_html_tag() in light of this?

Definitely related, I've added it to the summary.

Comment #2:

The problem I see here, is that, yes, its true that people dont want to necessarily override the top-level themable, but they want to rather implement a subpattern suggestion, for example, to override a specific image style, Eg. theme_image_style__mystylename, or override a pager link theme_link__pager_first()

Would there be an alternate way to do the above, if we didnt have the top-level themable template?

I have a few thoughts here. Firstly, I want to bring up the principle of "Build from use cases." I think it is dangerous to simply remark "people want/don't want X." We need practical examples here. Can you provide some, @jwilson3? I'd be interested to hear your use case in which theme_image_style__mystylename is useful, so please elaborate. In the case of theme_link__pager_first() this seems like part of a larger markup structure in which the markup of subelements can generally be addressed via the parent template (or preprocessor), but I welcome examples to the contrary.

Comment #5:

The latter would take those arguments and pass it through l(). The difference is that you can do this in your theme override for the wrapper...

@sun, Thanks for the practical feedback here regarding technical implementation. You are correct that one must expose underlying variables for a particular link to the parent template if it should be available. I will remark that it seems l() may has some valid use cases as simply a helper function. For example, static links in hook_help or in the admin/reports/status table. Developers should consider the semantic context of the links they wish to show; it seems that l() may have a place in translatable placeholders or whether url() suffices. Consider the following:

t('Status: !status_link', array('!status_link => l($status, $status_href))
t('Status: <a href="!status_link">@status</a>', array('!status_link' => url($status_href), '@status' => $status))

However, one point I do want to make is that the use case you showed for mytheme_user_login_block(), is that many non-Drupal front-end folks have no clue what to do for this and will add/subtract classes in the *.html.twig template itself rather than adding to the #attributes and calling render() in PHP.

I don't want to get into many implementation details yet, but perhaps l() or something like it could just be helper to return a renderable array?

Comment #7:

That way coders get to keep their shorthand l() notation, and themers could implement link--login.tpl.php

To echo my response to comment #2, without a use case this seems overkill, but I will give this some more thought.

Comment #8:

functions like l()...produce a HTML string, not render arrays or twig objects

Yes indeed. I definitely do not think the output of l() should be sent to a template if one expects to override it. The context needs be considered here, and as mentioned in my reply to comment #5, I'd like to give this more thought myself.

Comment #9

It needs to remove the 'active' class.

This is not a job of the theme system, but rather a bug in l(), which I believe is addressed by #1410574: Make l() respect the URL query string before adding the 'active' CSS class

Comment #11

@Fabianx, thanks for outlining some key points.

The reason why l() calls theme_link if a preprocess function or template exists is for flexibility.

While it is true that theme_link does exist for flexibility, the purpose of me opening this issue was to question the necessity of the flexibility for themers in the 90% of use cases, and whether that flexibility would live elsewhere than the theme system.

As we are converting things to Twig, we are looking to consolidate and reduce duplication and complexity. So, I'd like to focus this discussion on use cases that require flexibility, where the flexibility should live, and then steps to implement.

At minimum we'll proceed to convert link.html.twig (and others) for the sake of getting things functional and then see what refactoring can result from this discussion.

Fabianx’s picture

While it is true that theme_link does exist for flexibility, the purpose of me opening this issue was to question the necessity of the flexibility for themers in the 90% of use cases, and whether that flexibility would live elsewhere than the theme system.

I think this is the key point.

We need to look at use cases:

Lets take @suns example and now we need to show how we would do this with TWIG - with code -- without changing core.

steveoliver’s picture

Title:[meta] Markup Utility Functions» [META] Markup Utility Functions

Re: #5:

One goal of the Theme Component Library is that "wrapper template customizability" is *not* a given but rather a pattern for dealing with these small chunks of markup. I propose we encourage core and contrib to utilize these markup utility function and prefer customizations to wrappers around their markup. Markup utility functions draw a line in the sand by saying "this can't be altered".

In your example of the user login link, I'd like to see no special classes in the link, but rather in the wrapper, like this (from gist):

{# modules/user/templates/user-login-block.html.twig #}

{% block "login-link" %}
<div class="user-login-block-link">
  {{ link }}
</div>
{% endblock %}

{# themes/stark/templates/user/user-login-block-better.html.twig #}
{% extends "user-login-block" %}

{% block "login-link" %}
<div class="user-login-block-link-better">
  {{ link }}
</div>
{% endblock %}

If something really needs to be altered, the client code can call a non-"utility" theme function, in this case theme('link__user_login_block').

Yeah? No?

Crell’s picture

One of the key problems with the D7 theme system is that there are 5 ways to do anything, yet no one uses all 5. Most people will rarely touch more than 3, or even only 2. Yet there's plumbing and complexity in there to support all 5.

With Twig's flexibility and the performance tradeoffs of functions vs. templates totally different now, I am all for simplification by eliminating some of those options in favor of "you want to customize it? Here's a Twig template." Utility functions for common HTML that don't use the theme system sounds fine to me.

What does not sound fine is taking some of our few useful utility functions and making them worse by turning them into render arrays. Those are in decline, and if SCOTCH is successful you'll never deal with a template or render array larger than one block, OR one layout. hook_page_alter has no new-style equivalent. As such, render arrays become far less useful or viable. (Really, they suck as much for module devs as they do themers.)

The other question I'll throw out is at what point do we say "no, this part isn't flexible because it's just too much work to make flexible". To date, our answer has been "never", and that's how we ended up with 5 ways to change anything. As multiple others have said above, if it's not in the 80% or 90% use case, don't do it.

Would that include, say, l() generates a link and that's the end of it? Just don't let that specific tag be modified? Or whatever theme_image() turns into? Or these other "utility functions that aren't really theme functions"? I don't know, but please let's make sure to ask the question.

Crell’s picture

Issue summary:View changes

Removing some dramatic language, i.e. "chopping block." Adding theme_html_tag as a candidate. Removing "image()" since we need to clarify goals before we focus on implementation.

steveoliver’s picture

Crell, re #15:

What does not sound fine is taking some of our few useful utility functions and making them worse by turning them into render arrays.

100% agree. That's a step in the opposite direction.

Re:

at what point do we say "no, this part isn't flexible because it's just too much work to make flexible"

The time is now.

The way to deal with "Well I wanna change the style of this link or image!" is "Work in the wrapper template."

And with #1820158: Print template names in HTML comments, we'll have template names/paths in HTML comments for front end devs to find.

Also, c4rl: I've updated the issue summary using the Issue Summary Template.

sun’s picture

  1. An argument/principle and decision factor has been raised, which I think can be safely ignored — #11 states:

    * Simplicity
    ...
    The reason why l() is used in code is for simplicity.

    Putting back on my other developer hat — as a developer I do not care at all whether I'm calling l() or whether I'm supplying a render array anywhere. In fact, I rather find it mentally disturbing when I come across an l() or any other pre-rendered HTML markup string that is surrounded by tons of other code that does not do that. It further confuses me and especially newbies with regard to as to whether this is good practice or bad practice and what I'm ultimately supposed to do as a developer to make my themers happy.

    So I'd highly recommend to drop simplicity from the list. And much rather, replace it with Consistency.

  2. I certainly agree that the primary use-case for theme overrides of very generic theme functions like link or image are the specialized variants (i.e., 'theme_link__pager' vs. 'theme_link').

    I need to be able to override, e.g., all links that appear in menus, or in a certain menu — without overriding all links. I need to do that to implement my theme CSS and/or JS, and I also need to do that when I'm trying to integrate some JS dropdown menu library for a particular menu.

    The same applies to images. These examples are not made up, but rather very common tasks for me as a themer. (I have no idea why anyone would need "concrete evidence" for these tasks and it is a waste of time for me to bikeshed whether these are actual themer tasks.)

  3. Note that the secondary use-case of being able to override generic theme functions is what enabled HTML5 markup in D7.

    Given a unspecified spec that HTML5 is, this is actually where generic, overridable things like theme_html_tag() shine: Whatever insanity happens to HTML5 between 2013 and 2019 (predicted EOL of D8), you will be able to bend Drupal to do what the spec asks for. Drupal core will not and cannot.

Fabianx’s picture

#17:

1. Okay, consistency then.

2. I certainly agree that the primary use-case for theme overrides of very generic theme functions like link or image are the specialized variants (i.e., 'theme_link__pager' vs. 'theme_link').

But those would not be lost if l() was always hardcoding the markup instead.

The reason we have this discussion is:

l() is fast, theme_link is flexible, but now l() uses theme_link as soon as "anything" describes a preprocess function for theme_link or a template.

And suddenly l() is no longer fast, but also not really flexible, because it is too generic.

That is the "insanity" - that at least I see.

If one wants to use l(), use it, but then markup cannot be overridden. period. There are no suggestions for this anyway.

If one wants to use theme_link or a render array with #theme link that is possible.

I cannot prevent someone from currently writing:

$x['#markup'] = '<a href="$myurl">$mylink</a>';

or similar, so if you use l(), or any other markup utility function, the output is not over writable and if that is your goal, you are doing it wrong like if you hardcoded the HTML.

But if you use theme link it is overridable, suggestible, etc.

And I don't think all l() functions need to change for that in core. Most of those links are not meant to be overwritten (as far as I can see).

3. theme_html_tag is a valid case

Can you add a link to html5 for Drupal 7 use case in contrib? Interested to learn more ...

sun’s picture

Well, now we're making progress. :)

Because:

If one wants to use l(), use it, but then markup cannot be overridden. period. There are no suggestions for this anyway.

If one wants to use theme_link or a render array with #theme link that is possible.

This is factually in disagreement with the two "remaining tasks" being listed in the summary:
#1778610: Remove the check for a link template from l(), have l() always output just a string.
#1595614: [meta] Remove all the theme functions and templates in core that simply output a link. Replace with #type 'link' render arrays

What you just said means: theme_link() will not be removed.

Instead, it is purposively kept, and actually without any change; i.e., for the exact same purpose it has now:

1) Allowing '#theme' => 'link' in render arrays.
2) Allowing themes to override links of a specialized type via 'link__foo__bar'.

Fabianx’s picture

#19: Yeah, I am not necessarily in agreement with the new markup utility function parts and I think going the route of consolidation makes sense as @jenlampton makes the wrong assumption that l() markup is still changeable.

I think overall just keeping _one_ template and using suggestions makes more sense, but lets please remove the l() optimization to fall back to a template.

That is a misuse of l(), not of theme_link, which is fine for the more specialized cases and a very good example of component library part as it can be customized as much as one wants.

However I just understood this through this issue.

sun’s picture

Yes, that makes sense to me.

It would inherently imply that there's no change to theme_image() though, since there is no equivalent low-level helper function producing direct/raw HTML strings for that. (In fact, to my knowledge, l() is the only one in core.)

Somewhere earlier in this thread, someone mentioned the idea of only using l() on administrative pages, or perhaps rather, only in situations where no one could reasonably want to override the link — because sufficient theming/CSS context is available through the wrapping markup already, or, because the classes/attributes of the link can be easily altered through a wrapping theme function/template already (e.g., considering a list of links that goes through theme_links()). I think something along those lines would make sense, but we'd have to explain this a bit more concretely very early in the phpDoc for l() then.

That said, one of the potential outcomes of the theme function consolidations is that duplicate wrapper functions, such as theme_menu_local_tasks() + theme_menu_local_task() - which together produce a simple item list containing links — AND which is using l() currently — will be merged into a generic list theme function (using a subpattern, so the specialized list of page tabs/tasks can still be overridden) — but at the same time, the generic list theme function will no longer use l(), because it acts on list items with varying contents and attributes, which will have to be render arrays then.

In turn, this might very well mean that l() is going to be used a lot less in the end. Usage could potentially even drop so significantly that it might make sense to remove it. (Oh. Did I just opened the view to an entirely different horizon? :P)

That would first have to be analyzed carefully, of course. However, with regard to that concrete example of menu local tasks, the switch to the generic item list function + render arrays is guaranteed to be faster, since the current list goes through two — it's worse actually, it is 1 list + N, one for each local task item — theme functions instead of a single list theme function. — And now I realize my math is wrong, because each link in the list would go through drupal_render() and thus through #theme, each. However, we could certainly consider to add a special case for #type/#theme 'link' into drupal_render() itself (in the same way we special-case #markup already).

c4rl’s picture

Priority:Normal» Major
Issue tags:-Twig, -theme system cleanup

#14 @steveoliver, I'm glad you brought this up, but we do risk gripes of relying on wrappers. It's a valid point you make, though.

#21 @sun, I had many of the same realizations later today, so it seems we're converging onto some foundations.

As the author of the original issue, I really appreciate everyone's thought and feedback that's going on here.

I started working some code this afternoon to get a proof of concept and found that this is a hard problem to solve. :) I did some more thinking about everything we've discussed and want to summarize a bit.

The inspiration behind this issue was part of the process in converting our existing theme functions to Twig, and I noticed some very bare bones templates that, well, just looked like HTML tags. They seemed so minimal that I questioned the necessity (we were questioning the necessity of many things at the sprint). The thinking went: "Suppose this didn't exist? What are the implications?" We've explored many already. I'd like to highlight a few.

Structure must exist if we want links to be drillable via Twig objects (indicated in comment #5)

We should make sure that l() is avoided. Consistency is one of the principles we established at the sprint (see link in summary).

Altering can to happen in the right places

Themers do need control of *most* markup. With drillable arrays in Twig, we may see more usage of accessing variables directly in parent templates (which is arguably more intuitive to front-end folks) rather than theme_link__foo_bar__baz(), both of these potentially having the same outcome, e.g. changing a class. As we being to refactor some existing theme functions/templates into Twig, I would encourage using a technique of avoiding calls to both l() and theme_link() in templates such as menus and pagers to see how this works out. Again, arrays must define links for this to work (whether we use alter hooks in native PHP or Twig objects in Twig templates).

Are Theme Components the answer?

Considering an array structure for links being renderables in the context of a Theme Component Library, I realized that there are components we will likely define for developers to use to build UI and markup.

Consider for example, the administrative list of content types, in which we have the "Add content type" link at the top. This pattern, (being a button to create an entity/bundle/configuration/etc), would certainly qualify as a pattern we'd want developers to use and, indeed, could exist as a Button Component, or even simpler (drumroll) a Link Component. This conclusion, somewhat ironically, then justifies the usage of a template to provide this particular component as a design pattern.

So, when looking at link.html.twig taking this Component perspective into account seems to shine a new light here. I think there's some more thinking to do with regard to this, but we should indeed make sure that the component pattern is used *appropriately,* as indicated by #21 and my remarks here. That is, it's not one-size-fits-all to be used everywhere, especially since drillable Twig objects potentially offer help here to affect the available links in a larger structure like menus and pagers.

This is a meta issue mostly for the sake of conversation. Let's make progress with Twig conversion, but continue to consider the role of theme simplicity vs flexibility. I'm happy to postpone this issue for now unless others feel there's more to discuss.

c4rl’s picture

Tags went missing

c4rl’s picture

Priority:Major» Normal

Changing status to normal. We've uncovered some interesting ideas here, but I think the Twig conversion as-is can proceed without having this be a blocker.

c4rl’s picture

Priority:Major» Normal
Issue tags:+Twig, +theme system cleanup

After discussing this issue with jenlampton we'd like to propose some steps to guide the current work on weighting theme_link() and l() for consolidating theme functions presently.

There are too many theme functions that simply output a link: #1595614: [meta] Remove all the theme functions and templates in core that simply output a link. Replace with #type 'link' render arrays. jenlampton and I agreed that, in the name of progress, we should simply convert the majority of these to use theme_link(), or l() if they simply are calling l(). This way we reduce the number of theme functions, which is definitely a step in the right direction, and preserve existing functionality. Though my original intent questioned theme_link(), I do see some merit as I explained in #22.

During the consolidation, we should definitely attempt to see if it is feasible to consolidate some theme_link() calls as drillables on the parent template for simplicity.

It is pertinent to consider #1778616: Add an .active class for links rendered by theme('link') that have an href to the URL being viewed and sanitize output of url() here as well.

So, for now, let's proceed to clean-up the existing theme functions to use theme_link() as it applies.

catch’s picture

With Twig's flexibility and the performance tradeoffs of functions vs. templates totally different now, I am all for simplification by eliminating some of those options in favor of "you want to customize it? Here's a Twig template." Utility functions for common HTML that don't use the theme system sounds fine to me.

This isn't proven yet - we don't know what the tradeoff will be between a theme function and a template, we've only prdofiled template vs. template and it's entirely possible we'll run into situations (and links are a likely one) where Twig templates are still expensive.

I don't think it's necessarily good for performance that l() returns a raw HTML string. There are lots of situations where l() gets called, but the HTML it produces never gets rendered. So something which saves expensive processing for when it's actually needed might be an OK trade-off to make.

steveoliver’s picture

To try to summarize my understanding of the issues and proposed next steps:

Regarding links and l vs. theme_link:

  1. Clean up l function to remove theme_link fallback functionality.
  2. Use l when most use cases won't need to alter links directly.
  3. Use l or (more likely) theme_link function to consolidate existing link themables.

The implication of these is that l can be described as a (the first) Markup Utility Function which should be preferred for performance, and theme_link as the alternative to l for developers who wish to maintain alter-able links all the way down to the theme layer.

Also, with l/theme_link as examples, *every* markup utility function would have an alternate theme function, with doc blocks describing the two+ related options, similar to the existing doc blocks for l and theme_link.

Lastly, catch, in #26, you wrote:

I don't think it's necessarily good for performance that l() returns a raw HTML string. There are lots of situations where l() gets called, but the HTML it produces never gets rendered. So something which saves expensive processing for when it's actually needed might be an OK trade-off to make.

Beside the fact that some generated markup never gets rendered (which I think is a tangential but slightly different issue than this one), I don't see how l can not be good for performance, when compared to a callback such as theme_link that dives into the theme layer.

Are you giving preference to the invokation of url, Attributes, etc. and generation of a link /when it is actually needed/ in a template file vs. the pre-generation of a link with l before the template decides whether or not to print the link?

ry5n’s picture

I’ve been following this issue with interest, but without a good understanding of the plumbing involved. As a themer I’ve rarely called l() or theme_link() directly in a template. However, re-reading the issue summary this morning reminded me of a use case we should account for. Keep in mind that I'm not sure this is a problem, but I thought I'd bring it up as something to consider.

The folks at Filament Group (who did the Boston Globe, the first large-scale responsive site) have a markup/js pattern for including 'nice-to-have' content on pages with larger viewports. Example: a small-screen page displays a chunk of discrete secondary content, such today's weather, as a simple link by default. When appropriate, some JS is invoked based on some data-attributes on the <a> that fetches that content and appends it to the DOM.

The important thing is that this pattern may see widespread use among Drupal's enterprise users, and it involves adding unique data-attributes to the link directly, which may be in conflict with the idea that (from the issue summary) "if the markup of a specific link or image tag needs to be customized, that element is part of a larger structure (such as a pager or a menu) and the specifics of that link can be addressed via the parent template."

catch’s picture

Beside the fact that some generated markup never gets rendered (which I think is a tangential but slightly different issue than this one), I don't see how l can not be good for performance, when compared to a callback such as theme_link that dives into the theme layer.

I'm not talking about whether we call into the theme layer or not, just whether we build the HTML immediately or not. An l() replacement renderable object could make various things like attributes or theming optional.

steveoliver’s picture

Ryan, re: #29:
A developer can pass any HTML attributes in to l() or theme('link'); A designer can modify those attributes if the developer used theme('link'), either in preprocess or in the template. A designer can not modify those attributes if the developer used l().

Catch, re: #29:
Ooooh, now we're talking.

<?php
class l extends markupUtilityObject {
  private function
__construct($path, $options, $attributes) {
   
// ...
 
}
  public function
href() {
    return
check_plain(url($this->path, $this->options));
  }
 
// ...
}
?>
<?php
  $variables
['link'] = new l($path, $options, $attributes);
?>

Implemenatation, where ry5n wants to add 'data-' attributes.

  {# link--custom.html.twig #}
  <a href="{{ link.href }}" data-media="(min-width: 30em)" {{ link.attributes }}>{{ link.title }}</a>

?

Crell’s picture

I was just thinking along the same lines as #29 and #30! That should be a strong indication that it's the right direction if we're all arriving at that conclusion. ;-)

Making l() and friends an object (although likely named differently) also helps with the DX issue of the big anonymous array at the end of it. Just make those separate chainable methods, and the code for the developer becomes more self-documenting *and* it's easier for the themer. They should use the same __toString() approach as the ArrayAttributes object. To wit:

<?php
class Link {
  public function
__construct($path, $title) {
   
// Assign both to properties.
 
}

  public function
setExternal($external = TRUE) {
   
$this->external = $external;
    return
$this;
  }

 
// And various other methods for setting things, eliminating the $options array

  // Getters for the parts a themer would want if they're doing it themselves
 
public function getHref() {
   
// Wrap to url(), or actually the generator object when that gets in, which is then an interesting
    // external dependency problem to solve.
 
}
 
  public function
getTitle() {
   
// Also need to think about the dependency.
   
return $this->alreadyEscaped ? $this->title : check_plain($this->title);
  }

 
// And others for classes, etc.

 
public function __toString() {
   
// This would be more complicated of course.
   
return '<a href="' . $this->getHref() . '">" . $this->getTitle() . "</a>\n";
  }
}
?>

Then in Twig, you can either do:

{{ linkvar }}

To get the default a element in all its Drupally glory, or:

<a href="{{ link.href }}" data-media="(min-width: 30em)" {{ link.attributes }}>{{ link.title }}</a>

To take over and pw0n it yourself (and take the risk of forgetting a piece, like the RDF stuff).

That gives themers a lot of template-level flexibility, and developers a clean, OO API for managing links that is better than both l() with a big array of anonymous options and a render array.

Front-end folks: Hate me yet?

c4rl’s picture

Status:Active» Postponed

The intention here was not really to refactor l() to be OO, though it is interesting. As the author of the original issue, I'm going to postpone this for now, and I'll explain my reasons. There are other issues in the queue that have also been postponed with regard to this discussion, and jenlampton and I will be going back through these to determine the direction forward.

First let me address some of the recent comments.

#28 @ry5n, thanks for sharing this, though I don't want to derail conversation too much. I think your point is that we need tools for customization and I definitely agree with you. Since these examples relate to content rather than appearance, they seem applicable to the render array that will be used or a custom preprocessor.

#27, #30 @steveoliver, I definitely appreciate your enthusiasm and contribution to the discussion, so thanks for your participation. I agree in general to your three action items outlined in #27. However, #29 although interesting, is beyond the scope of what I was intending, so I feel things are heading a bit outside the lines.

The intention was to say "What if we removed X? Let's discuss." The discussion has been useful and we've learned some things that I highlighted in #22. These items resolve much of my original question: we can't just throw away theme_link() quite yet, though we will likely see l() used less and other related theme functions consolidated.

In the name of progress, I'd like to focus our efforts elsewhere in the Twig conversion. That process may shed some light here in the future, but until then, we'll postpone.

steveoliver’s picture

c4rl: Cool.

Maybe #1778610: Remove the check for a link template from l(), have l() always output just a string. could use an update and Title change (currently "Deprecate theme_link in favor of l markup utility function").

sun’s picture

#31 looks interesting, but I'm concerned that a full-blown object for every single link, along with requiring multiple method calls to set up a simple link, will be detrimental in terms of performance.

However, in light of that, I could get behind an object that is only prepared for rendering, but doesn't involve method calls and processing until it is actually used, or rendered:

class Link extends Element {
  /**
   * @todo Copy current l() @param docs.
   */
  function __construct($title, $path, array $options) {
    // Save passed in arguments. They will only be processed when attributes are
    // accessed or when this element is rendered. Otherwise, nothing happens.
    $this->content = $title;
    $this->linkPath = $path;
    $this->linkOptions = $options;
  }

  // From Element class:
  function __toString() {
    $this->ensureProcessed();
    return '<' . $this->tag . new Attribute($this->attributes) . '>' . $this->getContent() . '</' . $this->tag . '>';
  }
}

ry5n’s picture

@steveoliver (#30) Thanks for the summary, that helps :)

@c4rl (#32) Sounds good; my comment in #28 was to bring up a use case that might not be typical, but should be possible. I'm happy.

Fabianx’s picture

Agree with #34, that is what I thought JenLampton had thought l() was doing :-).

I think however we should not blindly return this on l(), but rather use our Link component on a case by case basis.

Therefore we would have:

* l() - raw output
* theme('link__suggestion') - for overridable template
* new Link() for link within base structure, but also allow access to "rendered" properties.

The new Link() makes very much sense for example for theme("pager") instead of the render arrays for first, last, etc.. Performace for lazy rendering object vs. render array should be the same in terms of function calls.

Also this Link() now is the first thing that actually is a component of like a Theme Component Library (yay!).

I like this.

steveoliver’s picture

Definitely cool stuff. I always wondered how we'd do lazy loading of variables only when we need them. Link is nice and simple, but maybe deeper/more complex data structures will see real performance benefits.

Let's fire up a "[meta] Renderable Objects" issue? If no one else gets to it before tomorrow, I'll pull a summary from the ideas in these last few comments.

catch’s picture

I opened #1836730: Add a renderable object that is equivalent to l() to split out the l() discussion.

jenlampton’s picture

@steveoliver I searched for your meta issue on renderable objects but couldn't find one so I created #1843798: [meta] Refactor Render API to be OO

It's also related to the work I'm doing over in #1843650: Remove the process layer (hook_process and hook_process_HOOK)

thedavidmeister’s picture

I wanted to provide another use-case for having a functional theme('link') that I'm running into in #1898420: image.module - Convert theme_ functions to Twig that i don't think is discussed here, or if it is it's "implied" by what people have been saying and I think it should be made explicit for newcomers to this issue.

So, I have a twig template that renders a link if a path is provided or just the "raw" content if no path is provided, something along the lines of this:

{% if linked_content %}
  {{ linked_contentvar }}
{% else %}
  {{ contentvar }}
{% endif %}

Basically, for a template like this to have much "real world" usefulness, linked_contentvar and contentvar can't just be pre-rendered strings - you need to expose their structure to the template or all you can really do is wrap the whole thing in some static HTML. Since the content of one is identical to the other but wrapped in a link, which for argument's (testbot's) sake *must* behave as though it was rendered by the current implementation of l(). We can't simply do this <a href="{{ url }}">{{ contentvar }}</a> because we lose both the .active class on the a tag and the extra sanitization/preprocessing that l() provides.

l() doesn't accept render arrays for content, so we can't do this even if we wanted to by enabling l() as a twig function (I could be underestimating twig's capabilities here, let me know if I'm wrong about this):

  {{ l( contentvar, url) }}

So, IIRC the only way to keep the structure of the content we want to render wrapped with a "Standard Drupal Link" preserved all the way to the template is to create a working theme_link() that is decoupled from l() and start using it in these situations.

AFAICS this is a new problem introduced with D8 converting everything into twig templates, previously we side-stepped the issue by using theme functions instead of templates when we wanted to do most of our content processing "up front" and cheekily wrap what we'd produced in l() right at the end as we return the string we've rendered.

This use-case is different to what has been discussed previously as here we want to preserve the customisability of the structure of the *content* of the link, rather than the customisability of the structure of the <a> tag itself.

I believe this provides more motivation for at least the summary of proposed changes in #27 - doesn't have any bearing on the "OO as a third option" discussion.

jenlampton’s picture

@thedavidmeister I don't see anything in your explanation that justifies a theme('link'). What I see is a need for an alterable l(), and that's something that I could get behind. Making a renderable thingy for l() is a good alternative to a theme('link'). I still haven't seen a valid use case for wanting to theme every link on your site. Until I see one of those, then death to theme('link')!

thedavidmeister’s picture

#41 - Is it really just about alterable l()? I believe the situation is a little more gnarly than that.
Could explain to me a bit better how an alter would work to fix this use-case "properly".

Problem:

- We want to turn theme function X into a twig template Y without modifying other existing behaviour.
- Theme function X either returns rendered content, or the same rendered content wrapped in a link.
- Having two templates, one template for content and one for linked content is not an option.
- Template Y should be equally flexible, allowing *drillable* content editing by front-end developers whether the output is to be a link or not.

You're saying (I think) that your desired/best-case solution is to make developers do this if they want to change the *unlinked* content in the template:

- If the content is not a link, use the drillable structure provided by the preprocess function within the template as normal to make changes.
- If the content is a link, print the flat, pre-rendered version of the content that is squashed into a string by l() in the preprocess function as a "black box" in the template file. Implement an alter for l(), slowing down every link on the site a little bit as it now has to call your function. Inside the alter reproduce whatever arbitrary changes you made to the un-linked content through raw string manipulation (because l() doesn't support renderable arrays passed as content).

That sounds like poor DX to me..

The reason I suggested getting theme('link') working as it's own "thing" is because it would actually be useful if it could:

- Be used immediately and swapped out later pretty easily for some other implementation of array('#theme' => 'link') in renderable arrays if the markup utility function path leads to an alternative for '#theme'.
- Handle renderable arrays passed to it as content as well as just plain text strings, allowing this kind of thing to be preserved all the way to the template:

<?php
 
array('#theme' => 'link', '#text' => array('#markup' => 'foo'), '#path' => 'example.com'));
?>

- Be removed from l()
- Not be poorly documented, tempting themers to shoot themselves in the foot and bypass url sanitisation in their links.
- Be consistent with how we currently structure every other renderable array

thedavidmeister’s picture

Building on what @jenlampton said in #41, would it be fair to say that since MUFs are not themeable, they should all be alterable? That ties in with the developer-not-themer focus and keeps things flexible.

thedavidmeister’s picture

Issue summary:View changes

Reformat with Issue Summary Template, adding Crell's comment in #15 to the Problem/Motivation.

jenlampton’s picture

Issue summary:View changes