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.

#1987418: user.module - Convert theme_ functions to Twig Assigned to: Cottser
#1757550: [META-63] Convert core theme functions to Twig templates
#1938938: Convert theme_user_admin_permissions() to table #type
#1548204: Convert user signature into a normal field
#1843788: Remove template_process_username()

Files: 
CommentFileSizeAuthor
#91 1898468-91.patch5 KBCottser
PASSED: [[SimpleTest]]: [MySQL] 55,667 pass(es).
[ View ]
#91 interdiff.txt384 bytesCottser
#83 1898468-83.patch5.01 KBidflood
PASSED: [[SimpleTest]]: [MySQL] 56,061 pass(es).
[ View ]
#83 interdiff.txt1.25 KBidflood
#77 1898468-77.patch5.01 KBidflood
PASSED: [[SimpleTest]]: [MySQL] 55,689 pass(es).
[ View ]
#77 1898468-77-alternate.patch5.02 KBidflood
PASSED: [[SimpleTest]]: [MySQL] 55,812 pass(es).
[ View ]
#77 interdiff.txt633 bytesidflood
#74 1898468-74.patch5.03 KBCottser
PASSED: [[SimpleTest]]: [MySQL] 55,846 pass(es).
[ View ]
#74 interdiff.txt780 bytesCottser
#73 interdiff.txt1.77 KBCottser
#73 1898468-73.patch5.3 KBCottser
PASSED: [[SimpleTest]]: [MySQL] 55,821 pass(es).
[ View ]
#71 twig-user-template-only-1898468-71.patch7.07 KBshanethehat
PASSED: [[SimpleTest]]: [MySQL] 55,901 pass(es).
[ View ]
#67 drupal-1898468-67.patch17.08 KBc4rl
PASSED: [[SimpleTest]]: [MySQL] 55,398 pass(es).
[ View ]
#67 drupal-1898468-67.patch-interdiff.txt1.42 KBc4rl
#62 drupal-1898468-62.patch16.9 KBc4rl
PASSED: [[SimpleTest]]: [MySQL] 54,465 pass(es).
[ View ]
#62 drupal-1898468-62.patch-interdiff.txt5.55 KBc4rl
#59 twig-convert_user-1898468-59.patch15.66 KBjenlampton
PASSED: [[SimpleTest]]: [MySQL] 54,477 pass(es).
[ View ]
#56 twig-convert_user-1898468-56.patch16.2 KBjenlampton
PASSED: [[SimpleTest]]: [MySQL] 54,438 pass(es).
[ View ]
#56 interdiff.txt3.12 KBjenlampton
#51 1898468-51.patch16.59 KBCottser
PASSED: [[SimpleTest]]: [MySQL] 54,446 pass(es).
[ View ]
#51 interdiff.txt4.66 KBCottser
#42 interdiff.txt1.42 KBscor
#41 twig-user-1898468-41.patch16.75 KBscor
PASSED: [[SimpleTest]]: [MySQL] 54,425 pass(es).
[ View ]
#35 interdiff.txt2.48 KBjoelpittet
#35 twig-user-1898468-35.patch17.08 KBjoelpittet
FAILED: [[SimpleTest]]: [MySQL] 54,031 pass(es), 9 fail(s), and 0 exception(s).
[ View ]
#30 1898468-25-reconstructed.patch16.09 KBCottser
FAILED: [[SimpleTest]]: [MySQL] 53,841 pass(es), 13 fail(s), and 1,535 exception(s).
[ View ]
#29 1898468-29.patch15.45 KBCottser
FAILED: [[SimpleTest]]: [MySQL] 53,920 pass(es), 6 fail(s), and 1,407 exception(s).
[ View ]
#29 interdiff.txt6.66 KBCottser
#25 interdiff.txt3.32 KBjoelpittet
#25 twig-user-1898468-25.patch28.23 KBjoelpittet
FAILED: [[SimpleTest]]: [MySQL] 53,683 pass(es), 13 fail(s), and 1,519 exception(s).
[ View ]
#22 twig-user-1898468-22.patch15.93 KBjoelpittet
FAILED: [[SimpleTest]]: [MySQL] 53,869 pass(es), 13 fail(s), and 1,524 exception(s).
[ View ]
#22 interdiff.txt1.6 KBjoelpittet
#18 interdiff.txt12.12 KBjoelpittet
#18 twig-user-1898468-18.patch15.35 KBjoelpittet
FAILED: [[SimpleTest]]: [MySQL] 53,753 pass(es), 6 fail(s), and 1,486 exception(s).
[ View ]
#14 twig-user-1898468-14.patch22.24 KBjoelpittet
FAILED: [[SimpleTest]]: [MySQL] 53,551 pass(es), 8 fail(s), and 1,840 exception(s).
[ View ]
#14 interdiff.txt9.38 KBjoelpittet
#6 drupal-1898468-6.patch20.35 KBc4rl
FAILED: [[SimpleTest]]: [MySQL] 49,704 pass(es), 9 fail(s), and 2,026 exception(s).
[ View ]
#4 drupal-1898468-4.patch20.34 KBc4rl
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-1898468-4.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Comments

