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

CommentFileSizeAuthor
#91 1898468-91.patch5 KBstar-szr
#91 interdiff.txt384 bytesstar-szr
#83 1898468-83.patch5.01 KBidflood
#83 interdiff.txt1.25 KBidflood
#77 1898468-77.patch5.01 KBidflood
#77 1898468-77-alternate.patch5.02 KBidflood
#77 interdiff.txt633 bytesidflood
#74 1898468-74.patch5.03 KBstar-szr
#74 interdiff.txt780 bytesstar-szr
#73 interdiff.txt1.77 KBstar-szr
#73 1898468-73.patch5.3 KBstar-szr
#71 twig-user-template-only-1898468-71.patch7.07 KBshanethehat
#67 drupal-1898468-67.patch17.08 KBc4rl
#67 drupal-1898468-67.patch-interdiff.txt1.42 KBc4rl
#62 drupal-1898468-62.patch16.9 KBc4rl
#62 drupal-1898468-62.patch-interdiff.txt5.55 KBc4rl
#59 twig-convert_user-1898468-59.patch15.66 KBjenlampton
#56 twig-convert_user-1898468-56.patch16.2 KBjenlampton
#56 interdiff.txt3.12 KBjenlampton
#51 1898468-51.patch16.59 KBstar-szr
#51 interdiff.txt4.66 KBstar-szr
#42 interdiff.txt1.42 KBscor
#41 twig-user-1898468-41.patch16.75 KBscor
#35 interdiff.txt2.48 KBjoelpittet
#35 twig-user-1898468-35.patch17.08 KBjoelpittet
#30 1898468-25-reconstructed.patch16.09 KBstar-szr
#29 1898468-29.patch15.45 KBstar-szr
#29 interdiff.txt6.66 KBstar-szr
#25 interdiff.txt3.32 KBjoelpittet
#25 twig-user-1898468-25.patch28.23 KBjoelpittet
#22 twig-user-1898468-22.patch15.93 KBjoelpittet
#22 interdiff.txt1.6 KBjoelpittet
#18 interdiff.txt12.12 KBjoelpittet
#18 twig-user-1898468-18.patch15.35 KBjoelpittet
#14 twig-user-1898468-14.patch22.24 KBjoelpittet
#14 interdiff.txt9.38 KBjoelpittet
#6 drupal-1898468-6.patch20.35 KBc4rl
#4 drupal-1898468-4.patch20.34 KBc4rl
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

c4rl’s picture

Issue tags: +Twig

Tagging

trrroy’s picture

Assigned: Unassigned » trrroy
c4rl’s picture

Assigned: trrroy » c4rl

Assigning

c4rl’s picture

Status: Active » Needs review
FileSize
20.34 KB

Status: Needs review » Needs work

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

c4rl’s picture

Status: Needs work » Needs review
FileSize
20.35 KB

Rebased.

Status: Needs review » Needs work

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

rteijeiro’s picture

Assigned: c4rl » rteijeiro
Issue tags: +SprintWeekend2013

Assigning and tagging.

rteijeiro’s picture

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

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.

steveoliver’s picture

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.

rteijeiro’s picture

Assigned: Unassigned » rteijeiro

Thanks steveoliver,

I will continue the issue...

DamienMcKenna’s picture

Issue tags: +SprintWeekend2013

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

star-szr’s picture

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

joelpittet’s picture

Issue tags: -SprintWeekend2013
FileSize
9.38 KB
22.24 KB

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

joelpittet’s picture

Status: Needs work » Needs review

Deploy testbot!

Status: Needs review » Needs work

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

star-szr’s picture

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.

joelpittet’s picture

FileSize
15.35 KB
12.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?

joelpittet’s picture

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.

star-szr’s picture

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.

star-szr’s picture

Issue summary: View changes

Added type table issue

joelpittet’s picture

Assigned: Unassigned » joelpittet
FileSize
1.6 KB
15.93 KB

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

joelpittet’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

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

joelpittet’s picture

Status: Needs work » Needs review
FileSize
28.23 KB
3.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.

steveoliver’s picture

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.

star-szr’s picture

Assigned: joelpittet » star-szr

Temporarily assigning to look at the tests and #27.

star-szr’s picture

Assigned: star-szr » Unassigned
Status: Needs work » Needs review
FileSize
6.66 KB
15.45 KB

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?

star-szr’s picture

Erg, forgot the reconstructed patch from #25.

Status: Needs review » Needs work

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

joelpittet’s picture

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

star-szr’s picture

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.

joelpittet’s picture

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

joelpittet’s picture

