Task

Use Twig instead of PHPTemplate

Remaining

  • Replace all theme functions and templates with .html.twig equivalent templates
  • Add new preprocess functions for the .html.twig equivalent templates if nessasary
  • Update all hook_theme definitions

Steps to reproduce

  • Enable tracker module.
  • Generate some nodes
  • Create a 'test' user and visit half the nodes with that user.
  • Update those nodes as admin.
  • Visit /tracker/ to see mark template in action.

Comments

larowlan’s picture

Assigned: Unassigned » larowlan

the last one was too easy

larowlan’s picture

Assigned: larowlan » Unassigned
FileSize
1.48 KB
PASSED: [[SimpleTest]]: [MySQL] 53,061 pass(es). View

You guys have made this too easy, surely I'm missing something?

joelpittet’s picture

Status: Active » Needs review

Haven't seen how constants work in twig... my guess is they don't. Changing to needs review to get testbot's answer.

joelpittet’s picture

Status: Needs review » Needs work

Well I am surprised that passed the tests, maybe it needs more tests?

I did a manual test to see what {{ MARK_UPDATED }} gives me which was zip followed by the usual
MARK_UPDATED could not be found in _context

Cottser’s picture

Cottser’s picture

Although if it's not a class constant it looks like maybe you don't need to use that syntax, not sure.

There are tests for markers in \Drupal\system\Tests\Form\ElementsLabelsTest.

So I think this is just legitimately working :)

Edit: Could be there are no tests, haven't looked thoroughly.

joelpittet’s picture

Just did a search, yeah there are no tests. @larowlan Could you possible write some simple tests to see if the output is what we are looking for?

Also, could you pass those constants through the preprocess to the twig file so they are available. I talked briefly with FabianX about this this morning and that may be the quickest solution for now.

larowlan’s picture

Assigned: Unassigned » larowlan

Sure

larowlan’s picture

Hang on, I'm certain there are tests for this in the CommentInterface tests (after spending hours debugging them for #731724: Convert comment settings into a field to make them work with CMI and non-node entities).
Going for a look-see.

larowlan’s picture

Ah but they run in standard profile which uses Bartik...
So we need some new tests that use stark.

larowlan’s picture

Status: Needs work » Needs review
FileSize
1.86 KB
PASSED: [[SimpleTest]]: [MySQL] 53,132 pass(es). View
2.24 KB
FAILED: [[SimpleTest]]: [MySQL] 53,117 pass(es), 4 fail(s), and 0 exception(s). View
1.17 KB

So, turns out - there are tests in tracker module for the new functionality - and these use Stark.
But the twig patch above didn't include the required drupal_common_theme() update to reference the template, so kept using the theme_mark() function, hence it passed.

Please find attached an interdiff from 2, a patch that includes the template entry in drupal_common_theme (and will fail because of the constants in the twig file) and a pass patch that introduces the constants as variables in the twig template preprocessing.

larowlan’s picture

Assigned: larowlan » Unassigned
Cottser’s picture

Status: Needs review » Needs work

Thanks @larowlan! We'll need to remove theme_mark(), and the preprocess docs could be updated to meet #1913208: [policy] Standardize template preprocess function documentation.

larowlan’s picture

Status: Needs work » Needs review
FileSize
2.18 KB
PASSED: [[SimpleTest]]: [MySQL] 53,106 pass(es). View
997 bytes

Thanks, I wasn't aware that all core themes used Twig, but if we're removing theme_mark() then I guess they are.
Also updated preprocess docblock.

joelpittet’s picture

FileSize
936 bytes
3.57 KB
PASSED: [[SimpleTest]]: [MySQL] 53,125 pass(es). View

Small cleanup, may still need some tests, not sure thoughts? Otherwise RTBC?

larowlan’s picture

Status: Needs review » Needs work

There are tests in the tracker module.

