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 issue
  • Patch review
  • Profiling (done in #72)
Theme function name/template path Conversion status
theme_user_admin_permissions Was converted in #type table issue: #1938938: Convert theme_user_admin_permissions() to table #type
theme_user_admin_roles 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

  1. Create an article node and add a comment to it. The username on the comment should be output by username.html.twig.
  2. 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

#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

CommentFileSizeAuthor
#99 1987418-twig-user-99.patch14.03 KBLinL
#96 interdiff.txt1.17 KBjoelpittet
#96 1987418-twig-user-96.patch14.1 KBjoelpittet
#94 interdiff.txt1.12 KBjoelpittet
#94 1987418-twig-user-94.patch14.17 KBjoelpittet
#91 1987418-twig-user-91.patch14.11 KBscor
#89 1987418-twig-user-89.patch14.05 KBjoelpittet
#89 interdiff.txt2.2 KBjoelpittet
#84 1987418-twig-user-83.patch14.07 KBLewisNyman
#77 1987418-twig-user-77.patch14.11 KBManuel Garcia
#75 1987418-innerdiff-75.txt649 bytesManuel Garcia
#75 1987418-twig-user-75.patch13.05 KBManuel Garcia
#70 1987418-twig-user-70.patch14.34 KBManuel Garcia
#61 1987418-twig-user-61.patch14.32 KBjoelpittet
#61 interdiff.txt889 bytesjoelpittet
#59 1987418-twig-user-58.patch14.31 KBjoelpittet
#59 interdiff.txt607 bytesjoelpittet
#55 1987418-twig-user-55.patch14.31 KBjoelpittet
#49 1987418-49-twig-user.patch13.9 KBjoelpittet
#49 interdiff.txt3.32 KBjoelpittet
#48 1987418-48-twig-user.patch10.62 KBscor
#48 interdiff.txt3.15 KBscor
#45 1987418-45-twig-user.patch15.71 KBdrupalninja99
#45 interdiff.txt3.07 KBdrupalninja99
#40 1987418-40-twig-user.patch15.66 KBdrupalninja99
#40 interdiff.txt901 bytesdrupalninja99
#39 1987418-39-twig-user.patch15.46 KBdrupalninja99
#36 1987418-36-twig-user.patch13.21 KBjoelpittet
#33 useradmin-usermodule-rdfmodule-1987418-33.patch20.24 KBrakhimandhania
#30 useradmin-user-rdf-1987418-29.patch19.75 KBrakhimandhania
#29 useradmin-user-rdf-1987418-29.patch19.75 KBrakhimandhania
#27 user-rdf-useradmin-1987418-27.patch19.38 KBrakhimandhania
#26 1987418-26-partial.patch3.03 KBscor
#14 1987418-14.patch12.28 KBminneapolisdan
#10 1987418-10.patch12.26 KBstar-szr
#10 interdiff.txt1.77 KBstar-szr
#6 interdiff.txt515 bytesshanethehat
#6 twig-user-theme-only-1987418-6.patch10.5 KBshanethehat
#2 twig-user-theme-only-1987418-2.patch10.6 KBshanethehat
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

star-szr’s picture

Issue summary: View changes

Updated issue summary.

c4rl’s picture

Title: user.module - Convert PHPTemplate templates to Twig » user.module - Convert theme_ functions to Twig

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

c4rl’s picture

Issue summary: View changes

Updated issue summary.

shanethehat’s picture

Status: Active » Needs review
FileSize
10.6 KB

Status: Needs review » Needs work
Issue tags: -needs profiling, -Twig

The last submitted patch, twig-user-theme-only-1987418-2.patch, failed testing.

shanethehat’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
Issue tags: +needs profiling, +Twig

The last submitted patch, twig-user-theme-only-1987418-2.patch, failed testing.

shanethehat’s picture

Status: Needs work » Needs review
FileSize
10.5 KB
515 bytes
star-szr’s picture

Status: Needs review » Needs work

Thanks @shanethehat! The rdf.module (rdf_preprocess_username()) changes should be moved here as well.

shanethehat’s picture

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

<a lang="" datatype="" property="foaf:name" typeof="sioc:UserAccount" about="/user/1" class="username" title="View user profile." href="/user/1">admin</a>

And this is unauthenticated:

<span lang="" datatype="" property="foaf:name" typeof="sioc:UserAccount" about="/user/1" class="username">admin</span>

Same two again without the patch:

<a lang="" datatype="" property="foaf:name" typeof="sioc:UserAccount" about="/user/1" class="username" title="View user profile." href="/user/1">admin</a>
<span lang="" datatype="" property="foaf:name" typeof="sioc:UserAccount" about="/user/1" class="username">admin</span>
star-szr’s picture

Status: Needs work » Needs review

See #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 :)

star-szr’s picture

FileSize
1.77 KB
12.26 KB

Eh, looking at it again I think removing NestedArray::mergeDeep() will be a performance bump anyway, so moving that code here for now.

minneapolisdan’s picture

Issue tags: -needs profiling, -Twig

#10: 1987418-10.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +needs profiling, +Twig

The last submitted patch, 1987418-10.patch, failed testing.

minneapolisdan’s picture

Assigned: Unassigned » minneapolisdan

going to reroll this

minneapolisdan’s picture

Status: Needs work » Needs review
FileSize
12.28 KB

Patch rerolled for testing, hope it works, first time doing this...

ezeedub’s picture

I'm gonna profile this.

ezeedub’s picture

Base Scenario: 1 node, with 10 comments.

Scenario #1: anon users without "view user profiles" perm (username not a link).

=== 8.x..8.x compared (519ffb7471c72..519ffbe6eb79f):

ct  : 74,534|74,534|0|0.0%
wt  : 577,759|578,488|729|0.1%
cpu : 572,035|572,036|1|0.0%
mu  : 7,165,108|7,165,108|0|0.0%
pmu : 7,250,996|7,250,996|0|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=519ffb7471c72&...

=== 8.x..user-1987418-14 compared (519ffb7471c72..519ffc4c0008b):

ct  : 74,534|75,012|478|0.6%
wt  : 577,759|580,827|3,068|0.5%
cpu : 572,035|576,036|4,001|0.7%
mu  : 7,165,108|7,181,572|16,464|0.2%
pmu : 7,250,996|7,269,684|18,688|0.3%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=519ffb7471c72&...

Scenario #2: anon users with "view user profiles" perm (username is a link).

=== 8.x..8.x compared (519ff7ed8fdf9..519ff8a97ec46):

ct  : 75,143|75,143|0|0.0%
wt  : 581,907|583,045|1,138|0.2%
cpu : 576,035|584,036|8,001|1.4%
mu  : 7,165,648|7,165,648|0|0.0%
pmu : 7,251,932|7,251,932|0|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=519ff7ed8fdf9&...

=== 8.x..user-1987418-14 compared (519ff7ed8fdf9..519ff921aa26d):

ct  : 75,143|75,818|675|0.9%
wt  : 581,907|586,375|4,468|0.8%
cpu : 576,035|584,037|8,002|1.4%
mu  : 7,165,648|7,183,248|17,600|0.2%
pmu : 7,251,932|7,271,908|19,976|0.3%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=519ff7ed8fdf9&...

Status: Needs review » Needs work
Issue tags: -needs profiling, -Twig

The last submitted patch, 1987418-14.patch, failed testing.

ezeedub’s picture

Status: Needs work » Needs review
Issue tags: +needs profiling, +Twig

#14: 1987418-14.patch queued for re-testing.

ezeedub’s picture

Issue summary: View changes

Revise summary

ezeedub’s picture

Same 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).

