API page: https://api.drupal.org/api/drupal/core!modules!system!templates!links.ht...

Array key 'href' has changed to 'url'. This is reflected within the code but not on the API docs.

Question for the Theme System / Twig maintainers

We have a case here where there is a preprocessing function that expects certain input from the render array, and then the Twig template expects the processed output of that as its input.

My question is whether in the Twig template we are supposed to document what comes in after preprocessing? In which case, where are we documenting what goes into the render array? This is kind of a generic question, not just for this issue:
- Module developers passing stuff from their module through a given theme hook -- where do they look for docs and what do they expect?
- Theme developers overriding a theme template -- where do they look for docs and what do they expect?

Members fund testing for the Drupal project. Drupal Association Learn more

Comments

ocastle created an issue. See original summary.

jhodgdon’s picture

Title: Update links.html.twig Docs. » links.html.twig docs are out of date
Issue tags: +Novice

Thanks for the issue! This does need to be fixed. It needs to be fixed in all of the links.html.twig template headers (there are 3). You can get the correct documentation from template_preprocess_links().

leolando.tan’s picture

Assigned: Unassigned » leolando.tan
leolando.tan’s picture

As per @jhodgdon instructed, I have updated the three links.html.twig template headers:

  • core/themes/stable/templates/navigation/links.html.twig
  • core/themes/classy/templates/navigation/links.html.twig
  • core/modules/system/templates/links.html.twig

I also double-checked the final used variable from template_preprocess_links

I also tested to apply the patch using `git apply -v` to see if it works.

Thanks!

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

Perfect, thanks!

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

Unfortunately there might more out of date stuff here...

  1. +++ b/core/modules/system/templates/links.html.twig
    @@ -8,11 +8,11 @@
      *     to l() as its $options parameter.
    
    +++ b/core/themes/classy/templates/navigation/links.html.twig
    @@ -8,11 +8,11 @@
      *     to l() as its $options parameter.
    
    +++ b/core/themes/stable/templates/navigation/links.html.twig
    @@ -8,11 +8,11 @@
      *     to l() as its $options parameter.
    

    l() does not exist anymore... it is passed to the link generator service.

  2. We're also missing documentation for item.text and item.text_attributes... plus this seems to have replaced the if omitted functionality... so maybe title does not exist anymore.
leolando.tan’s picture

Thanks for your additional feedback @alexpott and @jhodgdon! I'll work on the new changes needed.

hussainweb’s picture

I found this from #2639656-3: Fix documentation for links.html.twig and template_preprocess_links. I am uploading the patch from there which is practically the same as the one in #4 with fixes from comments in #6. Further, there is no url variable in links.html.twig (it is actually link). The patch fixes that as well. Lastly, this patch fixes the outdated references in template_preprocess_links as well.

I think point #6.2 is pending. I will look into it in due course.

leolando.tan’s picture

Awesome @hussainweb but did you mean to upload the patch you've created which is fix_documentation_for_links-2639656-2.patch instead?

Sorry, I just wanted to clarify so that I can help review because you've submitted a fix already.

hussainweb’s picture

Oh, did I upload your patch?! Thanks for catching that. I had downloaded it to compare and I guess I uploaded the same file again. Here is the correct patch.

Thank you for the review as well. :)

leolando.tan’s picture

No problem at all @hussainweb! :)

Review process

  • Pulled an updated code base of 8.0.x
  • Applied the latest patch at comment #10
  • Used kint() and XDebug to check the data inside the variables.

Basis of findings

From the template_preprocess_links file I noticed these lines of codes:
$item['attributes'] = new Attribute($li_attributes);
Is for the LI attributes.

$item['text_attributes'] = new Attribute($link['attributes']);
Is for the text or actual link attributes.

Findings

In links.html.twig:

  1. We may need to change “title” into “text” on the links comment section as being used in:
    <span{{ item.text_attributes }}>{{ item.text }}</span>
  2. We may need to add in the “text_attributes” element into the “links” comment section. For the description, we can use the “attributes”’s element description w/c is also seen on the reference code above.
  3. We may need to change the “attributes” element description to match what it’s really used for based on the twig usage w/c is for the LI:
    <li{{ item.attributes.addClass(key|clean_class) }}>
  4. We may need to remove the “link_key” element from the comment since it’s not present when I use kint() to display the available data we can use on the twig file.

I'm also not sure if I need to change the status to needs work. Please correct me if or feel free to change the status to Needs Work.

leolando.tan’s picture

I went ahead and created a patch and an interdiff that contains the fixes based on findings I reported.

I also tested to apply the patch using `git apply -v` to see if it works.

The last submitted patch, 12: interdiff-2620854-10-12.patch, failed testing.

leolando.tan’s picture

FileSize
3.12 KB

Sorry for being so careless. On comment #12 the interdiff I uploaded is using the .patch file type but should be .txt as instructed. I hope this doesn't break anything.

Here is the right interdiff file with the right file type.

Thanks!