Issue tags:+Twig

Tagging

Assigned:Unassigned» trrroy

Assigned:trrroy» c4rl

Assigning

Status:Active» Needs review
StatusFileSize
new20.34 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-1898468-4.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Status:Needs review» Needs work

The last submitted patch, drupal-1898468-4.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new20.35 KB
FAILED: [[SimpleTest]]: [MySQL] 49,704 pass(es), 9 fail(s), and 2,026 exception(s).
[ View ]

Rebased.

Status:Needs review» Needs work

The last submitted patch, drupal-1898468-6.patch, failed testing.

Assigned:c4rl» rteijeiro
Issue tags:+#SprintWeekend

Assigning and tagging.

Assigned:rteijeiro» Unassigned
Status:Needs work» Needs review
Issue tags:-#SprintWeekend

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

Status:Needs review» Needs work

While I look into the various exceptions and errors, here are some standards/best practices things I noticed:

+++ b/core/modules/user/templates/user-admin-permissions.html.twig
@@ -0,0 +1,20 @@
+{#
+/**
+ * @file
+ * Default theme implementation for the administer permissions page.

DocBlock format needs updated. See drupal.org/sandbox/pixelmord/1750250#preprocess-docblocks

+++ b/core/modules/user/templates/user-admin-roles.html.twig
@@ -0,0 +1,18 @@
+{#
+/**
+ * @file
+ * Default theme implementation for the role order and new role form.

DocBlock.

+++ b/core/modules/user/templates/user-permission-description.html.twig
@@ -0,0 +1,26 @@
+{#
+/**
+ * @file
+ * Default theme implementation for an individual permission description.
+ *
+ * Available variables:

And so on...

+++ b/core/modules/user/user.admin.inc
@@ -766,7 +766,7 @@ function user_admin_permissions_submit($form, &$form_state) {
+function template_preprocess_user_admin_permissions(&$variables) {
   $form = $variables['form'];
   $roles = user_role_names();
@@ -794,14 +794,13 @@ function theme_user_admin_permissions($variables) {
   foreach (element_children($form['role_names']) as $rid) {
     $header[] = array('data' => drupal_render($form['role_names'][$rid]), 'class' => array('checkbox'));
   }
-  $output = theme('system_compact_link');
-  $output .= theme('table', array('header' => $header, 'rows' => $rows, 'attributes' => array('id' => 'permissions')));
-  $output .= drupal_render_children($form);
-  return $output;
+  $variables['system_compact_link'] = theme('system_compact_link');
+  $variables['table'] = theme('table', array('header' => $header, 'rows' => $rows, 'attributes' => array('id' => 'permissions')));
+  $variables['form'] = drupal_render_children($form);
}

Turn that theme('table') into a render array. See http://drupal.org/node/1920746#render

+++ b/core/modules/user/user.admin.inc
@@ -910,7 +903,7 @@ function user_admin_roles_order_submit($form, &$form_state) {
- * Returns HTML for the role order and new role form.
+ * Preprocess variables for user-admin-roles template.
  *
  * @param $variables
  *   An associative array containing:
@@ -918,7 +911,7 @@ function user_admin_roles_order_submit($form, &$form_state) {
@@ -918,7 +911,7 @@ function user_admin_roles_order_submit($form, &$form_state) {
  *
  * @ingroup themeable
  */
-function theme_user_admin_roles($variables) {
+function template_preprocess_user_admin_roles(&$variables) {

Docblock.

+++ b/core/modules/user/user.admin.inc
@@ -942,15 +938,16 @@ function theme_user_admin_roles($variables) {
+  $variables['table'] = theme('table', array('header' => $header, 'rows' => $rows, 'attributes' => array('id' => 'user-roles')));

Render array.

Assigned:Unassigned» rteijeiro

Thanks steveoliver,

I will continue the issue...

Issue tags:+#SprintWeekend

Updating the tags to point to the correct (at least more used) tag.

Thanks 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

Issue tags:-#SprintWeekend
StatusFileSize
new9.38 KB
new22.24 KB
FAILED: [[SimpleTest]]: [MySQL] 53,551 pass(es), 8 fail(s), and 1,840 exception(s).
[ View ]

@Cottser the tables in these two functions will be removed theme_user_admin_permissions and theme_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.

Status:Needs work» Needs review

Deploy testbot!

Status:Needs review» Needs work

The last submitted patch, twig-user-1898468-14.patch, failed testing.

Assigned:rteijeiro» Unassigned

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

StatusFileSize
new15.35 KB
FAILED: [[SimpleTest]]: [MySQL] 53,753 pass(es), 6 fail(s), and 1,486 exception(s).
[ View ]
new12.12 KB

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

Status:Needs work» Needs review

last but not least, testbot!

Status:Needs review» Needs work

The last submitted patch, twig-user-1898468-18.patch, failed testing.

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

Issue summary:View changes

Added type table issue

Assigned:Unassigned» joelpittet
StatusFileSize
new1.6 KB
new15.93 KB
FAILED: [[SimpleTest]]: [MySQL] 53,869 pass(es), 13 fail(s), and 1,524 exception(s).
[ View ]

Realized 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()

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, twig-user-1898468-22.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new28.23 KB
FAILED: [[SimpleTest]]: [MySQL] 53,683 pass(es), 13 fail(s), and 1,519 exception(s).
[ View ]
new3.32 KB

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

Status:Needs review» Needs work

The last submitted patch, twig-user-1898468-25.patch, failed testing.

First, this needs a reroll after rebasing on 8.x. Then...

+++ b/core/modules/user/user.module
@@ -802,42 +801,15 @@ function template_process_username(&$variables) {
-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>';
+    $attributes = (array) $variables['attributes'];
+    $variables['link_options']['attributes'] = new Attribute(array_merge_recursive($variables['link_attributes'], $attributes));
+    $variables['link'] = array(
+      '#theme' => 'link',
+      '#text' => $variables['name'] . $variables['extra'],
+      '#path' => $variables['link_path'],
+      '#options' => $variables['link_options'],
+    );
   }

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.

+++ b/core/modules/user/user.pages.inc
@@ -192,6 +195,10 @@ function template_preprocess_user(&$variables) {
+
+  // Set up attributes.
+  $variables['attributes'] = array('class' => array('profile'));
+  $variables['attributes'] = new Attribute($variables['attributes']);
}

Why the interim $variables['attributes']?

Just do $variables['attributes'] = new Attribute(array('class' => array('profile'))); ?

Hope that helps.

Assigned:joelpittet» Cottser

Temporarily assigning to look at the tests and #27.

Assigned:Cottser» Unassigned
Status:Needs work» Needs review
StatusFileSize
new6.66 KB
new15.45 KB
FAILED: [[SimpleTest]]: [MySQL] 53,920 pass(es), 6 fail(s), and 1,407 exception(s).
[ View ]

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

  1. The most controversial change: I removed template_preprocess_user_signature() and added the "clear" class in user-signature.html.twig instead. Any objections? @joelpittet seemed mostly okay with it :)
  2. Rolled back #1843788: Remove template_process_username() (added in #22), this should bring down the failures and exceptions slightly. From what I can tell, RDF is using the process function, and that’s a bigger problem to solve so I think we should leave that out of this issue if we can.
  3. Removed the interim $variables['attributes'] mentioned in #27.
  4. Updated docs throughout.
  5. +++ b/core/modules/user/templates/user.html.twigundefined
    @@ -6,20 +6,20 @@
      *
    - * Use {{ content }} to print all content, or print a subset
    - * such as {{ content.field_example }}.
    + * Use 'content' to print all content, or print a subset
    + *  such as 'content.field_example'.
      * By default, $user_profile['summary'] is provided, which contains data on the
    - * user's history. Other data can be included by modules.
    + *  user's history. Other data can be included by modules.
      *
      * Available variables:
      *   - content: A list of content items.
      *   - Field variables: for each field instance attached to the user a
    - *     corresponding variable is defined; e.g., account.field_example has a
    - *     variable 'field_example' defined. When needing to access a field's raw
    - *     values, developers/themers are strongly encouraged to use these
    - *     variables. Otherwise they will have to explicitly specify the desired
    - *     field language, e.g. account.field_example['en'], thus overriding any
    - *     language negotiation rule that was previously applied.
    + *      corresponding variable is defined; e.g., account.field_example has a
    + *      variable 'field_example' defined. When needing to access a field's raw
    + *      values, developers/themers are strongly encouraged to use these
    + *      variables. Otherwise they will have to explicitly specify the desired
    + *      field language, e.g. account.field_example['en'], thus overriding any
    + *      language negotiation rule that was previously applied.

    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?

StatusFileSize
new16.09 KB
FAILED: [[SimpleTest]]: [MySQL] 53,841 pass(es), 13 fail(s), and 1,535 exception(s).
[ View ]

Erg, forgot the reconstructed patch from #25.

Status:Needs review» Needs work

The last submitted patch, 1898468-25-reconstructed.patch, failed testing.

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

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

@Cottser, that looks like a good deal of the problem, at least at first glance!

Assigned:Unassigned» joelpittet
Status:Needs work» Needs review
StatusFileSize
new17.08 KB
FAILED: [[SimpleTest]]: [MySQL] 54,031 pass(es), 9 fail(s), and 0 exception(s).
[ View ]
new2.48 KB

This should get rid of a majority of the exceptions and at least one fail:) I will await the results to plug further into this.

Status:Needs review» Needs work

The last submitted patch, twig-user-1898468-35.patch, failed testing.

Issue summary:View changes

Updated issue summary.

Assigned:joelpittet» Unassigned

Not sure how to fix those RDF errors. From debugging the attribute arrays are nesting to deep.

Anybody is welcome to pick this up.

Issue tags:+Needs manual testing

Tagging.

Assigned:Unassigned» Cottser

Will 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):

+++ b/core/modules/user/user.moduleundefined
@@ -728,9 +731,17 @@ function user_template_preprocess_default_variables_alter(&$variables) {
+ *   - account: The user account to check access for
+ *     (Drupal\user\Plugin\Core\Entity\User).

We're not checking access here as far as I know.

Assigned:Cottser» Unassigned

I haven't had a chance to look deeper into this, hopefully the information in #39 will be a good starting point for debugging.

Status:Needs work» Needs review
StatusFileSize
new16.75 KB
PASSED: [[SimpleTest]]: [MySQL] 54,425 pass(es).
[ View ]

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

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

    $attributes = (array) $variables['attributes'];

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

StatusFileSize
new1.42 KB

forgot the interdiff for #41.

Status:Needs review» Reviewed & tested by the community

Looks good to me.

Status:Reviewed & tested by the community» Needs review

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

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

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

Status:Needs review» Needs work

hmm, 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?

+++ b/core/modules/user/user.moduleundefined
@@ -808,42 +819,16 @@ function template_process_username(&$variables) {
+    $variables['link'] = array(
+      '#theme' => 'link',
+      '#text' => $variables['name'] . $variables['extra'],
+      '#path' => $variables['link_path'],
+      '#options' => $variables['link_options'],
+    );
...
-    $output = l($variables['name'] . $variables['extra'], $variables['link_path'], $variables['link_options']);

@thedavidmeister - so it sounds like for now we should use l() instead of '#theme' => 'link' in this patch.

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

Assigned:Unassigned» Cottser

I'll revise this patch tonight, thanks @thedavidmeister!

Status:Needs work» Needs review
StatusFileSize
new4.66 KB
new16.59 KB
PASSED: [[SimpleTest]]: [MySQL] 54,446 pass(es).
[ View ]

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

Status:Needs review» Needs work

Issue summary:View changes

Add conversion summary table

Assigned:Cottser» Unassigned
Status:Needs work» Needs review

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

Issue summary:View changes

Add testing steps

Issue summary:View changes

rm sand

Issue summary:View changes

remove theme_user_sig

Assigned:Unassigned» jenlampton

reviewing

Status:Needs review» Needs work
Issue tags:-Needs manual testing

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

View developer output like variable printouts, query log, etc. <em class="permission-warning">Warning: Give to trusted roles only; this permission has security implications.</em>

after:
View developer output like variable printouts, query log, etc.      <em class="permission-warning">Warning: Give to trusted roles only; this permission has security implications.</em>

username.html.twig
before:

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

after:
<a href="/user/1" rel="author" class="username" lang="" about="/user/1" typeof="sioc:UserAccount" property="foaf:name" datatype="">root</a>

user.html.twig
before:

<article class="profile" class="user" typeof="sioc:UserAccount" about="/user/1">
  <div class="form-item form-type-item">
<label>Member for</label> 10 hours 32 min
</div>
</article>

after:
<article class="profile" typeof="sioc:UserAccount" about="/user/1">
  <div class="form-item form-type-item">
<label>Member for</label> 10 hours 29 min
</div>
</article>

Assigned:jenlampton» Unassigned
Status:Needs work» Needs review
StatusFileSize
new3.12 KB
new16.2 KB
PASSED: [[SimpleTest]]: [MySQL] 54,438 pass(es).
[ View ]

Okay, three minor things from above fixed. Needs another review!

Issue summary:View changes

remove

Thanks @jenlampton! Updated the testing steps to remove the user signature testing.

This is in the interdiff of #56:

+++ b/core/modules/user/user.module
@@ -782,8 +778,8 @@ function template_preprocess_username(&$variables) {
-    $variables['link_attributes']['title'] = t('View user profile.');
     $variables['link_path'] = 'user/' . $variables['uid'];
+    $variables['link_attributes'] = array('title' => t('View user profile.'));

This will wipe out any other key from the 'link_attributes' array element. Is this change intentional?

Assigned:Unassigned» jenlampton
StatusFileSize
new15.66 KB
PASSED: [[SimpleTest]]: [MySQL] 54,477 pass(es).
[ View ]

That was completely unintentional. good catch scor! rerolled.

Assigned:jenlampton» Unassigned

whoops, cant review my own patch :)

Assigned:Unassigned» c4rl
Status:Needs review» Needs work

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

Assigned:c4rl» Unassigned
Status:Needs work» Needs review
StatusFileSize
new5.55 KB
new16.9 KB
PASSED: [[SimpleTest]]: [MySQL] 54,465 pass(es).
[ View ]

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

Issue summary:View changes

Update testing steps

this may not apply to head anymore. retesting.

Assigned:Unassigned» jenlampton

#62: drupal-1898468-62.patch queued for re-testing.

+          '#description' => $hide_descriptions ? '' : $perm_item['description']
+            . (!empty($perm_item['restrict access']) ? (!empty($perm_item['description']) ? ' ' : '')
+            . '<em class="permission-warning">' . t('Warning: Give to trusted roles only; this permission has security implications.') . '</em>' : ''),

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

<?php
  $warning
= t('Warning: Give to trusted roles only; this permission has security implications.');
 
$item_description = $perm_item['description'];
 
$item_description .= !empty($perm_item['restrict access']) ? ' <em class="permission-warning">' . $warning . '</em>' : '';
 
$form['permission'][$perm]['#description'] = !$hide_descriptions ? trim($item_description) : '';
?>

Or doing it with a renderable array:

<?php
 
// Prepare description markup if required.
 
if(!$hide_descriptions) {
    if(!empty(
$perm_item['restrict access'])) {
     
$warning = array(
       
'#theme' => 'html_tag',
       
'#tag' => 'em',
       
'#attributes' => array('class' => 'permission-warning'),
       
'#value' => t('Warning: Give to trusted roles only; this permission has security implications.'),
      );
     
$perm_item['description'] .= ' ' . drupal_render($warning);
     
// If description was blank we need to trim now.
     
$perm_item['description'] = trim($perm_item['description']);
    }
  }
  else {
   
$perm_item['description'] = '';
  }
 
$form['permission'][$perm] = array(
   
'#type' => 'item',
   
'#markup' => $perm_item['title'],
   
'#description' => $perm_item['description'],
  );
?>

StatusFileSize
new1.42 KB
new17.08 KB
PASSED: [[SimpleTest]]: [MySQL] 55,398 pass(es).
[ View ]

Made changes for legibility as indicated by #65.

Interdiff with respect to #62.

Assigned:jenlampton» Unassigned

Title:Convert user module to Twiguser.module - Convert PHPTemplate templates to Twig
Status:Needs review» Needs work

Per #1757550-44: [META-63] 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 Assigned to: Cottser for theme_ function conversion.

Issue summary:View changes

Updated issue summary.

Assigned:Unassigned» shanethehat

Assigned:shanethehat» Unassigned
Status:Needs work» Needs review
StatusFileSize
new7.07 KB
PASSED: [[SimpleTest]]: [MySQL] 55,901 pass(es).
[ View ]

Assigned:Unassigned» steveoliver

Thanks @shanethehat, off to @steveoliver for another review!

StatusFileSize
new5.3 KB
PASSED: [[SimpleTest]]: [MySQL] 55,821 pass(es).
[ View ]
new1.77 KB

Just moving the rdf.module changes over to #1987418: user.module - Convert theme_ functions to Twig for now.

StatusFileSize
new780 bytes
new5.03 KB
PASSED: [[SimpleTest]]: [MySQL] 55,846 pass(es).
[ View ]

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

+++ b/core/modules/user/user.module
@@ -93,8 +93,8 @@ function user_theme() {
     'user' => array(
       'render element' => 'elements',
-      'template' => 'user',
       'file' => 'user.pages.inc',
+      'template' => 'user',

How about adding 'attributes' => array() here?

+++ b/core/modules/user/user.pages.inc
@@ -196,6 +198,9 @@ function template_preprocess_user(&$variables) {
+
+  // Set up attributes.
+  $variables['attributes'] = array('class' => array('profile'));

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

Assigned:steveoliver» Unassigned
Status:Needs review» Needs work
Issue tags:+Novice

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

Status:Needs work» Needs review
StatusFileSize
new633 bytes
new5.02 KB
PASSED: [[SimpleTest]]: [MySQL] 55,812 pass(es).
[ View ]
new5.01 KB
PASSED: [[SimpleTest]]: [MySQL] 55,689 pass(es).
[ View ]

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

<?php
$variables
['attributes']['class'] = array('profile');
?>

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.

Issue tags:-Novice

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

Issue tags:+Needs profiling

Tagging for profiling.

Assigned:Unassigned» steveoliver

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

#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!

Status:Needs review» Needs work

Looks good. Only needs docs tweaks and profiling.

+++ b/core/modules/user/templates/user.html.twig
@@ -0,0 +1,30 @@
+ * Available variables:
+ * - content: A list of content items. Use 'content' to print all content, or
+ *   print a subset such as 'content.field_example'.
+ * - Field variables: For each field instance attached to the user a
+ *   corresponding variable is defined; e.g., account.field_example has a
+ *   variable 'field_example' defined. When needing to access a field's raw
+ *   values, developers/themers are strongly encouraged to use these
+ *   variables. Otherwise they will have to explicitly specify the desired
+ *   field language, e.g. account.field_example.en, thus overriding any
+ *   language negotiation rule that was previously applied.
+ *
+ * @see template_preprocess()
+ * @see template_preprocess_user()
+ *
+ * @ingroup themeable
+ */
+#}
+<article{{ attributes }}>
+  {% if content is not empty %}
+    {{- content -}}
+  {% endif %}
+</article>

I know they weren't documented in the .tpl.php, but maybe we add a line for " - attributes: HTML attributes for the container element."

+++ b/core/modules/user/user.pages.inc
@@ -179,12 +179,14 @@ function user_logout() {
+ *   - account: The user account to check access for
+ *     (Drupal\user\Plugin\Core\Entity\User)

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.

Status:Needs work» Needs review
StatusFileSize
new1.25 KB
new5.01 KB
PASSED: [[SimpleTest]]: [MySQL] 56,061 pass(es).
[ View ]

Here is a reroll with the changes proposed in #82.

Issue tags:-Needs profiling

stats on #83 vs. 8.x with a view of 50 users:

=== baseline-8.x..8.x compared (5196646858cb6..5196690530c90):
ct  : 64,277|64,276|-1|-0.0%
wt  : 726,983|1,542,461|815,478|112.2%
cpu : 720,045|724,045|4,000|0.6%
mu  : 56,900,608|56,900,808|200|0.0%
pmu : 57,024,096|57,024,592|496|0.0%
=== baseline-8.x..8.x-1898468-83 compared (5196646858cb6..519669936c893):
ct  : 64,277|65,602|1,325|2.1%
wt  : 726,983|1,585,410|858,427|118.1%
cpu : 720,045|744,046|24,001|3.3%
mu  : 56,900,608|58,670,024|1,769,416|3.1%
pmu : 57,024,096|58,799,376|1,775,280|3.1%

So we have a ~3.25% performance penalty with Twig here.

Status:Needs review» Needs work

+  {% if content is not empty %}
+    {{- content -}}
+  {% endif %}

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

Issue tags:+Needs profiling

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

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

Assigned:steveoliver» Unassigned

One more. Same test (clarification: 51 users, not 50 ;)). Baseline is stable this time:

=== 50-user-view..8.x compared (51972fb0786ef..519730b884300):
ct  : 65,830|65,830|0|0.0%
wt  : 9,291,522|9,292,366|844|0.0%
cpu : 736,045|732,046|-3,999|-0.5%
mu  : 56,948,528|56,948,528|0|0.0%
pmu : 57,074,504|57,074,504|0|0.0%
=== 50-user-view..8.x-1898468-83 compared (51972fb0786ef..5197310fa6a63):
ct  : 65,830|68,104|2,274|3.5%
wt  : 9,291,522|9,568,296|276,774|3.0%
cpu : 736,045|760,048|24,003|3.3%
mu  : 56,948,528|58,714,896|1,766,368|3.1%
pmu : 57,074,504|58,843,992|1,769,488|3.1%

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.

Assigned:Unassigned» Cottser

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

Assigned:Cottser» Unassigned
Status:Needs work» Needs review
StatusFileSize
new384 bytes
new5 KB
PASSED: [[SimpleTest]]: [MySQL] 55,667 pass(es).
[ View ]

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

=== 8.x..8.x compared (51978cc86c728..51978d2b1f3d1):
ct  : 33,570|33,570|0|0.0%
wt  : 345,567|345,166|-401|-0.1%
cpu : 315,116|314,178|-938|-0.3%
mu  : 30,324,000|30,324,000|0|0.0%
pmu : 30,433,448|30,433,448|0|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51978cc86c728&...

=== 8.x..user-1898468-91 compared (51978cc86c728..51978d4d92dd9):
ct  : 33,570|33,703|133|0.4%
wt  : 345,567|345,816|249|0.1%
cpu : 315,116|314,995|-121|-0.0%
mu  : 30,324,000|30,345,720|21,720|0.1%
pmu : 30,433,448|30,456,824|23,376|0.1%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51978cc86c728&...

So performance is definitely comparable here. @steveoliver, RTBC?

Status:Needs review» Reviewed & tested by the community

Ah, but of course! Thanks Cottser. Definitely RTBC.

Issue tags:-Needs profiling

Oops, meant to do that. Thanks @thedavidmeister!

Title:user.module - Convert PHPTemplate templates to Twig[READY] user.module - Convert PHPTemplate templates to Twig
Status:Reviewed & tested by the community» Closed (duplicate)

Issue summary:View changes

Revise summary

Title:[READY] user.module - Convert PHPTemplate templates to Twiguser.module - Convert PHPTemplate templates to Twig
Status:Closed (duplicate)» Fixed

Committed dcfd4de and pushed to 8.x. Thanks!

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

Issue summary:View changes

Add commit message