Problem/Motivation

This is a follow-up from #1922454-124: Gut l(), fix theme('link'), have a working framework for consistently generated links inside and outside of Twig. #theme => link uses #path and #text while #type => link uses #title and #href, which is confusing.

Proposed resolution

Consolidate these to common terms (TBD).

Remaining tasks

* Decide what the terms should be.
* Patch to replace all usages of these in core.

User interface changes

None

API changes

Keys for render arrays and form API arrays will be consistent.

#1922454: Gut l(), fix theme('link'), have a working framework for consistently generated links inside and outside of Twig Assigned to: catch

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett’s picture

From D7 to D8, theme_image went from 'path' to 'uri'.

I'd like to see 'path' become 'href' here.

thedavidmeister’s picture

I'd like to see 'text' be 'content' and 'path' be 'uri'.

tim.plunkett’s picture

Content is used nowhere else in core that I know of.

AFAIK, drupal_pre_render_link doesn't accept URIs, just URLs. Meaning, youtube:// would not work.

thedavidmeister’s picture

It accepts Drupal "internal paths" and absolute paths - whatever url() accepts. but 'text' is wrong as it is really 'inner HTML'.

tim.plunkett’s picture

Yes, url() doesn't accept URIs.
theme_image() calls file_create_url(), which is why it accepts URIs.

The current 'text' value is only 'inner HTML' if you specify 'html' => TRUE in the options.

#title and #text could easily just become #value.

thedavidmeister’s picture

'value' and 'href' sound fine to me.

Fabianx’s picture

Priority: Minor » Normal

href is wrong as it is run through url.

It is for most purposes an internal drupal path.

E.g. I have seen: <a href="http://www.drupal.org">Some text</a> more than <a href=" . url('node/1') . '">Some text</a>';

It is href only after it has been "processed".

value is in general fine to me, but might be confusable with #type = value.

I like content also the most and it is used for nodes:

{{ content.comments }}

and IIRC also comment template.

I think inner HTML == content, more than anything else. However content always can / should also be a render array.

( need to see if type_link or l() can support this as well )

thedavidmeister’s picture

Priority: Normal » Minor
Issue tags: +theme system cleanup
thedavidmeister’s picture

#7 - you're right, the variable names should be pre-process not post process.

thedavidmeister’s picture

Title: Consolidate arguments in #type => link and #theme => link » Remove theme_link()

What do you guys think of just removing theme('link')?