hussainweb’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/system/templates/links.html.twig
    @@ -7,12 +7,12 @@
    + *   - attributes: (optional) HTML attributes for the li HTML element.
    

    Is it better to say 'list item element'?

  2. +++ b/core/modules/system/templates/links.html.twig
    @@ -7,12 +7,12 @@
    + *   - text_attributes: (optional) HTML attributes for the anchor, or for the <span>
    

    text_attributes is only used for the span element, not anchor. Also, this line exceeds 80 characters but if you remove the word anchor, I think it would be fine.

Yes, interdiffs should be .txt, but it is okay even if you upload .patch. You will just get a failure which you can ignore. Interdiffs are only for reference to reviewers. It doesn't matter to the system. Thank you for the patches!

leolando.tan’s picture

@hussainweb, thanks for your feedback and the additional clarification and information about creating interdiffs. Will definitely remember that!

I have created a new patch based on the recommendations given. Since for the first item stated that it's probably best to use "list item element", I went ahead and used the same format for the text_attributes key using the format "span element" too. I have also fixed the 80 character length issue.

jhodgdon’s picture

Status: Needs review » Needs work

Thanks for the patches and reviews! I think there are still some problems here though...

  1. +++ b/core/includes/theme.inc
    @@ -575,14 +575,12 @@ function template_preprocess_datetime_wrapper(&$variables) {
    - *     If the 'href' element is supplied, the entire link array is passed to
    - *     l() as its $options parameter.
    

    We lost this important information. Please put it back... it needs to be fixed, not removed (see comment #6).

  2. +++ b/core/includes/theme.inc
    @@ -575,14 +575,12 @@ function template_preprocess_datetime_wrapper(&$variables) {
    + *     route_name + route_parameters or url (path), language and query options
    

    If we're fixing this line... can we also get a comma before the "and" here? Thanks!

  3. +++ b/core/modules/system/templates/links.html.twig
    @@ -7,13 +7,12 @@
    + *   - link: The link URL. If omitted, the 'title' is shown as a plain text
    

    Shouldn't this element be url and not link? Applies to all of the template files.

leolando.tan’s picture

Thanks for your review @jhodgdon!

Things done:

  • Re-applied "url" instead of "link". My bad, I didn't notice the recent patch from the other duplicate ticket changed it to "link".
  • Tried my best to add the right information about comment #6. I also tried to trace the details on how the new function is being used and saw a method called preRenderLink located in core/lib/Drupal/Core/Render/Element/Link.php that gave some details on how the array elements were passed.
  • Added the recommendation you gave on item #2 found in the precious comment.

Thanks!

jhodgdon’s picture

Status: Needs review » Needs work

This is looking pretty good... However, the bit about

+ *     If the 'url' element is supplied, the entire link array is passed to
+ *     Drupal::linkGenerator()->generate() as its parameters.

didn't make any sense to me, since you can't pass a link array as 2 parameters to that function.

So I dug up the code that is actually doing this. So. Here is what actually happens:

a) In template_preprocess_links() [the function being documented in theme.inc in this patch], it sets up individual link elements like this, starting from each individual link array:

     $keys = ['title', 'url'];
     $link_element = array(
        '#type' => 'link',
        '#title' => $link['title'],
        '#options' => array_diff_key($link, array_combine($keys, $keys)),
        '#url' => $link['url'],
        '#ajax' => $link['ajax'],
      );

So basically, everything except 'title' and 'url' is being put into the #options in the type 'link' render arrays.

b) In the Link element class (which processes '#type' = 'link' for render arrays), method preRenderLink (see
https://api.drupal.org/api/drupal/core!lib!Drupal!Core!Render!Element!Li...
) does this to actually generate the link:

  if (!empty($element['#url']) && $element['#url'] instanceof CoreUrl) {
    $options = NestedArray::mergeDeep($element['#url']->getOptions(), $element['#options']);
    $link_generator = \Drupal::service('link_generator');
    $generated_link = $link_generator->generate($element['#title'], $element['#url']->setOptions($options));
    $element['#markup'] = $generated_link->getGeneratedLink();

So... What this documentation needs to say is that if the 'url' element is a URL object, any elements from the original link array, except 'title' and 'url', are being added to the options already present in the URL object, and then the resulting URL object is passed into \Drupal::linkGenerator()->generate().

And incidentally, whenever you mention a class in documentation, it needs to include the namespace starting with \ so don't forget that on the Drupal class (which is in the top-level namespace).

This also needs to be updated in the docs for the various link template files.

leolando.tan’s picture

Oh! I see will remember the points you said! I have also reached the Link class and the preRenderLink method but I haven't translated what it really does properly. I hope this update is acceptable.

Thanks!

jhodgdon’s picture

Status: Needs review » Needs work

Much better, thanks! I think it can be improved slightly...

  1. +++ b/core/includes/theme.inc
    @@ -575,14 +575,16 @@ function template_preprocess_datetime_wrapper(&$variables) {
      *     - url: (optional) The url object to link to. If omitted, no a tag is
    

    Can we fix this?

    url object => URL object
    (in the text, we want to say URL not url -- not at the beginning of the line however!).

  2. +++ b/core/includes/theme.inc
    @@ -575,14 +575,16 @@ function template_preprocess_datetime_wrapper(&$variables) {
    + *     If the 'url' element is supplied, any elements from the original link
    + *     array, except 'title' and 'url', are being added to the options already
    + *     present in the URL object, and then the resulting URL object is passed
    + *     into \Drupal::linkGenerator()->generate().
    

    Good, at least this is accurate! I think it needs a bit of English editing though.

    How about:

    If the 'url' element is supplied, it is used to generate a link using \Drupal::linkGenerator()->generate(), after adding elements from the link array (except 'title' and 'url') to existing options on the URL object.

    Also, can this text be moved into the 'url' section of the documentation above?

  3. +++ b/core/modules/system/templates/links.html.twig
    @@ -7,13 +7,15 @@
    + *     text item in the links list. If this element is supplied, any elements
    + *     from the original link array, except 'title' and 'url', are being added
    + *     to the options already present in the URL object, and then the resulting
    + *     URL object is passed into \Drupal::linkGenerator()->generate().
    

    Make similar change to note above in this file as well as the other templates please.

leolando.tan’s picture

Thank you for your feedback and recommendation @jhodgdon. Here I have applied the latest recommendations.

Before re-applying the patch at #20 for the new changes I updated my local by doing `git pull origin 8.0.x`.

Thanks!

jhodgdon’s picture

OK, this all looks right now.

My only question is... we have a bit of a strange mix in the template file docs of "document what you need to put in your render array to use this theme hook" and "document what gets here after the preprocessing function".

For instance, in the preprocess function it has "attributes" and says they are used for either the a link, or if there is no URL, the span. But in the template file, it has separate keys for link/span attributes (presumably these are separated out by the preprocessing function? I assume so).

But then for the URL element, in the preprocess function, it says the thing about generating the URL, but really by the time you get to the template, I think that has already been done, because the Twig template doesn't do stuff like that.

So. I think we need to figure out what we're doing in the template docs. They're kind of a weird mix of being for the programmer who is setting up a render array, and the themer who wants to make a custom theme, so I'm not sure what the right thing to do is. Thoughts?

leolando.tan’s picture

Thanks for your review and feedback @jhodgdon! I see what you mean, also I saw that the preprocess function definitely separates the link's render array and text.

The link is by the ff. code snippets:

$link_element = array(
  '#type' => 'link',
  '#title' => $link['title'],
  '#options' => array_diff_key($link, array_combine($keys, $keys)),
  '#url' => $link['url'],
  '#ajax' => $link['ajax'],
);
$item['link'] = $link_element;

The text is by the ff. code snippets:

// Handle title-only text items.
$item['text'] = $link['title'];
if (isset($link['attributes'])) {
  $item['text_attributes'] = new Attribute($link['attributes']);
}

I'm not very familiar with the steps on how and when render arrays get "converted" but I just know that they are somewhat done in render() and/or drupal_render(). So I tried to research on how D8 renders things in the theme layer and came across Twig best practices - preprocess functions and templates that states, "Twig renders everything automatically so there is no need to call drupal_render() or theme() within a preprocess function". So does this mean TWIG is rendering the arrays?

Maybe we can:

  1. Remove the explanation of \Drupal::linkGenerator()->generate() from the template files.
  2. Just explain that 'url' element holds the link while 'text' can also be used with 'text_attributes' as an alternative if no link is available.

What are your thoughts on this?

Thanks!

jhodgdon’s picture

Component: documentation » theme system
Issue summary: View changes
Issue tags: +Twig

I think we need to see what we're doing for other Twig template documentation, where there are preprocess functions.

My *guess* is that we need to document what goes into the render array somewhere, but probably not in the Twig template.

Tagging with "Twig" and moving temporarily into the Theme component to hopefully catch the attention of the theme system maintainers. Adding this question to the issue summary as well.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

cilefen’s picture

Issue tags: -Novice

I removed the Novice tag because the direction on this issue is unknown right now.

jhodgdon’s picture

The standards question in this issue is being discussed now on a Coding Standards issue on #2731445: Decide how to document theme variables. Some theme system maintainer comment there would be helpful!

Jeff Burnz’s picture

Re the template docs:

If the 'url' element is supplied, it is used to generate a link using \Drupal::linkGenerator()->generate()

Ok. But what about item.link? No mention? Instead we appear to be fluffing around discussing it's internals? Themers just want to know what to print, and what it might do. If they want to know it's internals we @see template_preprocess_links()

after adding elements from the link array (except 'title' and 'url') to existing options on the URL object.

I've read that 20 times and looking at the code right now (template_preprocess_links()) and I still have no idea what you are talking about. Seems redundant in a template, just document item.link, no need to delve into it's internals - themers just want to know what they can print or use as a condition etc.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.