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.

CommentFileSizeAuthor
#27 Temafejl med php 7.PNG9.71 KBUv516
#6 1187032_theme_link.patch775 bytesdroplet
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jhodgdon’s picture

Title: Documentation problem with theme_link » theme_link generates warning if $variables['options']['html'] is missing
Version: 7.x-dev » 8.x-dev
Component: documentation » theme system
Issue tags: +Needs backport to D7

Good 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.

droplet’s picture

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.

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.

jhodgdon’s picture

Title: theme_link generates warning if $variables['options']['html'] is missing » theme_link needs to be clearer about saying not to call it directly
Component: theme system » documentation
Issue tags: +Novice

OK, 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...

chriscohen’s picture

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

$link = array(
  '#theme' => 'link',
  '#path' => 'node',
  '#text' => t('link text'),
);
print render($link);
jhodgdon’s picture

Component: documentation » theme system

Ah, that is a good point... back to theme system for consideration of whether the html option should be given a default again.

droplet’s picture

Status: Active » Needs review
FileSize
775 bytes

hmm good point.

jhodgdon’s picture

That looks reasonable to me. Maybe we should have a test to verify that it fixes the identified bug?

droplet’s picture

a test ?? do we have test for other theme function ?? we can start a new issue for them.

jhodgdon’s picture

OK then, someone needs to test it manually to make sure it fixes the identified bug.

effulgentsia’s picture

Re #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.

Niklas Fiekas’s picture

Re #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().

Maybe we should add that to the documentation, too.

Having

$link = array(
  '#theme' => 'link',
  '#path' => 'node',
  '#text' => t('link text'),
);

would still show notices, even with #6, because options isn't given.

effulgentsia’s picture

Component: theme system » documentation

Moving 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:

$options += array(
    'attributes' => array(),
    'html' => FALSE,
  );

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.

effulgentsia’s picture

Status: Needs review » Needs work
effulgentsia’s picture

For 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.

xenophyle’s picture

I 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

array(
    '#type' => 'link',
    '#title' => t('Some link text'),
    '#href' => 'some-url',
)

instead of

array(
    '#theme' => 'link',
    '#text' => t('Some link text'),
    '#path' => 'some-url',
)
jhodgdon’s picture

That seems like a separate issue?

stevetweeddale’s picture

@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().

jaredsmith’s picture

Status: Needs work » Reviewed & tested by the community

This looks good to me. Read through the changes (they make sense to me), and made sure the patch applies cleanly.

jaredsmith’s picture

Status: Reviewed & tested by the community » Needs work

Oops, posted that comment on the wrong issue. Sorry for the noise.

thedavidmeister’s picture

Status: Needs work » Closed (fixed)
naught101’s picture

Is there an actual answer/other issue for the question in #15?

thedavidmeister’s picture

I'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().

jhodgdon’s picture

Yeah, #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.

thedavidmeister’s picture

#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.

jcisio’s picture

Version: 8.x-dev » 7.x-dev
Issue summary: View changes
Status: Closed (fixed) » Active
Issue tags: -Needs backport to D7

#20 and #22 link to D8 issues.

jhodgdon’s picture

I 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.

Uv516’s picture

Version: 7.x-dev » 7.57
Component: documentation » theme system
FileSize
9.71 KB

I have changed to PHP 7.0 and got the error shown in attached file.

The patch from #6 is used and extended to this:

- return '<a href="' . check_plain(url($variables['path'], $variables['options'])) . '"' . drupal_attributes($variables['options']['attributes']) . '>' . ($variables['options']['html'] ? $variables['text'] : check_plain($variables['text'])) . '</a>';

+  return '<a href="' . check_plain(url($variables['path'], $variables['options'])) . '"' . (!empty($variables['options']['attributes']) ? drupal_attributes($variables['options']['attributes']) : '') . '>' . (!empty($variables['options']['html']) ? $variables['text'] : check_plain($variables['text'])) . '</a>';

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.

Version: 7.57 » 7.x-dev

Core issues are now filed against the dev versions where changes will be made. Document the specific release you are using in your issue comment. More information about choosing a version.