#theme 'link' is:
- slower than #type 'link' (probably, we should profile this but I'd be surprised if it wasn't)
- not used by anything in core or contrib (#1187032: theme_link needs to be clearer about saying not to call it directly was open until very recently)
- about as useful as theme_html_tag() was and we got rid of that
- only going to get slower if we try to convert it to a Twig template in the future (that's something we *have* profiled)
- providing no DX advantage over calling l() directly now that l() is both alterable and accepts render arrays as $text
- blocking things like #2005970: In renderable arrays, #type should provide a #theme suggestion, similar to how drupal_prepare_form() works as long as it has different parameters to #type 'link' (I'm bumping this up from minor to normal because of this)

Removing would be the ultimate "consolidation", right?

thedavidmeister’s picture

Status: Active » Needs review
FileSize
14.09 KB

this will fail but i want to see where.

Status: Needs review » Needs work

The last submitted patch, 1985470-11.patch, failed testing.

thedavidmeister’s picture

Status: Needs work » Needs review
FileSize
22.27 KB

try this.

jenlampton’s picture

Status: Needs review » Reviewed & tested by the community

Applied the patch and tested by clicking around the entire site, adding a link field to content and testing the link field. Everything seems to be working as usual.

This is awesome. :)

catch’s picture

Status: Reviewed & tested by the community » Needs work

Patch doesn't appear to remove this from drupal_common_theme()?

thedavidmeister’s picture

Status: Needs work » Needs review
FileSize
22.67 KB

@catch, not sure how I missed that, thanks!

catch’s picture

Status: Needs review » Reviewed & tested by the community
catch’s picture

Title: Remove theme_link() » Change notice: Remove theme_link()
Priority: Normal » Critical
Status: Reviewed & tested by the community » Active

Committed/pushed to 8.x, thanks!

Will need a short change notice.

scor’s picture

Issue tags: +Needs change record

update tags (normalize to "Needs change notification")

catch’s picture

cconrad’s picture

Assigned: Unassigned » cconrad

Trying to write change notice (IRC office hours, nick cconrad, mentor heddn)

cconrad’s picture

Assigned: cconrad » Unassigned
Status: Active » Needs review

Suggested change notice - needs thorough review:

Summary:

  • theme_link has been removed from core in order to improve rendering performance and because it no longer provides any benefits over calling l() directly.
  • The attributes '#text' and '#path' have been changed to '#title' and '#href' respectively in order to provide consistent attribute naming throughout the Render API and Form API.
  • Themers should replace '#theme' => 'link' with '#type' => 'link' in any render arrays, while also changing '#text' to '#title' and '#path' to '#href'.
  • See Change notice: Remove theme_link() comment #10 for the reasoning behind this change.

Code example:

Before (D7):

$link_array = array(
	'#theme' => 'link',
	'#text' => 'The text to render inside the &lt;a&gt; tag',
	'#path' => 'http://www.example.com/',
);
drupal_render($link_array);

After (D8):

$link_array = array(
	'#type' => 'link',
	'#title' => 'The text to render inside the &lt;a&gt; tag',
	'#href' => 'http://www.example.com/',
);
drupal_render($link_array);

Affects:

  • Themers
  • Module developers
Fabianx’s picture

I think it would be good to add (and there is a change record for that as well) that there is a hook_link_alter() called from l() that can be used to change the structure of a link. Before often hook_preprocess_link was used for that.

jwilson3’s picture

Status: Needs review » Needs work

I agree with #23. CNW. Why not go ahead and create the notice, and then we can incrementally update it?

mondrake’s picture

Good start! I think it would also be good to add that you can also specify an '#options' key in the array, that would take any l() or url() options, to specify query parameters, attributes, etc.

You can have a look at theme_pager_* functions have been removed for an example of '#options' and for an example of using hook_link_alter() to manipulate the link data.

star-szr’s picture

@jwilson3 - change notices are automatically tweeted and added to RSS so we try to get some eyes on them before posting. Not always possible but this one is getting plenty of reviews :)

star-szr’s picture

@cconrad - you've done great work on the change notice so far! Are you still working on this or should we have someone else finish this up?

cconrad’s picture

@Cottser I'm pretty busy at the moment, maybe someone else could finish this sooner? Sorry!

thedavidmeister’s picture

Status: Needs work » Needs review





Summary:

  • theme_link has been removed from core in order to improve rendering performance and because it no longer provides any benefits over calling l() directly.
  • The attributes '#text' and '#path' have been changed to '#title' and '#href' respectively in order to provide consistent attribute naming throughout the Render API and Form API.
  • Themers should replace '#theme' => 'link' with '#type' => 'link' in any render arrays, while also changing '#text' to '#title' and '#path' to '#href'.
  • An '#options' attribute can be specified that will be passed to l() (and url() internally within l()) to specify extra options for the link such as query parameters, HTML attributes, etc.
  • See Change notice: Remove theme_link() comment #10 for the reasoning behind this change.
  • Themers should replace any hook_preprocess_link() implementations with a hook_link_alter() implementation. hook_link_alter() was added in https://drupal.org/node/1922454.

Code example:

Before (D7):

$link_array = array(
            '#theme' => 'link',
            '#text' => 'The text to render inside the <a> tag',
            '#path' => 'http://www.example.com/'
          );
          drupal_render($link_array);

After (D8):

$link_array = array(
            '#type' => 'link',
            '#title' => 'The text to render inside the <a> tag',
            '#href' => 'http://www.example.com/',
          );
          drupal_render($link_array);

For additional examples demonstrating usage of both '#options' and hook_link_alter(), please reference https://drupal.org/node/1819788.

Affects:

  • Themers
  • Module developers


star-szr’s picture

Status: Needs review » Reviewed & tested by the community

Looks great, ship it! Thanks everyone.

Edit: Other than coding standards in the code samples ;) - d7 example needs a trailing comma in the array and indenting.

webchick’s picture

Make it so! :) Thanks for writing this!

thedavidmeister’s picture

Status: Reviewed & tested by the community » Fixed
star-szr’s picture

Title: Change notice: Remove theme_link() » Remove theme_link()
Priority: Critical » Normal
star-szr’s picture

Issue tags: -Needs change record

:)

Automatically closed -- issue fixed for 2 weeks with no activity.