+++ b/core/modules/system/tests/modules/common_test/common_test.moduleundefined
+++ b/core/modules/system/tests/modules/common_test/common_test.moduleundefined
@@ -247,18 +247,12 @@ function common_test_theme() {

@@ -247,18 +247,12 @@ function common_test_theme() {
   return array(
     'common_test_foo' => array(
       'variables' => array('foo' => 'foo', 'bar' => 'bar'),
+      'template' => 'common-test-foo',
     ),
   );
 }
 
 /**
- * Provides a theme function for drupal_render().
- */
-function theme_common_test_foo($variables) {
-  return $variables['foo'] . $variables['bar'];
-}
-
-/**
  * Implements hook_library_info_alter().
  */
 function common_test_library_info_alter(&$libraries, $module) {
diff --git a/core/modules/system/tests/modules/common_test/templates/common-test-foo.html.twig b/core/modules/system/tests/modules/common_test/templates/common-test-foo.html.twig

diff --git a/core/modules/system/tests/modules/common_test/templates/common-test-foo.html.twig b/core/modules/system/tests/modules/common_test/templates/common-test-foo.html.twig
new file mode 100644
index 0000000..6f9b471

index 0000000..6f9b471
--- /dev/null

--- /dev/null
+++ b/core/modules/system/tests/modules/common_test/templates/common-test-foo.html.twigundefined

+++ b/core/modules/system/tests/modules/common_test/templates/common-test-foo.html.twigundefined
+++ b/core/modules/system/tests/modules/common_test/templates/common-test-foo.html.twigundefined
@@ -0,0 +1,15 @@

@@ -0,0 +1,15 @@
+{#
+/**
+ * @file
+ * Default theme implementation for the common test foo.
+ *
+ * Available variables:
+ * - foo: foo.
+ * - bar: bar.
+ *
+ * @see template_preprocess()
+ *
+ * @ingroup themeable
+ */
+#}

I assume this wasn't supposed to be here :)

joelpittet’s picture

Status: Needs work » Needs review
FileSize
2.21 KB
PASSED: [[SimpleTest]]: [MySQL] 53,062 pass(es). View
919 bytes

Hmm... let's try that again shall we. I'll get my workflow down eventually:S

larowlan’s picture

This looks good to go to me but I don't think I'm eligible to RTBC it.

tlattimore’s picture

+++ b/core/includes/theme.incundefined
@@ -2273,24 +2273,23 @@ function theme_tablesort_indicator($variables) {
+  $variables['MARK_NEW'] = MARK_NEW;
+  $variables['MARK_UPDATED'] = MARK_UPDATED;

Is there a reason that these array keys are all caps? According to the coding standards all variable names should be lower case. http://drupal.org/coding-standards#naming

Cottser’s picture

FileSize
1.66 KB
2.2 KB
PASSED: [[SimpleTest]]: [MySQL] 54,265 pass(es). View

After doing some testing while working on #1898054: comment.module - Convert PHPTemplate templates to Twig, Twig's constant function works quite well and I think we should embrace it:
http://twig.sensiolabs.org/doc/functions/constant.html

This passes tracker tests locally.

Also tweaked the docs a bit.

thedavidmeister’s picture

Status: Needs review » Reviewed & tested by the community

I can't see anything wrong with this.

"updated" test in TrackerTest.php is in testTrackerCronIndexing().

"new" test in TrackerTest.php is in testTrackerNewNodes().

thedavidmeister’s picture

#20: 1939092-20.patch queued for re-testing.

Cottser’s picture

FileSize
997 bytes
2.2 KB
PASSED: [[SimpleTest]]: [MySQL] 54,240 pass(es). View

Thanks @thedavidmeister!

Two minors tweaks, leaving at RTBC:

  1. Data type cleanup in the template per http://drupal.org/node/1823416#datatypes.
  2. Added serial comma per http://drupal.org/coding-standards#array.
Cottser’s picture

FileSize
2.22 KB
PASSED: [[SimpleTest]]: [MySQL] 54,506 pass(es). View

I messed up, the actual patch from #28 doesn't include the changes from the interdiff. I was experimenting with http://hojtsy.hu/blog/2012-apr-13/quick-and-simple-interdiffs and missed a step. Here's the correct patch with docs tweak (patch + interdiff).

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs profiling
joelpittet’s picture

Issue tags: +Needs reroll

Needs a re-roll, patch #24 doesn't apply any longer.

pwieck’s picture

Assigned: Unassigned » pwieck

Re-rolling #24 now

pwieck’s picture

Status: Needs work » Needs review
FileSize
2.37 KB
2.33 KB
FAILED: [[SimpleTest]]: [MySQL] 57,890 pass(es), 3 fail(s), and 0 exception(s). View

re-rolled.

thedavidmeister’s picture

Status: Needs review » Needs work

This re-roll seems to be a regression on #2010672: Rename 'type' variable of theme_mark to 'status'

pwieck’s picture

I will fix re-roll, this should not be in there -> function theme_mark($variables) {

Only interdiff file is wrong. Patch is correct

Thanks @thedavidmeister

thedavidmeister’s picture

-      'variables' => array('mark_type' => MARK_NEW),
+      'variables' => array('type' => MARK_NEW),

is a problem in that patch.

pwieck’s picture

Status: Needs work » Needs review

@thedavidmeister The patch is right the interdiff.txt file is wrong for some reason. Good for review just disregard interdiff.txt

thedavidmeister’s picture

Status: Needs review » Needs work

the patch in 28 is wrong because of

-      'variables' => array('mark_type' => MARK_NEW),
+      'variables' => array('type' => MARK_NEW),
pwieck’s picture

Sorry I see now. I thought this was my error. This was in the original patch. So it should be

This
'variables' => array('mark_type' => MARK_NEW),

Not this
'variables' => array('type' => MARK_NEW),

Everything else is correct in patch?

pwieck’s picture

Status: Needs work » Needs review
FileSize
2.24 KB
FAILED: [[SimpleTest]]: [MySQL] 57,883 pass(es), 3 fail(s), and 0 exception(s). View

@thedavidmeister,

Changed:
'variables' => array('type' => MARK_NEW),

To:
'variables' => array('mark_type' => MARK_NEW),

Status: Needs review » Needs work

The last submitted patch, 1939092-35.patch, failed testing.

pwieck’s picture

Assigned: pwieck » Unassigned

Kicking this back to the coders. I'm am just a re-roll monkey. Doesn't pass test.

Could it be this area in patch?

-function theme_mark($variables) {
-  $type = $variables['mark_type'];
+function template_preprocess_mark(&$variables) {
   global $user;
+  $variables['logged_in'] = FALSE;
   if ($user->uid) {
-    if ($type == MARK_NEW) {
-      return ' <span class="marker">' . t('new') . '</span>';
-    }
-    elseif ($type == MARK_UPDATED) {
-      return ' <span class="marker">' . t('updated') . '</span>';
-    }
+    $variables['logged_in'] = TRUE;
   }
 }
thedavidmeister’s picture

yeah, basically anything that was "type" should probably be "mark_type" now

pwieck’s picture

I'm a novice and trying to understand this part of the code.

How can starting with this:

function theme_mark($variables) {
  $type = $variables['mark_type'];
  global $user;
  if ($user->uid) {
    if ($type == MARK_NEW) {
      return ' <span class="marker">' . t('new') . '</span>';
    }
    elseif ($type == MARK_UPDATED) {
      return ' <span class="marker">' . t('updated') . '</span>';
    }
  }
}

Then applying patch:

-function theme_mark($variables) {
-  $type = $variables['mark_type'];
+function template_preprocess_mark(&$variables) {
   global $user;
+  $variables['logged_in'] = FALSE;
   if ($user->uid) {
-    if ($type == MARK_NEW) {
-      return ' <span class="marker">' . t('new') . '</span>';
-    }
-    elseif ($type == MARK_UPDATED) {
-      return ' <span class="marker">' . t('updated') . '</span>';
-    }
+    $variables['logged_in'] = TRUE;
   }
 }

Suppose to work and ending up like this:

  function template_preprocess_mark(&$variables) {
   global $user;
   $variables['logged_in'] = FALSE;
   if ($user->uid) {
     $variables['logged_in'] = TRUE;
   }
 }

I don't see where the styling is anymore

thedavidmeister’s picture

It is in the Twig template:

+{#
+/**
+ * @file
+ * Default theme implementation for a marker for new or updated content.
+ *
+ * Available variables:
+ * - type: Number representing the marker type to display. Use the constants
+ *   below for comparison:
+ *   - MARK_NEW
+ *   - MARK_UPDATED
+ *   - MARK_READ
+ * - logged_in: A flag indicating whether the user is logged in or not.
+ *
+ * @see template_preprocess()
+ * @see template_preprocess_mark()
+ *
+ * @ingroup themeable
+ */
+#}
+{% if logged_in %}
+  {% if type is constant('MARK_NEW') %}
+    <span class="marker">{{ 'new'|t }}</span>
+  {% elseif type is constant('MARK_UPDATED') %}
+    <span class="marker">{{ 'updated'|t }}</span>
+  {% endif %}
+{% endif %}
pwieck’s picture

Duh? I seen that too. Been re-rolling all day and into the night, guess I should sleep. I am not experienced enough to diagnose the patch failure.

Thx @thedavidmeister for taking the time to help me.

thedavidmeister’s picture

probably failing because the Twig template is still referencing 'type' (in the code and the docs) instead of 'mark_type'.

Cottser’s picture

Issue tags: -Needs reroll

Yep, agreed with #42. Thanks for all your work on this @pwieck :D

azinoman’s picture

Assigned: Unassigned » azinoman

dibs

azinoman’s picture

dibs

pwieck’s picture

Need to remove @see template_preprocess()

+ *
+ * @see template_preprocess()
+ * @see template_preprocess_mark()
+ *

[policy adopted, patch needed] Stop saying '@see template_preprocess()' in every twig file

azinoman’s picture

Assigned: azinoman » Unassigned
Status: Needs work » Needs review
FileSize
2.33 KB
FAILED: [[SimpleTest]]: [MySQL] 56,729 pass(es), 3 fail(s), and 0 exception(s). View

I changed the initial mark_type to just type under the instruction of Jen Lampton. I think that should fix everything now. Changing to needs review.

Status: Needs review » Needs work

The last submitted patch, twig-1939092-46.patch, failed testing.

thedavidmeister’s picture

I changed the initial mark_type to just type

Please don't do this.

If you do this:

$mark = array(
  '#theme' => 'mark',
  '#type' => 'foo',
);
drupal_render($mark);

Then drupal_render() thinks you're trying to render a 'foo' element and pulls up those defaults from element_info(). theme('mark') is then in appropriately using the #type attribute to render itself not based on the element type at all.

What's worse is that using #type like this means it is impossible for us to ever implement #type 'mark' in the future.

I don't mind if you want to rename the variable to something other than 'mark_type' but 'type' is not an option.

I just opened #2024743: Document "reserved variables" for renderables to avoid conflicts with drupal_render(), the FAPI, and _theme() so hopefully conflicts like this will be more obvious in the future.

pwieck’s picture

@azinoman - Please see #46, @see template_preprocess() is still in patch and needs to be removed.

jenlampton’s picture

Status: Needs work » Needs review
FileSize
1.67 KB
2.31 KB
FAILED: [[SimpleTest]]: [MySQL] 58,119 pass(es), 3 fail(s), and 0 exception(s). View

Sorry about that. Here I've changed 'type' to 'status' which is a better indication of what the heck a 'mark' is anyway.
Also removed that @see

Status: Needs review » Needs work

The last submitted patch, twig-convert_mark-1939092-51.patch, failed testing.

Cottser’s picture

We just changed it to mark_type in #2010672: Rename 'type' variable of theme_mark to 'status' and there's a pending change notice for that. So I am okay with changing it again but it should happen over there IMO.

jenlampton’s picture

Status: Needs work » Postponed

postponing this then, on https://drupal.org/node/2010672

jenlampton’s picture

Issue tags: +Needs reroll

Also, will need a reroll after that gets in. So, tagging.

jenlampton’s picture

Status: Postponed » Needs review
FileSize
2.29 KB
PASSED: [[SimpleTest]]: [MySQL] 56,356 pass(es). View

unpostponed! :) woot.

edit: whoops, forgot to remove reroll tag, plz do so when reviewing :)

Cottser’s picture

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

Hmm, looking at this again - why do we set the logged_in variable in preprocess? It's a default template variable that should be getting set in _template_preprocess_default_variables!

This means:

  • We don't need a preprocess function at all!
  • We can probably just remove the line documenting the logged_in variable. This is a default variable available to all templates.
jenlampton’s picture

Status: Needs work » Needs review
FileSize
1.38 KB
45.05 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch twig-convert_mark-1939092-58.patch. Unable to apply patch. See the log in the details link for more information. View

LOVE IT :)
removed the preprocess function and the docs lines about the variable, and also @see

Status: Needs review » Needs work

The last submitted patch, twig-convert_mark-1939092-58.patch, failed testing.

jenlampton’s picture

Status: Needs work » Needs review
FileSize
1.38 KB
1.96 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch twig-convert_mark-1939092-60.patch. Unable to apply patch. See the log in the details link for more information. View

trying again. sorry test bot!

thedavidmeister’s picture

Issue tags: +Novice, +Needs manual testing

This needs manual testing as well, I can't see that it has been done anywhere in this thread.

The manual testing is a novice task.

joelpittet’s picture

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

I think this got lost, so I added it back to #1757550: [Meta] Convert core theme functions to Twig templates
Needs a re-roll

joelpittet’s picture

Status: Needs work » Needs review
Issue tags: -Novice, -Needs manual testing, -Needs profiling, -Twig, -Needs reroll

#60: twig-convert_mark-1939092-60.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Novice, +Needs manual testing, +Needs profiling, +Twig, +Needs reroll

The last submitted patch, twig-convert_mark-1939092-60.patch, failed testing.

dbazuin’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
0 bytes
PASSED: [[SimpleTest]]: [MySQL] 58,836 pass(es). View

I rerolled the patch

dbazuin’s picture

FileSize
1.97 KB
PASSED: [[SimpleTest]]: [MySQL] 58,637 pass(es). View

Novice mistake sorry.
This is the good patch

mikl’s picture

Assigned: Unassigned » mikl

I'll test this.

mikl’s picture

Assigned: mikl » Unassigned
Issue tags: -Needs manual testing

Verified that the template is actually used.

The best way to test this is the tracker page, where the mark template will be rendered for each node (regardless of its status). This will likely have mean a significant performance hit, but we'll need someone to run an actual profiling to see how bad it is.

adsw12’s picture

Issue summary: View changes

Removed link sandbox and added a new a related link

joelpittet’s picture

Status: Needs review » Postponed
joelpittet’s picture

Issue summary: View changes

Updated issue summary.

Cottser’s picture

markcarver’s picture

Status: Postponed » Needs review
Related issues: +#1311372: Use <mark> element for 'mark' theme hook

We shouldn't postpone the initial conversion on #1311372: Use <mark> element for 'mark' theme hook. Changing existing markup should happen afterwards.

markcarver’s picture

Status: Needs review » Needs work
Issue tags: +Needs issue summary update, +Needs reroll
+++ b/core/includes/theme.inc
@@ -3046,6 +3025,7 @@ function drupal_common_theme() {
+      'template' => 'mark',

Just an FYI (as a @todo) this will not be necessary once #2226207: Make 'template' the default output option for hook_theme() gets in.

I'd like to RTBC this as it's a pretty straight forwards conversion, but it needs to be re-rolled and also profiled.

markcarver’s picture

mgifford’s picture

Status: Needs work » Needs review
FileSize
1.99 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,332 pass(es). View

reroll

joelpittet’s picture

Issue tags: -Needs reroll

Thanks @mgifford, this now needs some manual testing.

andypost’s picture

and profiling... I think tracker page is a nice example

mgifford’s picture

@andypost do you mean /user/1/track

I'm not sure where to best find this in Core.

joelpittet’s picture

I double checked #68 and it seems to be rendering before and after the patch exactly the same.

For profiling you may need to build a contrived example because it could be tricky to run xhprofkit with a logged in user, our normal approach has been with anonymous.

Added steps to test in the issue summary.

markcarver’s picture

I'm inclined to say that this actually doesn't need profiling now that I really think about it. The conversions that really need it are things like tables, images, etc. This is a very, very simple patch/conversion (ie: they both ultimately are just a PHP function that spits out simple markup).

That being said, I just manually tested this in simplytest.me and it appears to be working on node comments correctly. I did notice that when my second user updated a comment it still said "new", not "updated". Not sure if this is because of this issue or a bug in how we handle these markers on the front-end now (ie: separate bug/issue).

I'm going to leave this as CNR and tag with manual testing so someone else can take a look at this in more depth (locally).

steinmb’s picture

Assigned: Unassigned » steinmb

Let me take it for a spin

steinmb’s picture

Status: Needs review » Reviewed & tested by the community

Short story it works. There is a slight change in the markup though we could perhaps address some in a followup issue where we polish, discuss and argue about markup.

Before, logged in user:

<td><a href="/node/4">foobar</a>  <span class="marker">updated</span></td>
<td><a href="/node/3">bar</a>  <span class="marker">new</span></td>

Before, logged out user:

<td><a href="/node/4">foobar</a> </td>

After twig conversion, logged in user:

<td><a href="/node/4">foobar</a>       <span class="marker">updated</span>
  </td>
<td><a href="/node/3">bar</a>       <span class="marker">new</span>
  </td>

Logged out user:

<td><a href="/node/4">foobar</a> </td>

markcarver’s picture

joelpittet’s picture

Issue tags: -Needs manual testing

Thanks for the manual testing, would you mind creating a follow up issue to maybe consider moving the logic to the preprocess or maybe remove this template and leave the domain logic up to the domain (node) if possible?

@Mark Carver has some ideas on how we can remove this template but I'd like that to move to follow up and just get this in and done.

markcarver’s picture

Agreed, RTBC. Went ahead and created the follow-up (which is postponed anyway).

Cottser’s picture

+1, looks good to me.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 74: twig-convert_mark-1939092-74.patch, failed testing.

joelpittet’s picture

Status: Needs work » Reviewed & tested by the community

random fail?

webchick’s picture

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

  • Commit 1d47e0a on 8.x by webchick:
    Issue #1939092 by mgifford, dbazuin, jenlampton, azinoman, pwieck,...

Status: Fixed » Closed (fixed)

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