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.
Issue #1987418 by joelpittet, drupalninja99, rakhimandhania, Manuel Garcia, scor, shanethehat, Cottser, LewisNyman, minneapolisdan, LinL, ezeedub: user.module - Convert theme_ functions to Twig.
(as of #92)
Task
Convert theme_ functions to Twig templates.
Remaining
theme_ function -> Twig conversions (and preprocess function additions or changes) moved over from the existing conversion issuePatch reviewProfiling(done in #72)
Theme function name/template path | Conversion status |
---|---|
Was converted in #type table issue: #1938938: Convert theme_user_admin_permissions() to table #type | |
Was converted in #type table issue: #1938938: Convert theme_user_admin_permissions() to table #type | |
theme_user_permission_description | Converted inline, was overkill. Moved from #1898468: user.module - Convert PHPTemplate templates to Twig |
theme_user_signature | Removed here, not used currently. Also won't be needed once #1548204: Remove user signature and move it to contrib is in. Moved from #1898468: user.module - Convert PHPTemplate templates to Twig |
theme_username | Converted - Moved from #1898468: user.module - Convert PHPTemplate templates to Twig |
Testing steps
Username
- Create an article node and add a comment to it. The username on the comment should be output by username.html.twig.
- View the same node page as a logged out user, the username should still be output by username.html.twig but should not be linked.
User Permission Description.
Visit /admin/people/permissions
Related Issues
#1898468: user.module - Convert PHPTemplate templates to Twig
#1757550: [Meta] Convert core theme functions to Twig templates
#1987510: [meta] Convert all core *.tpl.php templates to Twig as singular patch
Comment | File | Size | Author |
---|---|---|---|
#99 | 1987418-twig-user-99.patch | 14.03 KB | LinL |
#96 | interdiff.txt | 1.17 KB | joelpittet |
#96 | 1987418-twig-user-96.patch | 14.1 KB | joelpittet |
#94 | interdiff.txt | 1.12 KB | joelpittet |
#94 | 1987418-twig-user-94.patch | 14.17 KB | joelpittet |
Comments
Comment #0.0
star-szrUpdated issue summary.
Comment #1
c4rl CreditAttribution: c4rl commentedPer #1757550-44: [Meta] Convert core theme functions to Twig templates, retitling to indicate this issue applies to theme_ functions rather than templates. See #1898468: user.module - Convert PHPTemplate templates to Twig for template conversion.
Comment #1.0
c4rl CreditAttribution: c4rl commentedUpdated issue summary.
Comment #2
shanethehat CreditAttribution: shanethehat commentedComment #4
shanethehat CreditAttribution: shanethehat commented#2: twig-user-theme-only-1987418-2.patch queued for re-testing.
Comment #6
shanethehat CreditAttribution: shanethehat commentedComment #7
star-szrThanks @shanethehat! The rdf.module (rdf_preprocess_username()) changes should be moved here as well.
Comment #8
shanethehat CreditAttribution: shanethehat commented@Cottser I don't see any changes to the RDF module in the last patch on #1898468: user.module - Convert PHPTemplate templates to Twig (http://drupal.org/node/1898468#comment-7341984). Please can you be more specific?
This is the markup whilst signed in:
And this is unauthenticated:
Same two again without the patch:
Comment #9
star-szrSee #1898468-41: user.module - Convert PHPTemplate templates to Twig for more information, I'm actually thinking now that this change can be removed entirely now that #1982024: Lazy-load Attribute objects later in the rendering process only if needed is in, going to test that hypothesis. The first hunk in the patch there is rdf.module though :)
Comment #10
star-szrEh, looking at it again I think removing NestedArray::mergeDeep() will be a performance bump anyway, so moving that code here for now.
Comment #11
minneapolisdan CreditAttribution: minneapolisdan commented#10: 1987418-10.patch queued for re-testing.
Comment #13
minneapolisdan CreditAttribution: minneapolisdan commentedgoing to reroll this
Comment #14
minneapolisdan CreditAttribution: minneapolisdan commentedPatch rerolled for testing, hope it works, first time doing this...
Comment #15
ezeedub CreditAttribution: ezeedub commentedI'm gonna profile this.
Comment #16
ezeedub CreditAttribution: ezeedub commentedBase Scenario: 1 node, with 10 comments.
Scenario #1: anon users without "view user profiles" perm (username not a link).
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=519ffb7471c72&...
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=519ffb7471c72&...
Scenario #2: anon users with "view user profiles" perm (username is a link).
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=519ff7ed8fdf9&...
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=519ff7ed8fdf9&...
Comment #18
ezeedub CreditAttribution: ezeedub commented#14: 1987418-14.patch queued for re-testing.
Comment #18.0
ezeedub CreditAttribution: ezeedub commentedRevise summary
Comment #19
ezeedub CreditAttribution: ezeedub commentedSame as #16, on Stark instead of Bartik.
Base Scenario: 1 node, with 10 comments.
Scenario #1: anon users without "view user profiles" perm (username not a link).
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51a0ef63cb19e&...
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51a0ef63cb19e&...
Scenario #2: anon users with "view user profiles" perm (username is a link).
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51a0f1cae1ba2&...
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51a0f1cae1ba2&...
Quite a difference between anon and logged in. This may be skewed by the link theme function as well? There wasn't such a discrepancy with Bartik in #16.
Summary of the wall times:
Bartik anon: .5
Bartik auth: .8
Stark anon: .6
Stark auth: 1.4
Comment #20
ezeedub CreditAttribution: ezeedub commentedTried again on a different machine. This time, 1 node with 25 comments, anon user with perm to view user profiles.
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51a14c32c53db&...
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51a14c32c53db&...
Comment #21
ezeedub CreditAttribution: ezeedub commented1 node, 25 comments, anon user with no perm to see user profiles.
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51a152cadd6cc&...
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51a152cadd6cc&...
Comment #22
ezeedub CreditAttribution: ezeedub commented'nuther machine,
1 node, 25 comments, user has perm to view profiles
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51a152d922ae0&...
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51a152d922ae0&...
user can't view user profiles.
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51a14be6130cb&...
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51a14be6130cb&...
Comment #23
thedavidmeister CreditAttribution: thedavidmeister commentedthis looks like it's too slow still. 3-68ms slowdowns here :/
Comment #23.0
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #24
jenlamptonCan we figure out which conversion is slow, and separate that out into it's own issue so the other conversions can get in? I expect it's username.html.twig that's problematic.
Comment #25
star-szrThis one needs a reroll.
Comment #26
scor CreditAttribution: scor commentedstarted rerolling this patch, only got the rdf part rerolled so far. Looks like there is already some work that has been done to fix user_permission_description.
Comment #27
rakhimandhania CreditAttribution: rakhimandhania commentedRe-rolling the patch after comment-14
Comment #29
rakhimandhania CreditAttribution: rakhimandhania commentedRe-rolling the patch after comment-14
Comment #30
rakhimandhania CreditAttribution: rakhimandhania commentedComment #32
scor CreditAttribution: scor commented@rakhimandhania could you make sure to incorporate the patch I posted in #26 for the RDF module, it should solve the PHP syntax you keep having in rdf.module. Note that the patch #26 only contains changes for rdf.module, so all you have to do is apply your patch #30, run "git co core/modules/rdf" and then apply my patch.
Comment #33
rakhimandhania CreditAttribution: rakhimandhania commentedComment #35
star-szrThis one might be a bit trickier to reroll. Thanks @scor and @rakhimandhania for your work :)
Comment #36
joelpittetRe-rolled. Started with the RDF in #26 and applied the changes manually. There may be some RDF errors as I had some earlier. @scor maybe you could take a second look?
Comment #38
drupalninja99 CreditAttribution: drupalninja99 commentedI feel like the isset condition "isset($variables['link_path'])" needs to remain bc we are getting a lot of undefined link_path warnings.
Comment #39
drupalninja99 CreditAttribution: drupalninja99 commentedHere is the #36 patch rerolled with a couple conflicts fix.
Comment #40
drupalninja99 CreditAttribution: drupalninja99 commentedIt looks like the twig template that uses the variable 'link' has it wrapped in an if condition. So all I did was wrap the "$variables['link']" assignment in a condition where we check the link_path to see if it's initialized to mimic was was there before. I think that will fix the link_path warnings.
Comment #42
joelpittetGood call @drupalninja99 thanks for putting that back in.
Comment #43
joelpittetLooks like something when wrong with the re-roll in #39 and a function called user_admin_account got added back in some how.
Have a manual comparison between #36 ad #39 to see what I mean.
Comment #44
drupalninja99 CreditAttribution: drupalninja99 commentedYa I had a lot of problems rerolling the patch. I will likely have to do some manual fixing.
Comment #45
drupalninja99 CreditAttribution: drupalninja99 commentedOK so I have fixed the issue. The file user.admin.inc serves no purpose as user_admin_account() was removed a while back and now with no theme function it can just be completely removed so I have removed it from the attached patch.
Comment #46
joelpittetNice work, that's a good call:)
Comment #48
scor CreditAttribution: scor commentedThe fails came from the changes introduced by the patch in rdf_preprocess_username(). I'm reverting those here. The reason the attributes are kept in their own variable in rdf_preprocess_username() is that they can end up in a different place depending on whether the author is a link or not. If it's not a link, the author name will be wrapped in a span (that part was working). If it's a link, the attributes should be added to the link. The problem is that this patch make the link be rendered right away in template_preprocess_username(), so the RDF module does not have the chance to insert its attribute before the link rendering. template_preprocess_username() should use a render array instead. I haven't made that change yet, instead, for now and to prove my point, I'm rendering the link a second time in rdf_preprocess_username(), to see if all the tests are passing.
Comment #49
joelpittetHow about this? We just tack on the link to the attributes and dump it on an a tag instead of rendering it through l(). The options can go to url() instead.
I may need to also merge link_options.attributes into attributes... but would prefer if link_options didn't need to be considered for this theme function.
Comment #50
scor CreditAttribution: scor commentedI was thinking about a render array initialized in template_preprocess_username() which rdf_preprocess_username() could alter. This render array would get rendered by Twig. Wouldn't that work? I'm fine with your approach too.
Comment #51
joelpittet@scor thanks for pointing out the problem. I think a renderable array could work as well. Though as you can see it's a bit more work passing around the arrays of attributes between elements and slightly easier to just build the attributes on their own and apply them to their respective HTML element. Also, renderable arrays aren't really drillable (yet). You can get at their properties but they are all as defined.
{{ link['#href'] }}
As long as the implementation is that a username is not always a link, but a possible span tag, then passing the attributes to either span/a tag should work well as is.
Would like a couple more opinions on the matter though, let see what people say, or if there are issues we haven't spotted.
To implement the renderable link it would probably need to be type=>link, I believe.
Comment #51.0
joelpittetrm todo
Comment #52
joelpittet49: 1987418-49-twig-user.patch queued for re-testing.
Comment #53
star-szrComment #54
star-szrComment #55
joelpittetRe-rolled #49
Comment #58
joelpittetForgot to remove this.
Comment #59
joelpittetSeems the likely fail is due to how assertText() works with line breaks:
Here is the results before and after the patch:
Before
<p class="submitted">Submitted by <span>zscu7SWo</span> on Sun, 03/09/2014 - 15:17</p>
After:
Note the extra 'username' class because we aren't resetting the attribute array on the prepreprocess. Maybe we should clobber the attributes on the way in? Thought that class name would be helpful for themers?
For the actual fail is likely the linebreak, so considering xpath to fix that. Sound good?
Comment #61
joelpittetThis works with whitespace modifiers, it was the easier and cleaner markup option of the two in this case but usually I just prefer xpath. Extra span class is still there though... discuss.
Comment #62
star-szrDo we know where that class is coming from? It does seem useful to me.
Edit: and also familiar maybe? What does the D7 markup look like?
Comment #63
joelpittetD7:
In D7's preprocess:
Which is applied to the a tag or the span tag by way of
'<span' . drupal_attributes($variables['attributes_array']) . '>'
So yeah D7 had the class.
In D8 it's coming from the same place in the preprocess, yet it's applied to link_options:
$variables['link_options']['attributes']['class'] = array('username');
Which currently is not being merged except in rdf's preprocess.
Comment #64
jsbalseraComment #65
jsbalseraTested. As a logged user the markup is:
And as an anonymous user:
so it looks good.
Comment #66
pjezek CreditAttribution: pjezek commentedComment #67
pjezek CreditAttribution: pjezek commentedComment #68
pjezek CreditAttribution: pjezek commentedI did the profiling and reviewed the change. From my point of view it is RTBC.
Comment #69
joelpittetHave a look at the function call difference. It's 0 which likely means bbranches was called from the twig branch. The result is likely 1% slower still but do you mind running this profile once more?
Comment #70
Manuel Garcia CreditAttribution: Manuel Garcia commentedPatch on #61 was no longer applying, here is a re-roll
Comment #71
star-szrUpdating conversion table, going to take a look at profiling/reviewing this.
Comment #72
star-szrHere's the profiling for now for theme_username() vs. username.html.twig. I think there may be some savings possible by looking at preprocess and url() vs. l() but here's how things stand:
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=535139ab6e707&...
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=535139ab6e707&...
Baseline w/ render cache:
wall time - 617,401 microsecs
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?extra=8.x..8.x&sour...
Comment #73
Manuel Garcia CreditAttribution: Manuel Garcia commented#70 patch is still applying cleanly, profiling is done... is this is ready for RTBC?
Comment #74
joelpittetThanks for the poke.
This is not possible as it stands. I tried:( Can you please remove this line?
It's still possible and noted in the preprocess. I've got an issue on the go to try to make this possible in twig too. #2239945: Refactor Attribute class into one class The AttributeArray needs to go away for this to happen.
Comment #75
Manuel Garcia CreditAttribution: Manuel Garcia commentedHere is the same patch as #70 with the comments mentioned in #74 removed.
Comment #77
Manuel Garcia CreditAttribution: Manuel Garcia commentedForgot to add the new twig file...
Comment #78
joelpittetThank you @Manuel Garcia, I think this just needs a manual testing as I don't see any above.
Comment #79
LewisNyman CreditAttribution: LewisNyman commented77: 1987418-twig-user-77.patch queued for re-testing.
Comment #80
LewisNyman CreditAttribution: LewisNyman commentedComment #82
joelpittetComment #83
joelpittetComment #84
LewisNyman CreditAttribution: LewisNyman commentedReroll.
Comment #85
joelpittetSweet thanks for the re-roll @LewisNyman. Needs manual testing next.
Comment #86
scor CreditAttribution: scor commentedManual testing looks good.
username logged in
without the patch:
with the patch:
username logged out
without the patch:
with the patch:
permission warning
without the patch:
with the patch:
Comment #87
scor CreditAttribution: scor commentedComment #88
star-szrThis is not quite ready for prime time yet, but it's mostly docs and maybe a logic tweak from what I can see:
I'm not sure why we are removing the docs for the 'warning' key and why we are changing the behaviour to append instead of replace in this issue. That seems out of scope but maybe I'm missing something.
This is not accurate, uid is not received as a parameter, it is set within the preprocess. This line should be removed as far as I can see.
This comment talks about using l() but we are actually using url() now.
Comment #89
joelpittetRegarding #88.1 re: append vs replace.
Regarding the warning, I'm not sure but I added it back.
And did the other two items on the list.
Hope that resolves the questions and docs @Cottser? Let me know?
Comment #90
star-szrI did a manual test and it looks like the behaviour of warning is not changing so that's cool. I think this is either RTBC or very close.
Tagging for reroll.
Comment #91
scor CreditAttribution: scor commentedreroll
Comment #92
star-szrThanks @scor! I think just some more small docs things and this is ready to fly. Updating the suggested commit message because it was very out of date.
This was probably copy + pasted but I don't think "The user account to check access for" makes sense in the context of outputting a username. Even just "The user account (Drupal\…)" would be an improvement I think.
The link_options variable is no longer being set, so this should be removed from the Twig template.
Comment #93
star-szrA couple more details after doing more manual testing and poking around for usernames:
1. We should only add the 'username' class when adding a link. This discrepancy can be seen in @scor's #86 under "username logged out". The span has an extra 'username' class that doesn't need to be there, the class is meant for the
<a>
tag and should only be added when we have a link_path.2. We are optionally doing some things with $variables['link_options'] towards the end of template_preprocess_username(), so I think it might make sense to keep that and actually declare link_options as part of the hook_theme() declaration in user_theme(). If we go that route then my comment from #92.2 would no longer apply.
Comment #94
joelpittet@Cottser, thanks for the review. I've done the first item in #92
Re #93.1 I really think that class needs there for themers to effectively style this element. They can do a.username or span.username to tell the difference but without it, there is nothing in particular that they could style this element. I see the before as a huge WTF as the class shouldn't be just gone because the tag changes. That seems like a d7 or earlier short sight.
Re #93.2 I added the link_options to the hook and left it in the template. Though those variables are almost entirely useless in the template atm, we will likely add a helper function to build urls and links in the template and then that would be helpful...
Comment #95
star-szrOk, the markup change has a good justification then. One idea for another small simplification, otherwise RTBC from my perspective:
Let's initialize link_options as an empty array in hook_theme() and just pass $variables['link_options'] to url().
Comment #96
joelpittetGreat idea!
I got all the modules in d7 downloaded at home and checked for "user_permission_description" theme overrides and in modules there are none. I'm currently downloading all themes and distros to check there as well (too big to fit on my laptop).
There are a few calling it through theme() because they are overriding the form but nothing needing to override that theme function. And I think I got 33 results and 60%+ of those were documentation or a textmate module. Nothing of note.
Comment #97
star-szrNice, thanks for checking that @joelpittet :)
Comment #98
joelpittetI checked all the distros and themes as well and the only one that implemented 'user_permission_description' was https://drupal.org/project/mobile_jquery
And all they did was change $variables to $vars...
So fewf;)
Comment #99
LinL CreditAttribution: LinL commentedQuick reroll as patch no longer applied following change to comments in
system.api.php
in #2216547: Fill in topic/@defgroup for User system overviewComment #100
star-szrReroll looks good, thank you very much @LinL!
Comment #101
alexpottCommitted 1742e5e and pushed to 8.x. Thanks!