=== 8.x..8.x compared (51a0ef63cb19e..51a0f01f2a0ce):

ct  : 74,684|74,684|0|0.0%
wt  : 578,626|577,632|-994|-0.2%
cpu : 572,037|572,036|-1|-0.0%
mu  : 7,090,464|7,090,464|0|0.0%
pmu : 7,168,320|7,168,320|0|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51a0ef63cb19e&...

=== 8.x..user-1987418-14 compared (51a0ef63cb19e..51a0f086f2681):

ct  : 74,684|75,176|492|0.7%
wt  : 578,626|582,229|3,603|0.6%
cpu : 572,037|580,036|7,999|1.4%
mu  : 7,090,464|7,108,108|17,644|0.2%
pmu : 7,168,320|7,188,412|20,092|0.3%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51a0ef63cb19e&...

Scenario #2: anon users with "view user profiles" perm (username is a link).

=== 8.x..8.x compared (51a0f1cae1ba2..51a0f2649c790):

ct  : 75,293|75,293|0|0.0%
wt  : 579,271|580,226|955|0.2%
cpu : 572,036|576,035|3,999|0.7%
mu  : 7,091,076|7,091,076|0|0.0%
pmu : 7,169,384|7,169,384|0|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51a0f1cae1ba2&...

=== 8.x..user-1987418-14 compared (51a0f1cae1ba2..51a0f3032a9a3):

