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

Files: 
CommentFileSizeAuthor
#16 1985470-16.patch22.67 KBthedavidmeister
PASSED: [[SimpleTest]]: [MySQL] 57,866 pass(es).
[ View ]
#13 1985470-13.patch22.27 KBthedavidmeister
PASSED: [[SimpleTest]]: [MySQL] 57,566 pass(es).
[ View ]
#11 1985470-11.patch14.09 KBthedavidmeister
FAILED: [[SimpleTest]]: [MySQL] 57,443 pass(es), 17 fail(s), and 0 exception(s).
[ View ]

Comments

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

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

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

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.

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

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.

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

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 )

Priority:Normal» Minor
Issue tags:+theme system cleanup

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

Title:Consolidate arguments in #type => link and #theme => linkRemove 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?

Status:Active» Needs review
StatusFileSize
new14.09 KB
FAILED: [[SimpleTest]]: [MySQL] 57,443 pass(es), 17 fail(s), and 0 exception(s).
[ View ]

this will fail but i want to see where.

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new22.27 KB
PASSED: [[SimpleTest]]: [MySQL] 57,566 pass(es).
[ View ]

try this.

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

Status:Reviewed & tested by the community» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new22.67 KB
PASSED: [[SimpleTest]]: [MySQL] 57,866 pass(es).
[ View ]

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

Status:Needs review» Reviewed & tested by the community

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.

Issue tags:+Needs change record

update tags (normalize to "Needs change notification")

Assigned:Unassigned» cconrad

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

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

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.

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?

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.

@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 :)

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

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

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

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.

Make it so! :) Thanks for writing this!

Status:Reviewed & tested by the community» Fixed

Title:Change notice: Remove theme_link()Remove theme_link()
Priority:Critical» Normal

Issue tags:-Needs change record

:)

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