Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
API page: http://api.drupal.org/api/drupal/includes--theme.inc/function/theme_link/7
Describe the problem you have found:
theme_link REQUIRES the html option in the options array, but this is not mentioned in the documentation. The documentation should explicitly mention this requirement, and should, in my opinion, actually state an example of the error that will be generated if this requirement is not met, so people searching for the error text can easily find the cause.
Comment | File | Size | Author |
---|---|---|---|
#27 | Temafejl med php 7.PNG | 9.71 KB | Uv516 |
#6 | 1187032_theme_link.patch | 775 bytes | droplet |
Comments
Comment #1
jhodgdonGood point. I wonder if this was intentional though? Looking at the code, I bet someone tested it with less strict warnings in place, because it's not an error but a warning that would be generated if it's missing, I think?
Moving to theme system for clarification about whether this is a code bug or a documentation bug. If it's a documentation bug (i.e., we need to document that when calling theme('link') you need to make sure that options['html'] is set to either TRUE or FALSE), please move back to the documentation component. If it's a code bug (i.e., theme_link() should be using isset to avoid a code warning), then please consider for a d7/d8 code fix.
Comment #2
droplet CreditAttribution: droplet commentedAll Drupal code that outputs a link should call the l() function. That function performs some initial preprocessing, and then, if necessary, calls theme('link') for rendering the anchor tag.
what I know is druapl has a few of function working in this way,
You should call foo() instead of call goo() directly. in this case you should call l().
a Doc issue if need to be more clearly.
Comment #3
jhodgdonOK, I agree with this assessment. The function already says not to call it directly, but it could be stated more clearly, like "Don't call this function directly or with theme('link'). Instead, ... [text that is already there]".
Probably a good project for a novice doc contributor...
Comment #4
chriscohen CreditAttribution: chriscohen commentedWhat about render arrays? Is the following not valid in Drupal 7? If it's not valid, it seems that link generation would be skipping the render system by using l() directly. If that's the case, how would you allow other modules or themes to manipulate your links (adding further classes, ids, etc) before they are rendered?
Comment #5
jhodgdonAh, that is a good point... back to theme system for consideration of whether the html option should be given a default again.
Comment #6
droplet CreditAttribution: droplet commentedhmm good point.
Comment #7
jhodgdonThat looks reasonable to me. Maybe we should have a test to verify that it fixes the identified bug?
Comment #8
droplet CreditAttribution: droplet commenteda test ?? do we have test for other theme function ?? we can start a new issue for them.
Comment #9
jhodgdonOK then, someone needs to test it manually to make sure it fixes the identified bug.
Comment #10
effulgentsia CreditAttribution: effulgentsia commentedRe #4: You shouldn't make a render array with '#theme' => 'link'. You should use '#type' => 'link' instead, which causes drupal_pre_render_link() to be called which calls l().
I can't think of any good reason for theme_link() or theme('link') to ever be called directly, rather than via l().
I have no objections to #6 though.
Comment #11
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedMaybe we should add that to the documentation, too.
Having
would still show notices, even with #6, because
options
isn't given.Comment #12
effulgentsia CreditAttribution: effulgentsia commentedMoving this back to the documentation queue, since no one has argued with #10.
I agree with #11. Perhaps sample code in the PHPDoc of theme_link(), similar to how we have for t()? I think the sample code should show calling l() instead of theme('link') and for render arrays, using #type=>'link' instead of #theme=>'link'.
Why are we discouraging calling theme('link') or setting #theme=>'link'? Because that bypasses l(), and l() does some useful stuff, like figuring out whether to add an 'active' class, and cleaning up the 'title' attribute.
However, I looked at the user posted comments in http://api.drupal.org/api/drupal/includes--theme.inc/function/theme_link/7, and it seems like people are still gonna use '#theme' => 'link', even if we tell them not to. And that's understandable, since I think every other theme hook can be used as the value of a '#theme' without errors. So, if we want to allow this usage to work without errors, we can also add a:
as the first line inside theme_link(), just as it is in l(). If we do this, it would be nice to have a comment above this explaining that it's just for the edge case of someone bypassing l(), but that as per our PHPDoc, we don't endorse that edge case.
Comment #13
effulgentsia CreditAttribution: effulgentsia commentedComment #14
effulgentsia CreditAttribution: effulgentsia commentedFor anyone wanting background on why theme_link() is unlike most of our other theme functions, which can be called directly via theme()/#theme, see #318636: Make l() themable. Basically, in prior versions of Drupal, we just had l(), and it wasn't themeable. Since it outputs HTML, some people (including me) thought it should call a theme function, but there were performance concerns about that, so we made some compromises.
Comment #15
xenophyle CreditAttribution: xenophyle commentedI have always wondered how you can tell if you should use #type or #theme in render arrays. Is there any place that explains that?
It is also worth noting that when using #type, the other keys change too. I.e., use
instead of
Comment #16
jhodgdonThat seems like a separate issue?
Comment #17
stevetweeddale CreditAttribution: stevetweeddale commented@xenophyle the link type passes your element through to drupal_pre_render_link() which is effectively what you're looking for.
Interestingly, it appears there's some magic in there around the #ajax property not included in the main documentation of that function. But otherwise, your render array #keys line up pretty much with the arguments to l().
Comment #18
jaredsmith CreditAttribution: jaredsmith commentedThis looks good to me. Read through the changes (they make sense to me), and made sure the patch applies cleanly.
Comment #19
jaredsmith CreditAttribution: jaredsmith commentedOops, posted that comment on the wrong issue. Sorry for the noise.
Comment #20
thedavidmeister CreditAttribution: thedavidmeister commentedtheme_link() is ok now #1922454: Gut l(), fix theme('link'), have a working framework for consistently generated links inside and outside of Twig, you can use it if you want.
Comment #21
naught101 CreditAttribution: naught101 commentedIs there an actual answer/other issue for the question in #15?
Comment #22
thedavidmeister CreditAttribution: thedavidmeister commentedI've got a list of things that will help answer that question #2015147: [meta] Improve the DX of drupal_render(), renderable arrays and the Render API in general.
There is no simple answer at the moment, other than #type = merge in defaults and #theme = use this theme function. Except that #type can set a #pre_render that can create markup directly and #type can set #theme arbitrarily, so it's not a good explanation at all. See #2005970: In renderable arrays, #type should provide a #theme suggestion, similar to how drupal_prepare_form() works
One of the linked issues is #1985470: Remove theme_link().
Comment #23
jhodgdonYeah, #type means "merge in all the defaults from hook_element_info()" and #theme means "override the default theme for this type of element to use this theme hook instead". Do we need to reopen this issue? It is marked closed/fixed.
Comment #24
thedavidmeister CreditAttribution: thedavidmeister commented#23 - I don't think this needs to be re-opened. This issue was originally opened because l() called theme('link') internally after doing some weird internal checks. This issue was opened because that led to theme('link') not being usable for various reasons including security.
This has been fixed as per the issue linked in #20 - theme('link') is now safe to use, although core makes no use of it, it's slower than calling l() and I wouldn't be surprised if it is removed completely at some later date.
The discussion in #15 and #21 is covered by #22 and #23.
Comment #25
jcisio CreditAttribution: jcisio commented#20 and #22 link to D8 issues.
Comment #26
jhodgdonI do not think we intend to fix this for D7 -- we don't normally put code changes into D7 unless they are essential. And the theme_link() documentation for D7 already says:
All Drupal code that outputs a link should call the l() function. That function performs some initial preprocessing, and then, if necessary, calls theme('link') for rendering the anchor tag.
I think that is pretty clear. It says you need to call l().
We could patch this to say that putting #type = 'link' into a render array is OK.
Comment #27
Uv516 CreditAttribution: Uv516 commentedI have changed to PHP 7.0 and got the error shown in attached file.
The patch from #6 is used and extended to this:
I experienced that the $variables['options']['attributes'] was not set when I changed to PHP 7.
1) I wonder if #6 not is in newer versions of Drupal
2) I wonder if my error was comming from another place, but my changes seem to help.