ct  : 75,293|75,982|689|0.9%
wt  : 579,271|587,180|7,909|1.4%
cpu : 572,036|584,037|12,001|2.1%
mu  : 7,091,076|7,113,908|22,832|0.3%
pmu : 7,169,384|7,194,652|25,268|0.4%

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

ezeedub’s picture

Issue tags: -Twig

Tried again on a different machine. This time, 1 node with 25 comments, anon user with perm to view user profiles.

=== 8.x..8.x compared (51a14c32c53db..51a14d7a98950):

ct  : 231,302|231,302|0|0.0%
wt  : 2,097,618|2,107,001|9,383|0.4%
cpu : 2,092,130|2,104,132|12,002|0.6%
mu  : 16,319,616|16,319,616|0|0.0%
pmu : 16,741,848|16,741,848|0|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51a14c32c53db&...

=== 8.x..user-1987418-14 compared (51a14c32c53db..51a14dd399448):

ct  : 231,302|235,805|4,503|1.9%
wt  : 2,097,618|2,141,044|43,426|2.1%
cpu : 2,092,130|2,132,133|40,003|1.9%
mu  : 16,319,616|16,351,812|32,196|0.2%
pmu : 16,741,848|16,773,660|31,812|0.2%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51a14c32c53db&...

ezeedub’s picture

Issue tags: +Twig

1 node, 25 comments, anon user with no perm to see user profiles.

=== user-1987418-14..user-1987418-14 compared (51a152cadd6cc..51a15418571e4):

ct  : 232,850|232,850|0|0.0%
wt  : 2,102,579|2,106,663|4,084|0.2%
cpu : 2,100,132|2,100,132|0|0.0%
mu  : 16,349,324|16,349,324|0|0.0%
pmu : 16,768,912|16,768,912|0|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51a152cadd6cc&...

=== user-1987418-14..user-1987418-14 compared (51a152cadd6cc..51a155e569388):

ct  : 232,850|232,850|0|0.0%
wt  : 2,102,579|2,105,360|2,781|0.1%
cpu : 2,100,132|2,100,131|-1|-0.0%
mu  : 16,349,324|16,349,324|0|0.0%
pmu : 16,768,912|16,768,912|0|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51a152cadd6cc&...

ezeedub’s picture

'nuther machine,

1 node, 25 comments, user has perm to view profiles

=== 8.x..8.x compared (51a152d922ae0..51a154409a1df):

ct  : 253,640|253,640|0|0.0%
wt  : 1,998,842|2,001,850|3,008|0.2%
cpu : 1,940,122|1,952,121|11,999|0.6%
mu  : 10,930,576|10,930,576|0|0.0%
pmu : 11,451,020|11,451,020|0|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51a152d922ae0&...

=== 8.x..user-1987418-14 compared (51a152d922ae0..51a1557b90314):

ct  : 253,640|257,668|4,028|1.6%
wt  : 1,998,842|2,067,084|68,242|3.4%
cpu : 1,940,122|2,016,125|76,003|3.9%
mu  : 10,930,576|10,876,644|-53,932|-0.5%
pmu : 11,451,020|11,395,464|-55,556|-0.5%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51a152d922ae0&...

user can't view user profiles.

=== 8.x..8.x compared (51a14be6130cb..51a14e1b1e097):

