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 #1898468 by Cottser, joelpittet, rteijeiro, c4rl, steveoliver, scor, jenlampton, idflood, shanethehat, thedavidmeister: [READY] user.module - Convert PHPTemplate templates to Twig.
(as of #95)
Task
Convert PHPTemplate templates to Twig templates.
Remaining
Theme function name/template path | Conversion status |
---|---|
core/modules/user/templates/user-picture.tpl.php | Removed, not defined in user_theme(), not needed now that #1292470: Convert user pictures to Image Field is in. |
core/modules/user/templates/user.tpl.php | converted |
Testing steps
User profile page
Visit your user profile page at /user when logged in, the page should be output by user.html.twig.
Related
#1987418: user.module - Convert theme_ functions to Twig
#1757550: [Meta] Convert core theme functions to Twig templates
#1938938: Convert theme_user_admin_permissions() to table #type
#1548204: Remove user signature and move it to contrib
#1843788: Remove template_process_username()
Comment | File | Size | Author |
---|---|---|---|
#91 | 1898468-91.patch | 5 KB | star-szr |
#91 | interdiff.txt | 384 bytes | star-szr |
#83 | 1898468-83.patch | 5.01 KB | idflood |
#83 | interdiff.txt | 1.25 KB | idflood |
#77 | 1898468-77.patch | 5.01 KB | idflood |
Comments
Comment #1
c4rl CreditAttribution: c4rl commentedTagging
Comment #2
trrroy CreditAttribution: trrroy commentedComment #3
c4rl CreditAttribution: c4rl commentedAssigning
Comment #4
c4rl CreditAttribution: c4rl commentedComment #6
c4rl CreditAttribution: c4rl commentedRebased.
Comment #8
rteijeiro CreditAttribution: rteijeiro commentedAssigning and tagging.
Comment #9
rteijeiro CreditAttribution: rteijeiro commentedPatch tests seems ok locally but fail in the testbot. I can't know why so I mark as "need review" if someone could review that.
Comment #10
steveoliver CreditAttribution: steveoliver commentedWhile I look into the various exceptions and errors, here are some standards/best practices things I noticed:
DocBlock format needs updated. See drupal.org/sandbox/pixelmord/1750250#preprocess-docblocks
DocBlock.
And so on...
Turn that theme('table') into a render array. See http://drupal.org/node/1920746#render
Docblock.
Render array.
Comment #11
rteijeiro CreditAttribution: rteijeiro commentedThanks steveoliver,
I will continue the issue...
Comment #12
DamienMcKennaUpdating the tags to point to the correct (at least more used) tag.
Comment #13
star-szrThanks for the work on this, everyone! Just wanted to note, the standards for the docblocks in the .html.twig files are different from the preprocess functions.
Theme function standards: http://drupal.org/node/1823416
Preprocess standards: #1913208: [policy] Standardize template preprocess function documentation
Comment #14
joelpittet@Cottser the tables in these two functions will be removed
theme_user_admin_permissions
andtheme_user_admin_roles
here:#1938938: Convert theme_user_admin_permissions() to table #type
So I am thinking we exclude those two twig conversions from this issue as I have seen this in another issue. Thoughts?
Added the Type=>table conversion to the related on this issue.
Here is a doc cleanup attached and seemed to pass some of the tests locally so I thought I would just chuck it up.
Comment #15
joelpittetDeploy testbot!
Comment #17
star-szr@joelpittet - as discussed, yes please let's leave out the theme functions that will be removed via type #table conversions. No sense converting those. Let me know if you want me to look at the failing tests.
@rteijeiro - Unassigning since it's been a few weeks. Feel free to reassign if you'd like to work on this issue again.
Comment #18
joelpittetOk removed the type table conversions as mentioned in #14
theme_user_admin_permissions and theme_user_admin_roles
Also, removed
user-picture.html.twig
because there was no theme definition in core for it (I am guessing it's not needed any more?)
Also deleted
user-profile.html.twig
Because it's a dupe of user.html.twig and no theme definition for it.
Added 'link' variable needed for the username.html.twig file.
I know this won't pass, @Cottser maybe you could help me out with the exceptions bits, not sure where to look on those?
Comment #19
joelpittetlast but not least, testbot!
Comment #21
star-szrRegarding user-picture.html.twig/theme_user_picture(), the deletion makes sense, that was removed in #1292470: Convert user pictures to Image Field. So the Twig file may have just been hanging around the sandbox. Great catch!
I'll have a look at the tests over the next couple days.
Comment #21.0
star-szrAdded type table issue
Comment #22
joelpittetRealized I left in a function that is on the chopping block and started adding to it, so thought I would add that in as well. Usually try to leave these things separate but i think that removal should happen in concert with this issue. Thoughts?
#1843788: Remove template_process_username()
Comment #23
joelpittetComment #25
joelpittetAgain, this won't fix the exceptions, maybe a fail or two. But I really don't know how to go about debugging where the exceptions are coming from.
Comment #27
steveoliver CreditAttribution: steveoliver commentedFirst, this needs a reroll after rebasing on 8.x. Then...
Holy attributes, batman! And array_merge_recursive is a red flag to me -- this may have something to do with the exceptions. Check to see that this is doing what you want. Step through it with a debugger.
Why the interim $variables['attributes']?
Just do
$variables['attributes'] = new Attribute(array('class' => array('profile')));
?Hope that helps.
Comment #28
star-szrTemporarily assigning to look at the tests and #27.
Comment #29
star-szrTests are still broken, but I did some other work while I was not fixing the tests :)
First patch is just a reconstruction of #25 - #22 + the interdiff in #25. Interdiffs are awesome, everyone.
Summary of changes:
Removed these indentation changes from #25, paragraphs shouldn't be indented and for lists the indent level should match per http://drupal.org/node/1354#lists.
The Twig standards doc at http://drupal.org/node/1823416 incorrectly had the extra space on the second line of a list item, fixed now.
A documentation/Twig question I wasn't sure about in user.html.twig:
I changed account.field_example['en'] to account.field_example.en assuming it would work. Am I right?
Comment #30
star-szrErg, forgot the reconstructed patch from #25.
Comment #32
joelpittet#29 ++
Much cleaner docs:) and yeah fine with the preprocess removal and css class move. Really need some way to track down those "invalid render array key" @Fabianx said he may have some debug patch to give more help there.
Comment #33
star-szrMy frantic debugging last night led to rdf_preprocess_username(), most or all of the render keys in the exceptions are coming from there I think.
Comment #34
joelpittet@Cottser, that looks like a good deal of the problem, at least at first glance!
Comment #35
joelpittetThis should get rid of a majority of the exceptions and at least one fail:) I will await the results to plug further into this.
Comment #36.0
(not verified) CreditAttribution: commentedUpdated issue summary.
Comment #37
joelpittetNot sure how to fix those RDF errors. From debugging the attribute arrays are nesting to deep.
Anybody is welcome to pick this up.
Comment #38
star-szrTagging.
Comment #39
star-szrWill see if I can make sense of the failures, the most prominent issue I see so far is that there's a strange
*storage
attribute being added to the user link.Output from Drupal\rdf\Tests\UserAttributesTest test result:
<a href="/user/3" rel="author" title="View user profile." *storage="username /user/3 sioc:UserAccount foaf:name ">ragUSDWU</a>
I think this documentation needs to be tweaked (two occurrences in the patch):
We're not checking access here as far as I know.
Comment #40
star-szrI haven't had a chance to look deeper into this, hopefully the information in #39 will be a good starting point for debugging.
Comment #41
scor CreditAttribution: scor commentedThe problem actually exists if you disable the RDF module (possibly why Drupal\user\Tests\UserTranslationUITest and ExpandDrupal\views\Tests\Wizard\BasicTest failed as well). There is something odd with the Twig integration of the username. As pointed out in #39, the a link element generated contains the following attribute:
<a href="/user/3" rel="author" title="View user profile." *storage="username /user/3 sioc:UserAccount foaf:name ">ragUSDWU</a>
What happened is that all the attributes values were merged together into the *storage attribute (the class="username" and all the RDFa attribute values). In fact, you can disable the RDF module and reproduce the same bug, where you will see
<a href="/user/3" rel="author" title="View user profile." *storage="username">ragUSDWU</a>
The problem comes from this line:
$variables['attributes']
is a Twig Attribute object and the (array) cast doesn't work and produces the '*storage' key.I've moved the Twig Attribute to the process function so that the attributes key remains an array during preprocess, and only gets converted to a Twig object in process. I'm not sure this is the right way, but it works locally and the tests are passing.
Comment #42
scor CreditAttribution: scor commentedforgot the interdiff for #41.
Comment #43
matthewmack CreditAttribution: matthewmack commentedLooks good to me.
Comment #44
star-szrThanks @matthewmack - if you came here from our TODOs we just updated the instructions, many of the patches in 'needs review' still need a thorough code and documentation review before RTBC :)
Can you confirm that the markup matched up in your testing?
Comment #45
matthewmack CreditAttribution: matthewmack commented@Cottser, it did match up. The only differences I saw were the occasional white space.... I'm going to freshen up on the instructions that have been changed.
Comment #46
scor CreditAttribution: scor commentedBefore setting #41 to RTBC, I would really like someone more familiar than I am with Twig to approve the changes I made (see interdiff in #42). I'm not sure if it's kosher to keep the attributes as an array in the preprocess functions and switch them to a Twig Attribute object in the template_process_username() function.
Comment #47
thedavidmeister CreditAttribution: thedavidmeister commentedhmm, using #theme => 'link'? That's probably a security hole unless #1922454: Gut l(), fix theme('link'), have a working framework for consistently generated links inside and outside of Twig happens.
+ * - attributes: An array of HTML attributes for the wrapper of the signature.
Can we re-roll this patch without mentioning PHP data types in Twig template docs?
Comment #48
star-szr@thedavidmeister - so it sounds like for now we should use l() instead of '#theme' => 'link' in this patch.
Comment #49
thedavidmeister CreditAttribution: thedavidmeister commented#48 - Unless you want to postpone this issue on gut l(), I think that's a good idea. Unless we're passing a renderable array as content to theme('link') in this patch l() should suffice for now.
Comment #50
star-szrI'll revise this patch tonight, thanks @thedavidmeister!
Comment #51
star-szrTweaked docs throughout and used l() instead of theme_link() per #47.
Also, to my knowledge the {% spaceless %} tag in user-signature.html.twig was not actually doing anything since {% spaceless %} removes whitespace between HTML tags and there weren't any spaces between the HTML tags in the template. Ref: http://twig.sensiolabs.org/doc/tags/spaceless.html
Trying to debug a strange issue with the user-permission-description.html.twig template. It seems like the 'warning' descriptions on admin/people/permissions only show up when twig_debug is on. Posting what I have so far though, back at it tomorrow. Will also post manual testing steps after I've wrapped up.
Comment #52
star-szrComment #52.0
star-szrAdd conversion summary table
Comment #53
star-szrOkay, I must have been doing something funny last night. Both @joelpittet and myself re-tested admin/people/permissions and the warning is definitely being printed consistently. This could use another review, manual testing steps are up in the summary.
Comment #53.0
star-szrAdd testing steps
Comment #53.1
jenlamptonrm sand
Comment #53.2
jenlamptonremove theme_user_sig
Comment #54
jenlamptonreviewing
Comment #55
jenlamptonNeeded changes
- user-sginature template is never used, can be removed.
- The title attribute on username is missing
- extra whitespace in user-permission-description
Manual testing:
user-permission-description.html.twig
before:
after:
username.html.twig
before:
after:
user.html.twig
before:
after:
Comment #56
jenlamptonOkay, three minor things from above fixed. Needs another review!
Comment #56.0
jenlamptonremove
Comment #57
star-szrThanks @jenlampton! Updated the testing steps to remove the user signature testing.
Comment #58
scor CreditAttribution: scor commentedThis is in the interdiff of #56:
This will wipe out any other key from the 'link_attributes' array element. Is this change intentional?
Comment #59
jenlamptonThat was completely unintentional. good catch scor! rerolled.
Comment #60
jenlamptonwhoops, cant review my own patch :)
Comment #61
c4rl CreditAttribution: c4rl commentedAfter conferring with jenlampton in IRC, I'm going to re-roll this with theme_user_permission_description deprecated -- it's really doing nothing besides providing some basic markup in a form item #description.
If someone *really* wants to change this they can do a form alter.
Comment #62
c4rl CreditAttribution: c4rl commentedThe work from #59 looks pretty solid. I made a few changes to remove theme_user_permission_description.
If these additional changes seem good, I think this is ready for RTBC.
Comment #62.0
c4rl CreditAttribution: c4rl commentedUpdate testing steps
Comment #63
jenlamptonthis may not apply to head anymore. retesting.
Comment #64
jenlampton#62: drupal-1898468-62.patch queued for re-testing.
Comment #65
thedavidmeister CreditAttribution: thedavidmeister commentedI'm cool with dropping this theme function - a form alter does seem more appropriate than a theme function that does nothing but produce markup for descriptions of one form. That said, I don't know if it's just me but I can barely read this multiline, nested ternary thingo.
I find something like this easier to understand even if it's a bit more verbose:
Comment #66
thedavidmeister CreditAttribution: thedavidmeister commentedOr doing it with a renderable array:
Comment #67
c4rl CreditAttribution: c4rl commentedMade changes for legibility as indicated by #65.
Interdiff with respect to #62.
Comment #68
c4rl CreditAttribution: c4rl commentedComment #69
c4rl CreditAttribution: c4rl commentedPer #1757550-44: [Meta] Convert core theme functions to Twig templates, retitling to indicate this issue applies to templates rather than theme_ functions. See #1987418: user.module - Convert theme_ functions to Twig for theme_ function conversion.
Comment #69.0
c4rl CreditAttribution: c4rl commentedUpdated issue summary.
Comment #70
shanethehat CreditAttribution: shanethehat commentedComment #71
shanethehat CreditAttribution: shanethehat commentedComment #72
star-szrThanks @shanethehat, off to @steveoliver for another review!
Comment #73
star-szrJust moving the rdf.module changes over to #1987418: user.module - Convert theme_ functions to Twig for now.
Comment #74
star-szrAnd we don't need to instantiate an Attribute object in preprocess anymore now that #1982024: Lazy-load Attribute objects later in the rendering process only if needed is in.
Comment #75
steveoliver CreditAttribution: steveoliver commentedHow about adding 'attributes' => array() here?
...so we can set 'profile' class as
$variables['attributes']['class'][] = 'profile'
?I'm just concerned about the possibility that we'd potentially override existing attributes.
Comment #76
star-szrThanks @steveoliver!
Point #1 - don't think so, this uses a render element so we can't define any additional variables in hook_theme() as far as I know. Plus the default attributes variable is created in template_preprocess() anyway.
Point #2 - I agree, that would be better. CNW to change that line.
Comment #77
idflood CreditAttribution: idflood commentedHere is a reroll with the modified line. But when testing the patch I've found that a "clean" drupal will only have a "profile" class, same as with the patch in #74. With the proposed change the article got two classes "profile user".
So I attached a second patch "1898468-77-alternate.patch" with a little difference:
ps: the interdiff is against the first patch.
edit: It has been decided over irc that the the first patch is better than the alternate version. #1938430: Don't add a default theme hook class in template_preprocess() will remove the undesired "user" class.
Comment #78
star-szr@idflood - thanks! 1898468-77.patch should be good markup-wise after #1938430: Don't add a default theme hook class in template_preprocess() lands, which we just unpostponed after IRC discussions. I was under the impression that all .tpl's used the default theme hook class but this is an exception.
Comment #79
star-szrTagging for profiling.
Comment #80
star-szrCan you please give this another look @steveoliver? I know we're waiting on #1938430: Don't add a default theme hook class in template_preprocess() but the first patch in #77 could still use a review for code and docs.
Comment #81
star-szr#1938430: Don't add a default theme hook class in template_preprocess() is in so the output from 1898468-77.patch should be good now!
Comment #82
steveoliver CreditAttribution: steveoliver commentedLooks good. Only needs docs tweaks and profiling.
I know they weren't documented in the .tpl.php, but maybe we add a line for " - attributes: HTML attributes for the container element."
As mentioned in #39, I'm pretty sure we don't need the "...to check access for" and the namespace of the User class here.
I'll do the profiling in the meantime.
Comment #83
idflood CreditAttribution: idflood commentedHere is a reroll with the changes proposed in #82.
Comment #84
steveoliver CreditAttribution: steveoliver commentedstats on #83 vs. 8.x with a view of 50 users:
So we have a ~3.25% performance penalty with Twig here.
Comment #85
thedavidmeister CreditAttribution: thedavidmeister commentedDo we need this {% if %} here? the not empty thing was fixed a while ago.
Why on earth is such a simple template causing a 3.25% performance penalty?? that's so weird.
Comment #86
thedavidmeister CreditAttribution: thedavidmeister commentedwait.. what am i looking at in #84? is that right? wt (wall time) is what we're using and that profiling says 110-120% performance penalty. Since much more complicated patches than this came back with about 0.3% penalty for Twig I think something fishy has happening here.
Could we get a cross-check on the profiling here?
Comment #87
steveoliver CreditAttribution: steveoliver commentedYeah, something's not right -- The {% if %} check didn't seem to make any difference. I'll run these tests again and see what I get -- another pass from someone else would also be appreciated.
Comment #88
steveoliver CreditAttribution: steveoliver commentedCrazier still:
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=5196ef15c751e&...
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=5196ef15c751e&...
...Something's screwy.
Comment #89
steveoliver CreditAttribution: steveoliver commentedOne more. Same test (clarification: 51 users, not 50 ;)). Baseline is stable this time:
Baseline check: http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51972fb0786ef&...
Patch (from #83): http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51972fb0786ef&...
I'd like someone else to run tests on their end. Unassigning and moving on.
Comment #90
star-szrOkay, that looks much more reasonable @steveoliver, thanks :)
The first run/baseline does not have Twig loaded at all, so the numbers are reflecting loading Twig and this conversion. To isolate the profiling to only this conversion in cases like this where there are no nodes on the page, I've been creating a sample node and creating a views block to display that node and placing it somewhere on the page. Also, always use the Stark theme (so that node.html.twig is used) and enable APC class loader in settings.php.
I will re-profile this based on the above.
Comment #91
star-szrPatch here just removes the 'is not empty' from the if.
Here is profiling for the user profile page :)
I am also working on a more detailed profiling write-up to post somewhere - maybe on #1987510: [meta] Convert all core *.tpl.php templates to Twig as singular patch.
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51978cc86c728&...
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51978cc86c728&...
So performance is definitely comparable here. @steveoliver, RTBC?
Comment #92
steveoliver CreditAttribution: steveoliver commentedAh, but of course! Thanks Cottser. Definitely RTBC.
Comment #93
thedavidmeister CreditAttribution: thedavidmeister commentedComment #94
star-szrOops, meant to do that. Thanks @thedavidmeister!
Comment #95
alexpott+1. Ready for #1987510: [meta] Convert all core *.tpl.php templates to Twig as singular patch
Comment #95.0
alexpottRevise summary
Comment #96
alexpottCommitted dcfd4de and pushed to 8.x. Thanks!
Comment #97.0
(not verified) CreditAttribution: commentedAdd commit message