Assigned: Unassigned » joelpittet
Status: Needs work » Needs review
FileSize
17.08 KB
2.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.

Anonymous’s picture

Issue summary: View changes

Updated issue summary.

joelpittet’s picture

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.

star-szr’s picture

Issue tags: +Needs manual testing

Tagging.

star-szr’s picture

Assigned: Unassigned » star-szr

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.

star-szr’s picture

Assigned: star-szr » 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.

scor’s picture

Status: Needs work » Needs review
FileSize
16.75 KB

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.

scor’s picture

FileSize
1.42 KB

forgot the interdiff for #41.

matthewmack’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me.

star-szr’s picture

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?

matthewmack’s picture

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

scor’s picture

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.

thedavidmeister’s picture

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?

star-szr’s picture

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

thedavidmeister’s picture

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

star-szr’s picture

Assigned: Unassigned » star-szr

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

star-szr’s picture

Status: Needs work » Needs review
FileSize
4.66 KB
16.59 KB

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.

star-szr’s picture

Status: Needs review » Needs work
star-szr’s picture

Issue summary: View changes

Add conversion summary table

star-szr’s picture

Assigned: star-szr » 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.

star-szr’s picture

Issue summary: View changes

Add testing steps

jenlampton’s picture

Issue summary: View changes

rm sand

jenlampton’s picture

Issue summary: View changes

remove theme_user_sig

jenlampton’s picture

Assigned: Unassigned » jenlampton

reviewing

jenlampton’s picture

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>
jenlampton’s picture

Assigned: jenlampton » Unassigned
Status: Needs work » Needs review
FileSize
3.12 KB
16.2 KB

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

jenlampton’s picture

Issue summary: View changes

remove

star-szr’s picture

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

scor’s picture

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?

jenlampton’s picture

Assigned: Unassigned » jenlampton
FileSize
15.66 KB

That was completely unintentional. good catch scor! rerolled.

jenlampton’s picture

Assigned: jenlampton » Unassigned

whoops, cant review my own patch :)

c4rl’s picture

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.

c4rl’s picture

Assigned: c4rl » Unassigned
Status: Needs work » Needs review
FileSize
5.55 KB
16.9 KB

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.

c4rl’s picture

Issue summary: View changes

Update testing steps

jenlampton’s picture

this may not apply to head anymore. retesting.

jenlampton’s picture

Assigned: Unassigned » jenlampton

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

thedavidmeister’s picture

+          '#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:

  $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) : '';
thedavidmeister’s picture

Or doing it with a renderable array:

  // 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'],
  );
c4rl’s picture

Made changes for legibility as indicated by #65.

Interdiff with respect to #62.

c4rl’s picture

Assigned: jenlampton » Unassigned
c4rl’s picture

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

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

c4rl’s picture

Issue summary: View changes

Updated issue summary.

shanethehat’s picture

Assigned: Unassigned » shanethehat
shanethehat’s picture

Assigned: shanethehat » Unassigned
Status: Needs work » Needs review
FileSize
7.07 KB
star-szr’s picture

Assigned: Unassigned » steveoliver

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

star-szr’s picture

FileSize
5.3 KB
1.77 KB

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

star-szr’s picture

FileSize
780 bytes
5.03 KB

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.

steveoliver’s picture

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

star-szr’s picture

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.

idflood’s picture

Status: Needs work » Needs review
FileSize
633 bytes
5.02 KB
5.01 KB

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:

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

star-szr’s picture

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.

star-szr’s picture

Issue tags: +needs profiling

Tagging for profiling.

star-szr’s picture

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.

star-szr’s picture

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

steveoliver’s picture

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.

idflood’s picture

Status: Needs work » Needs review
FileSize
1.25 KB
5.01 KB

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

steveoliver’s picture

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.

thedavidmeister’s picture

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.

thedavidmeister’s picture

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?

steveoliver’s picture

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.

steveoliver’s picture

steveoliver’s picture

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.

star-szr’s picture

Assigned: Unassigned » star-szr

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.

star-szr’s picture

Assigned: star-szr » Unassigned
Status: Needs work » Needs review
FileSize
384 bytes
5 KB

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?

steveoliver’s picture

Status: Needs review » Reviewed & tested by the community

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

thedavidmeister’s picture

Issue tags: -needs profiling
star-szr’s picture

Oops, meant to do that. Thanks @thedavidmeister!

alexpott’s picture

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

Issue summary: View changes

Revise summary

alexpott’s picture

Title: [READY] user.module - Convert PHPTemplate templates to Twig » user.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.

Anonymous’s picture

Issue summary: View changes

Add commit message