ct  : 214,527|214,527|0|0.0%
wt  : 1,630,801|1,629,902|-899|-0.1%
cpu : 1,624,101|1,624,101|0|0.0%
mu  : 8,945,588|8,945,588|0|0.0%
pmu : 9,598,592|9,598,592|0|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51a14be6130cb&...

=== 8.x..user-1987418-14 compared (51a14be6130cb..51a14f12e142e):

ct  : 214,527|217,260|2,733|1.3%
wt  : 1,630,801|1,648,433|17,632|1.1%
cpu : 1,624,101|1,640,102|16,001|1.0%
mu  : 8,945,588|8,892,180|-53,408|-0.6%
pmu : 9,598,592|9,547,012|-51,580|-0.5%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51a14be6130cb&...

thedavidmeister’s picture

Assigned: minneapolisdan » Unassigned
Status: Needs review » Needs work
Issue tags: -needs profiling

this looks like it's too slow still. 3-68ms slowdowns here :/

thedavidmeister’s picture

Issue summary: View changes

Updated issue summary.

jenlampton’s picture

Issue tags: +needs profiling

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

star-szr’s picture

Issue tags: +Needs reroll

This one needs a reroll.

scor’s picture

FileSize
3.03 KB

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

rakhimandhania’s picture

Status: Needs work » Needs review
FileSize
19.38 KB

Re-rolling the patch after comment-14

Status: Needs review » Needs work

The last submitted patch, user-rdf-useradmin-1987418-27.patch, failed testing.

rakhimandhania’s picture

Re-rolling the patch after comment-14

rakhimandhania’s picture

Status: Needs work » Needs review
FileSize
19.75 KB

Status: Needs review » Needs work

The last submitted patch, useradmin-user-rdf-1987418-29.patch, failed testing.

scor’s picture

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

rakhimandhania’s picture

Status: Needs work » Needs review
FileSize
20.24 KB

Status: Needs review » Needs work

The last submitted patch, useradmin-usermodule-rdfmodule-1987418-33.patch, failed testing.

star-szr’s picture

This one might be a bit trickier to reroll. Thanks @scor and @rakhimandhania for your work :)

joelpittet’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
13.21 KB

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

Status: Needs review » Needs work

The last submitted patch, 1987418-36-twig-user.patch, failed testing.

drupalninja99’s picture

I feel like the isset condition "isset($variables['link_path'])" needs to remain bc we are getting a lot of undefined link_path warnings.

-function theme_username($variables) {
-  if (isset($variables['link_path'])) {
-    // We have a link path, so we should generate a link using l().
-    // Additional classes may be added as array elements like
-    // $variables['link_options']['attributes']['class'][] = 'myclass';
-    $output = l($variables['name'] . $variables['extra'], $variables['link_path'], $variables['link_options']);
-  }
-  else {
-    // Modules may have added important attributes so they must be included
-    // in the output. Additional classes may be added as array elements like
-    // $variables['attributes']['class'][] = 'myclass';
-    $output = '<span' . new Attribute($variables['attributes']) . '>' . $variables['name'] . $variables['extra'] . '</span>';
-  }
-  return $output;
+  $variables['link'] = l($variables['name'] . $variables['extra'], $variables['link_path'], $variables['link_options']);
 }
drupalninja99’s picture

FileSize
15.46 KB

Here is the #36 patch rerolled with a couple conflicts fix.

drupalninja99’s picture

Status: Needs work » Needs review
FileSize
901 bytes
15.66 KB

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

Status: Needs review » Needs work

The last submitted patch, 1987418-40-twig-user.patch, failed testing.

joelpittet’s picture

Good call @drupalninja99 thanks for putting that back in.

joelpittet’s picture

