Problem/Motivation
API page: https://api.drupal.org/api/drupal/core!modules!system!templates!links.ht...
- The 'link' element is not documented.
- The 'url' element is documented, but is not available in the template.
- It is unclear for developers what the input is for the theme function.
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?
Comment | File | Size | Author |
---|---|---|---|
#43 | 2620854-43.patch | 8.43 KB | idebr |
#43 | interdiff-37-43.txt | 1.34 KB | idebr |
Comments
Comment #2
jhodgdonThanks 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().
Comment #3
leolandotan CreditAttribution: leolandotan as a volunteer and at Promet Source commentedComment #4
leolandotan CreditAttribution: leolandotan as a volunteer and at Promet Source commentedAs per @jhodgdon instructed, I have updated the three links.html.twig template headers:
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!
Comment #5
jhodgdonPerfect, thanks!
Comment #6
alexpottUnfortunately there might more out of date stuff here...
l() does not exist anymore... it is passed to the link generator service.
title
does not exist anymore.Comment #7
leolandotan CreditAttribution: leolandotan as a volunteer and at Promet Source commentedThanks for your additional feedback @alexpott and @jhodgdon! I'll work on the new changes needed.
Comment #8
hussainwebI 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 intemplate_preprocess_links
as well.I think point #6.2 is pending. I will look into it in due course.
Comment #9
leolandotan CreditAttribution: leolandotan as a volunteer and at Promet Source commentedAwesome @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.
Comment #10
hussainwebOh, 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. :)
Comment #11
leolandotan CreditAttribution: leolandotan as a volunteer and at Promet Source commentedNo problem at all @hussainweb! :)
Review process
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:
<span{{ item.text_attributes }}>{{ item.text }}</span>
<li{{ item.attributes.addClass(key|clean_class) }}>
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.
Comment #12
leolandotan CreditAttribution: leolandotan as a volunteer and at Promet Source commentedI 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.
Comment #14
leolandotan CreditAttribution: leolandotan as a volunteer and at Promet Source commentedSorry 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!
Comment #15
hussainwebIs it better to say 'list item element'?
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!
Comment #16
leolandotan CreditAttribution: leolandotan as a volunteer and at Promet Source commented@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.
Comment #17
jhodgdonThanks for the patches and reviews! I think there are still some problems here though...
We lost this important information. Please put it back... it needs to be fixed, not removed (see comment #6).
If we're fixing this line... can we also get a comma before the "and" here? Thanks!
Shouldn't this element be url and not link? Applies to all of the template files.
Comment #18
leolandotan CreditAttribution: leolandotan as a volunteer and at Promet Source commentedThanks for your review @jhodgdon!
Things done:
Thanks!
Comment #19
jhodgdonThis is looking pretty good... However, the bit about
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:
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:
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.
Comment #20
leolandotan CreditAttribution: leolandotan as a volunteer and at Promet Source commentedOh! 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!
Comment #21
jhodgdonMuch better, thanks! I think it can be improved slightly...
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!).
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?
Make similar change to note above in this file as well as the other templates please.
Comment #22
leolandotan CreditAttribution: leolandotan as a volunteer and at Promet Source commentedThank 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!
Comment #23
jhodgdonOK, 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?
Comment #24
leolandotan CreditAttribution: leolandotan as a volunteer and at Promet Source commentedThanks 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:
The text is by the ff. code snippets:
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:
What are your thoughts on this?
Thanks!
Comment #25
jhodgdonI 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.
Comment #27
cilefen CreditAttribution: cilefen commentedI removed the Novice tag because the direction on this issue is unknown right now.
Comment #28
jhodgdonThe 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!
Comment #29
Jeff Burnz CreditAttribution: Jeff Burnz commentedRe the template docs:
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()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.
Comment #34
markhalliwellComment #36
anya_m CreditAttribution: anya_m as a volunteer and at Skilld commentedRerolled
Comment #37
idebr CreditAttribution: idebr at ezCompany commentedAttached patch implements the following changes:
template_preprocess_links()
for instead.template_preprocess_links()
documentation with a reference to\Drupal\Core\Url::fromUri()
where the Url options are documented. This happens in multiple places in\Drupal\Core\Url
.The underlying assumptions are:
hook_preprocess_HOOK()
or by scanningtemplate_preprocess_links()
.Comment #38
idebr CreditAttribution: idebr at iO commentedExpanded the 'Problem/Motivation' in the issue summary.
Comment #40
afeijoI approve the patch, new documentation looks good.
Comment #42
larowlanthanks folks, can we also do the more specific versions too - links--node.html.twig and links--media-library-menu.html.twig both have the old, wrong docs in them.
Comment #43
idebr CreditAttribution: idebr at iO commented#42 Updated links--media-library-menu.html.twig documentation. links--node.html.twig was already included in the latest patch in #37.
Comment #44
LendudeFeedback from @larowlan has been addressed, back to RTBC
Comment #46
idebr CreditAttribution: idebr at iO commentedRandom fail in Drupal\Tests\ckeditor\FunctionalJavascript\CKEditorIntegrationTest::testDrupalImageDialog
Comment #47
alexpottCrediting @jhodgdon, @alexpott, @Jeff Burnz, @larowlan for issue review.
Didn't backport to 8.8.x because we're in a freeze for a bugfix release.
Committed and pushed 4813116ddf to 9.0.x and a0761b218a to 8.9.x. Thanks!
Comment #51
alexpottBackported to 8.8.x as a docs only fix.