Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 UTC on 18 March 2024, to get $100 off your ticket.
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.
Related Issues
#1922454: Gut l(), fix theme('link'), have a working framework for consistently generated links inside and outside of Twig Assigned to: catch
Comment | File | Size | Author |
---|---|---|---|
#16 | 1985470-16.patch | 22.67 KB | thedavidmeister |
#13 | 1985470-13.patch | 22.27 KB | thedavidmeister |
#11 | 1985470-11.patch | 14.09 KB | thedavidmeister |
Comments
Comment #1
tim.plunkettFrom D7 to D8, theme_image went from 'path' to 'uri'.
I'd like to see 'path' become 'href' here.
Comment #2
thedavidmeister CreditAttribution: thedavidmeister commentedI'd like to see 'text' be 'content' and 'path' be 'uri'.
Comment #3
tim.plunkettContent 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.
Comment #4
thedavidmeister CreditAttribution: thedavidmeister commentedIt accepts Drupal "internal paths" and absolute paths - whatever url() accepts. but 'text' is wrong as it is really 'inner HTML'.
Comment #5
tim.plunkettYes, 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.
Comment #6
thedavidmeister CreditAttribution: thedavidmeister commented'value' and 'href' sound fine to me.
Comment #7
Fabianx CreditAttribution: Fabianx commentedhref 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 )
Comment #8
thedavidmeister CreditAttribution: thedavidmeister commentedComment #9
thedavidmeister CreditAttribution: thedavidmeister commented#7 - you're right, the variable names should be pre-process not post process.
Comment #10
thedavidmeister CreditAttribution: thedavidmeister commentedWhat 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?
Comment #11
thedavidmeister CreditAttribution: thedavidmeister commentedthis will fail but i want to see where.
Comment #13
thedavidmeister CreditAttribution: thedavidmeister commentedtry this.
Comment #14
jenlamptonApplied 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. :)
Comment #15
catchPatch doesn't appear to remove this from drupal_common_theme()?
Comment #16
thedavidmeister CreditAttribution: thedavidmeister commented@catch, not sure how I missed that, thanks!
Comment #17
catchComment #18
catchCommitted/pushed to 8.x, thanks!
Will need a short change notice.
Comment #19
scor CreditAttribution: scor commentedupdate tags (normalize to "Needs change notification")
Comment #20
catch#2031219: Remove theme_link from system.performance.yml.
Comment #21
cconrad CreditAttribution: cconrad commentedTrying to write change notice (IRC office hours, nick cconrad, mentor heddn)
Comment #22
cconrad CreditAttribution: cconrad commentedSuggested change notice - needs thorough review:
Summary:
Code example:
Before (D7):
After (D8):
Affects:
Comment #23
Fabianx CreditAttribution: Fabianx commentedI 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.
Comment #24
jwilson3I agree with #23. CNW. Why not go ahead and create the notice, and then we can incrementally update it?
Comment #25
mondrakeGood 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.
Comment #26
star-szr@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 :)
Comment #27
star-szr@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?
Comment #28
cconrad CreditAttribution: cconrad commented@Cottser I'm pretty busy at the moment, maybe someone else could finish this sooner? Sorry!
Comment #29
thedavidmeister CreditAttribution: thedavidmeister commentedSummary:
Code example:
Before (D7):
After (D8):
For additional examples demonstrating usage of both '#options' and hook_link_alter(), please reference https://drupal.org/node/1819788.
Affects:
Comment #30
star-szrLooks 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.
Comment #31
webchickMake it so! :) Thanks for writing this!
Comment #32
thedavidmeister CreditAttribution: thedavidmeister commentedhttps://drupal.org/node/2061877
Comment #33
star-szrComment #34
star-szr:)