Convert theme_mark() to Twig

the last one was too easy

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

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

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

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.

Sure

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.

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

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.

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.

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

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

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

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

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

I can't see anything wrong with this.

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

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

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

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

Re-rolling #24 now

re-rolled.

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

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

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

is a problem in that patch.

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

the patch in 28 is wrong because of

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

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?

@thedavidmeister,

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

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

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;
   }
 }

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

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

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 %}

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.

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

dibs

dibs

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

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.

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.

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

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

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

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

unpostponed! :) woot.

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

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

trying again. sorry test bot!

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.

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

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

I rerolled the patch

Novice mistake sorry.
This is the good patch

I'll test this.

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.

Removed link sandbox and added a new a related link

postponing on #1311372: Use <mark> element for 'mark' theme hook

Updated issue summary.

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

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

reroll

Thanks @mgifford, this now needs some manual testing.

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

@andypost do you mean /user/1/track

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

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.

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

Let me take it for a spin

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>

74: twig-convert_mark-1939092-74.patch queued for re-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.

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

random fail?

74: twig-convert_mark-1939092-74.patch queued for re-testing.

Committed and pushed to 8.x. Thanks!