+++ b/core/modules/user/user.admin.inc
@@ -6,32 +6,86 @@
+function user_admin_account() {

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

drupalninja99’s picture

Ya I had a lot of problems rerolling the patch. I will likely have to do some manual fixing.

drupalninja99’s picture

Status: Needs work » Needs review
FileSize
3.07 KB
15.71 KB

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

joelpittet’s picture

Nice work, that's a good call:)

Status: Needs review » Needs work

The last submitted patch, 1987418-45-twig-user.patch, failed testing.

scor’s picture

Status: Needs work » Needs review
FileSize
3.15 KB
10.62 KB

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

joelpittet’s picture

FileSize
3.32 KB
13.9 KB

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

scor’s picture

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

joelpittet’s picture

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

joelpittet’s picture

Issue summary: View changes

rm todo

joelpittet’s picture

49: 1987418-49-twig-user.patch queued for re-testing.

star-szr’s picture

Issue tags: +Twig conversion
star-szr’s picture

joelpittet’s picture

FileSize
14.31 KB

Re-rolled #49

Status: Needs review » Needs work

The last submitted patch, 55: 1987418-twig-user-55.patch, failed testing.

The last submitted patch, 55: 1987418-twig-user-55.patch, failed testing.

joelpittet’s picture

+++ b/core/modules/user/user.module
@@ -639,53 +641,26 @@ function template_preprocess_username(&$variables) {
   return $output;

Forgot to remove this.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
607 bytes
14.31 KB

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

<p class="submitted">Submitted by     <span class="username">bJn0WaqF</span>
 on Sun, 03/09/2014 - 15:04</p>

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?

Status: Needs review » Needs work

The last submitted patch, 59: 1987418-twig-user-58.patch, failed testing.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
889 bytes
14.32 KB

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

star-szr’s picture

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

joelpittet’s picture

D7:

<a class="username" datatype="" property="foaf:name" typeof="sioc:UserAccount" about="/ca/user/1" xml:lang="" title="View user profile." href="/ca/user/1">admin</a>

In D7's preprocess:

$variables['attributes_array'] = array('class' => array('username'));

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.

jsbalsera’s picture

Assigned: Unassigned » jsbalsera
jsbalsera’s picture

Tested. As a logged user the markup is:

<p class="commenter-name">
          <span rel="schema:author"><a lang="" datatype="" property="schema:name" typeof="schema:Person" about="/user/1" href="/user/1" class="username" title="Ver perfil del usuario.">admin</a></span>
        </p>

And as an anonymous user:

<p class="commenter-name">
          <span rel="schema:author"><span class="username" lang="" about="/user/1" typeof="schema:Person" property="schema:name" datatype="">admin</span></span>
        </p>

so it looks good.

pjezek’s picture

=== 1987418-twig-user-61..1987418-twig-user-61 compared (5336ab4751a49..5336adea0ce29):
ct  : 59,909|59,909|0|0.0%
wt  : 320,047|323,405|3,358|1.0%
cpu : 304,031|306,075|2,044|0.7%
mu  : 19,102,632|19,102,720|88|0.0%
pmu : 19,184,344|19,184,736|392|0.0%
=== 1987418-twig-user-61..1987418-twig-user-61 compared (5336ab4751a49..5336ae2ee2e72):
ct  : 59,909|59,909|0|0.0%
wt  : 320,047|323,483|3,436|1.1%
cpu : 304,031|306,226|2,195|0.7%
mu  : 19,102,632|19,102,720|88|0.0%
pmu : 19,184,344|19,184,736|392|0.0%
pjezek’s picture

Issue tags: -needs profiling
pjezek’s picture

I did the profiling and reviewed the change. From my point of view it is RTBC.

joelpittet’s picture

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

Manuel Garcia’s picture

FileSize
14.34 KB

Patch on #61 was no longer applying, here is a re-roll

star-szr’s picture

Assigned: jsbalsera » star-szr
Issue summary: View changes
Issue tags: +needs profiling

Updating conversion table, going to take a look at profiling/reviewing this.

star-szr’s picture

Assigned: star-szr » Unassigned
Issue tags: -needs profiling

Here'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:

  • 51 uses of the theme function/template
  • Anonymous user can view user profiles so the link is being used
  • RDF enabled
  • Render cache disabled using https://gist.github.com/Cottser/10997441, the page is not actually this slow
=== 8.x..8.x compared (535139ab6e707..53513a6d2163d):

ct  : 385,879|385,879|0|0.0%
wt  : 1,286,762|1,284,116|-2,646|-0.2%
cpu : 1,248,479|1,245,674|-2,805|-0.2%
mu  : 43,635,608|43,635,608|0|0.0%
pmu : 43,998,208|43,998,208|0|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=535139ab6e707&...

=== 8.x..twig-user-1987418-70 compared (535139ab6e707..53513afe3c568):

ct  : 385,879|387,550|1,671|0.4%
wt  : 1,286,762|1,292,363|5,601|0.4%
cpu : 1,248,479|1,254,847|6,368|0.5%
mu  : 43,635,608|43,659,440|23,832|0.1%
pmu : 43,998,208|44,020,856|22,648|0.1%

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

Manuel Garcia’s picture

#70 patch is still applying cleanly, profiling is done... is this is ready for RTBC?

joelpittet’s picture

Thanks for the poke.

+++ b/core/modules/user/templates/username.html.twig
@@ -0,0 +1,30 @@
+  {#-
+    Modules may have added important attributes so they must be included
+    in the output. Additional classes may be added as array elements like
+    {% set attributes.class = attributes.class|merge(["myclass"]) %}
+  -#}

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.

Manuel Garcia’s picture

Here is the same patch as #70 with the comments mentioned in #74 removed.

Status: Needs review » Needs work

The last submitted patch, 75: 1987418-twig-user-75.patch, failed testing.

Manuel Garcia’s picture

Status: Needs work » Needs review
FileSize
14.11 KB

Forgot to add the new twig file...

joelpittet’s picture

Thank you @Manuel Garcia, I think this just needs a manual testing as I don't see any above.

LewisNyman’s picture

77: 1987418-twig-user-77.patch queued for re-testing.

LewisNyman’s picture

Issue tags: +Needs reroll

Status: Needs review » Needs work

The last submitted patch, 77: 1987418-twig-user-77.patch, failed testing.

joelpittet’s picture

Issue summary: View changes
joelpittet’s picture

Issue summary: View changes
LewisNyman’s picture

Status: Needs work » Needs review
FileSize
14.07 KB

Reroll.

joelpittet’s picture

Issue tags: -Needs reroll

Sweet thanks for the re-roll @LewisNyman. Needs manual testing next.

scor’s picture

Manual testing looks good.

  • username logged in

    without the patch:

    <a href="/user/1" title="View user profile." class="username" lang="" about="/user/1" typeof="schema:Person" property="schema:name" datatype="">admin</a>
    

    with the patch:

    <!-- THEME DEBUG -->
    <!-- CALL: _theme('username') -->
    <!-- BEGIN OUTPUT from 'core/modules/user/templates/username.html.twig' -->
    <a title="View user profile." class="username" href="/user/1" lang="" about="/user/1" typeof="schema:Person" property="schema:name" datatype="">admin</a>
    <!-- END OUTPUT from 'core/modules/user/templates/username.html.twig' -->
    
  • username logged out

    without the patch:

    <span lang="" about="/user/1" typeof="schema:Person" property="schema:name" datatype="">admin</span>
    

    with the patch:

    <!-- THEME DEBUG -->
    <!-- CALL: _theme('username') -->
    <!-- BEGIN OUTPUT from 'core/modules/user/templates/username.html.twig' -->
    <span class="username" lang="" about="/user/1" typeof="schema:Person" property="schema:name" datatype="">admin</span>
    <!-- END OUTPUT from 'core/modules/user/templates/username.html.twig' -->
    
  • permission warning

    without the patch:

    <div class="permission form-item form-type-item form-item-permissions-synchronize-configuration-description" id="edit-permissions-synchronize-configuration-description">
          Synchronize configuration
              <div class="description" id="edit-permissions-synchronize-configuration-description--description">
          <em class="permission-warning">Warning: Give to trusted roles only; this permission has security implications.</em>
        </div>
      </div>
    

    with the patch:

    <div class="permission form-item form-type-item form-item-permissions-synchronize-configuration-description" id="edit-permissions-synchronize-configuration-description">
          Synchronize configuration
              <div class="description" id="edit-permissions-synchronize-configuration-description--description">
           <em class="permission-warning">Warning: Give to trusted roles only; this permission has security implications.</em>
        </div>
      </div>
    
scor’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs manual testing, -Novice
star-szr’s picture

Status: Reviewed & tested by the community » Needs work

This is not quite ready for prime time yet, but it's mostly docs and maybe a logic tweak from what I can see:

  1. +++ b/core/modules/system/system.api.php
    @@ -956,19 +956,9 @@ function hook_system_info_alter(array &$info, \Drupal\Core\Extension\Extension $
    - *   - warning: (optional) A translated warning message to display for this
    - *     permission on the permission administration page. This warning overrides
    - *     the automatic warning generated by 'restrict access' being set to TRUE.
    - *     This should rarely be used, since it is important for all permissions to
    - *     have a clear, consistent security warning that is the same across the
    - *     site. Use the 'description' key instead to provide any information that
    - *     is specific to the permission you are defining.
    

    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.

  2. +++ b/core/modules/user/user.module
    @@ -616,7 +610,15 @@ function user_template_preprocess_default_variables_alter(&$variables) {
    + *   - uid: The user uid number.
    

    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.

  3. +++ b/core/modules/user/user.module
    @@ -652,55 +654,26 @@ function template_preprocess_username(&$variables) {
    +  // We have a link path, so we should generate a link using l().
    ...
    +    $variables['attributes']['href'] = url($variables['link_path'], $link_options);
    

    This comment talks about using l() but we are actually using url() now.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
2.2 KB
14.05 KB

Regarding #88.1 re: append vs replace.

  • This theme function gets used once and it's abstracted from it's context a bit too much.
  • It has the possibility to produce no output (surrounding if) which would add a \n for twig template if hide = true.
  • It's drupal_rendered into a #description key so that removes the render array build up and instant flattening. From a look through all the #description keys it seems to be the only one that does this differently.

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?

star-szr’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll

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

scor’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
14.11 KB

reroll

star-szr’s picture

Issue summary: View changes
Status: Needs review » Needs work

Thanks @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.

  1. +++ b/core/modules/user/user.module
    @@ -618,7 +612,14 @@ function user_template_preprocess_default_variables_alter(&$variables) {
    + * @param array $variables
    + *   An associative array containing:
    + *   - account: The user account to check access for
    + *     (Drupal\user\Plugin\Core\Entity\User).
    

    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.

  2. +++ b/core/modules/user/templates/username.html.twig
    @@ -0,0 +1,25 @@
    + * - link_options: Options to pass to the url() function's $options parameter if
    + *   linking the user's name to the user's page.
    

    The link_options variable is no longer being set, so this should be removed from the Twig template.

star-szr’s picture

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

joelpittet’s picture

Status: Needs work » Needs review
FileSize
14.17 KB
1.12 KB

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

star-szr’s picture

Ok, the markup change has a good justification then. One idea for another small simplification, otherwise RTBC from my perspective:

+++ b/core/modules/user/user.module
@@ -108,15 +108,9 @@ function user_theme() {
+      'variables' => array('account' => NULL, 'attributes' => array(), 'link_options' => NULL),

@@ -655,56 +655,26 @@ function template_preprocess_username(&$variables) {
+    $link_options = isset($variables['link_options']) ? $variables['link_options'] : array();
+    $variables['attributes']['href'] = url($variables['link_path'], $link_options);

Let's initialize link_options as an empty array in hook_theme() and just pass $variables['link_options'] to url().

joelpittet’s picture

FileSize
14.1 KB
1.17 KB

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

star-szr’s picture

Status: Needs review » Reviewed & tested by the community

Nice, thanks for checking that @joelpittet :)

joelpittet’s picture

I 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;)

LinL’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
14.03 KB

Quick reroll as patch no longer applied following change to comments in system.api.php in #2216547: Fill in topic/@defgroup for User system overview

star-szr’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

Reroll looks good, thank you very much @LinL!

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 1742e5e and pushed to 8.x. Thanks!

  • alexpott committed 1742e5e on 8.x
    Issue #1987418 by joelpittet, drupalninja99, rakhimandhania, Manuel...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.