filter.module - Convert theme_ functions to Twig

Attached is a patch

Thanks for all the patches @disasm :)

I applied this one locally and the test failures are legitimate. Didn't poke around too much yet, but this looks like part of the problem:

Twig_Error_Syntax: The function "theme" does not exist in "core/modules/filter/templates/filter-guidelines.html.twig"

The line in filter-guidelines.html.twig:
{{ theme('filter-tips') }}

From what I can tell we need to create a render array for filter_tips in template_preprocess_filter_guidelines() and then print the render array in the template, see http://drupal.org/node/1920746#render.

Cleaned up a few places that were throwing errors, still going to need further attention.

Let's see how bad the bot chokes on at this point.

No matter the results of the automated test, there is still some weirdness. Haven't had a chance to get into the details of why these are happening.

The filter configuration screen as a second table:

Also the markup in the form widget has changed enough to where the javascripty magic to toggle the tips displayed based on the selection is no longer functional.

@kerasai nice go at this.

Couple things I noticed. Your template_preprocess functions need to have their $variables referenced
template_preprocess_filter_admin_format_filter_order(&$variables)

there should be no more $output .= left in your template_preprocess after you are done converting. The html tags should be moved to the twig template and the variables should be added to $variables['whatever'] so they can be output in the twig as {{ whatever }}

theme('table', ... should be converted to

// After - passing a render array to the template.
$variables['table'] = array(
  '#theme' => 'table',
  '#header' => $header,
  '#rows' => $rows,
);

@see http://drupal.org/node/1920746

Otherwise nice first go at it. That should help with those screenshots a bit...

Thanks again, cheers!

I've got a pretty good idea of what's needed to finish this (RTFM) but it'll be a bit before I can get back on it. If someone wants to grab it, that's cool. Otherwise I'll get back at it later this week.

Ok /filter/tips looks a bit... wrong but the bulk of this works great:) I think this issue may help that ugly bit:
http://drupal.org/node/1778624

I added #type table stuff but will post a separate patch just in-case this one doesn't fly.
http://drupal.org/node/1938904

Joel: nice work. Some good cleanup and consolidation in here.

Re: the consolidation of filter_tips into item_list__filter_tips...:
i'm a lil torn. Thought 1 is: Do it right the first time. Thought 2: it's consolidation and not conversion, and consolidation may be easier after conversion. Plus, it's filter.module, maybe move on.. :)

In any case, a once-over on the patch:

+++ b/core/modules/filter/filter.module
@@ -74,22 +74,18 @@ function filter_help($path, $arg) {
   return array(
-    'filter_admin_format_filter_order' => array(
-      'render element' => 'element',
-      'file' => 'filter.admin.inc',
-    ),
     'filter_tips' => array(
       'variables' => array('tips' => NULL, 'long' => FALSE),
       'file' => 'filter.pages.inc',
+      'template' => 'filter-tips',
     ),
     'text_format_wrapper' => array(
       'render element' => 'element',
-    ),
-    'filter_tips_more_info' => array(
-      'variables' => array(),
+      'template' => 'text-format-wrapper',

Nice to see these go in favor of the consolidation of #tabledrag into filter_admin_format_form.

+++ b/core/modules/filter/filter.module
@@ -989,24 +997,20 @@ function filter_form_access_denied($element) {
 /**
- * Returns HTML for a text format-enabled form element.
+ * Preprocess variables for the text-format-wrapper template.
  *
- * @param array $variables

Use new preprocess docblock format: #1913208: [policy] Standardize template preprocess function documentation.

+++ b/core/modules/filter/filter.module
@@ -1180,32 +1184,82 @@ function filter_dom_serialize_escape_cdata_element($dom_document, $dom_element,
 /**
- * Returns HTML for a link to the more extensive filter tips.
+ * Returns HTML for guidelines for a text format.
+ *
+ * @param array $variables
+ *   An associative array containing:
+ *   - format: An object representing a text format.
  *
  * @ingroup themeable
+ * Preprocess filter_guidelines template.
  */
-function theme_filter_tips_more_info() {
-  return '<p>' . l(t('More information about text formats'), 'filter/tips', array('attributes' => array('target' => '_blank'))) . '</p>';

Oh this docblock is all messed up. :)

+++ b/core/modules/filter/filter.module
@@ -1180,32 +1184,82 @@ function filter_dom_serialize_escape_cdata_element($dom_document, $dom_element,
 /**
- * Returns HTML for guidelines for a text format.
+ * Returns HTML for a set of filter tips.
  *
  * @param array $variables
  *   An associative array containing:
- *   - format: An object representing a text format.
- *

Preprocess docblock.

+++ b/core/modules/filter/filter.module
@@ -1180,32 +1184,82 @@ function filter_dom_serialize_escape_cdata_element($dom_document, $dom_element,
+      'list' => $tiplist,
+    );
+  }
+
+

remove one \n

+++ b/core/modules/filter/filter.module
@@ -1180,32 +1184,82 @@ function filter_dom_serialize_escape_cdata_element($dom_document, $dom_element,
@@ -1829,7 +1883,7 @@ function _filter_html_image_secure_process($text) {
  * @see _filter_html_image_secure_process()
  * @ingroup themeable
  */
-function theme_filter_html_image_secure_image(&$variables) {
+function template_preprocess_filter_html_image_secure_image(&$variables) {

I'm guessing this needs a docblock edit.

+++ b/core/modules/filter/templates/filter-guidelines.html.twig
@@ -0,0 +1,23 @@
+{#
+/**
+ * @file
+ * Default theme implementation for guidelines for a text format.
+ *
+ * Available variables:
+ * - format: An object representing a text format.
+ * - attributes: Remaining html attributes for the containing element.
+ * - tips: Descriptions and a CSS id in the form of 'module-name/filter-id'
+ *   (only used when $long is TRUE) for each filter in one or more text
+ *   formats.
+ *
+ * @todo remove striptags when auto_escape is resolved http://drupal.org/node/1712444
+ *
+ * @see template_preprocess()
+ *
+ * @ingroup themeable
+ */
+ #}

1. I think we need a @see template_preprocess_filter_guidelines() or whatever the function's called?

2. Remove space from ' #}' at the bottom.

+++ b/core/modules/filter/templates/filter-tips.html.twig
@@ -0,0 +1,55 @@
+ *
+ * @TODO reimplement once http://drupal.org/node/1778624 gets resolved.

Super-nit: Coding style: lowercase @todo.

+++ b/core/modules/filter/templates/filter-tips.html.twig
@@ -0,0 +1,55 @@
+{% if tips|length %}
+  {% if multiple %}
+    <div class="compose-tips">
+  {% endif %}
+
+  {% for tip in tips %}
+    {% if multiple %}
+     <div{{ tip.attributes }}>
+      <h3>{{ tip.name }}</h3>
+    {% endif %}
+
+    {% if tip.list|length > 0 %}
+      <ul class="tips">

Need the ' > 0' in the second, 'tip.list|length > 0'?

+++ b/core/modules/filter/templates/text-format-wrapper.html.twig
@@ -0,0 +1,22 @@
+{#
+/**
+ * @file
+ * Default theme implementation for a text format-enabled form element.
+ *
+ * Available variables:
+ * - children: Text format element children.
+ * - description: Text format element description.
+ *
+ * @see template_preprocess()
+ * @see theme_text_format_wrapper()

@see template_preprocess..., not @see theme_...

Regarding the super-nit, after the @todo needs to be a complete sentence and therefore start with a capital letter in this case.

http://drupal.org/node/1354#drupal
http://drupal.org/node/1354#todo

Cleaned up docblocks and preprocess docs and added fixes in from #1938904: Convert filter theme tables to table #type
Not sure which commit will land first though, may have to reroll this one if the other lands first.

Let's not have those exceptions... shall we? And I forgot to save the length>0 thing.

Woot! It's green, nice work @joelpittet! Documentation nitpicks ahead:

+++ b/core/modules/filter/filter.admin.incundefined
@@ -236,15 +236,19 @@ function filter_admin_format_form($form, &$form_state, $format) {
+    // For filter.admin.js

+++ b/core/modules/filter/filter.moduleundefined
@@ -1204,7 +1203,9 @@ function template_preprocess_filter_guidelines(&$variables) {
+ * Prepares variables for a set of filter tips

These need to end in a period.

+++ b/core/modules/filter/filter.moduleundefined
@@ -1872,16 +1869,13 @@ function _filter_html_image_secure_process($text) {
+ * Prepares variables for Formats an image DOM element that has an invalid source.

Hmm, maybe "Prepares variables for an image DOM element that has an invalid source."?

+++ b/core/modules/filter/templates/filter-guidelines.html.twigundefined
@@ -10,9 +10,10 @@
+ * @todo Remove striptags when auto_escape is resolved http://drupal.org/node/1712444.

This @todo needs to be wrapped because it's over 80 characters with the URL.

http://drupal.org/node/1354#todo

+++ b/core/modules/filter/templates/filter-tips.html.twigundefined
@@ -12,7 +12,7 @@
+ * @todo reimplement once http://drupal.org/node/1778624 gets resolved.

s/reimplement/Reimplement

Caught a few more too, thanks @Cottser

I'm thinking we need a .html.twig file for filter_html_image_secure_image, no?

I see the rename and docblock update from theme_filter_html_image_secure_image -> template_preprocess_filter_html_image_secure_image but no .html.twig file.

Also this will probably need a reroll now that #1938904: Convert filter theme tables to table #type is in.

@Cottser, filter_html_image_secure_image is so weird! it doesn't return anything... not sure how to convert that to twig.

And sweet, didn't even see that was committed:) Will re-roll after I have a bit more idea of what to do with secure image...

theme_filter_image_secure_image() is very strange indeed, it's the only theme function that I've seen that doesn't return anything.

As discussed on IRC, I'm wondering if we can change theme_filter_image_secure_image() to a theme_image suggestion like image__secure_image, and move the logic to template_preprocess_image__secure_image. So in theory the output would still be through image.html.twig and would still be themeable by creating image--secure-image.html.twig.

See #1344078-16: Local image input filter in core, theme_filter_image_secure_image() was originally an alter hook.

See #1939068: Convert theme_image() to Twig for the theme_image conversion.

Add conversion summary table

Unassigning, and backing away slowly.

Will give it a try, thanks @joelpittet!

Tagging.

I'm hoping to get to this later tonight or tomorrow.

First the reroll (removes #type table conversion as well).

(Adding JavaScript tag because this touches filter.admin.js)

Especially with theme functions going away, it no longer makes sense to keep theme_filter_html_image_secure_image() as themeable. This patch moves that functionality to an alter hook, which themes can still implement if they want.

theme_filter_html_image_secure_image() is unlike any other theme function that I know of:

• It doesn't return a string.
• It alters the variables (a DOMElement to boot) by reference.

API change as well as manual testing steps added to issue summary.

Other changes:

• Updated documentation to match #1913208: [policy] Standardize template preprocess function documentation and Twig coding standards.
• Did other minor tweaks to make markup match up (removing default theme hook class).
• Removed an entire preprocess function in a similar manner as #1939086-36: Convert theme_dropbutton_wrapper() to Twig.

Linkify main #type table issue, add filter #type table issue to related

#30: 1898416-30.patch queued for re-testing.

This is a strange one, assigning to @steveoliver for another review.

Update: will be reviewing this Thursday, incase anyone else wants to review in the meantime.

Just looking at the patch here. Unassigning for someone else to do manual testing. :p

@@ -70,18 +70,15 @@ function filter_theme() {
     'filter_tips' => array(
       'variables' => array('tips' => NULL, 'long' => FALSE),
       'file' => 'filter.pages.inc',
+      'template' => 'filter-tips',
     ),
     'text_format_wrapper' => array(
-      'render element' => 'element',
-    ),
-    'filter_tips_more_info' => array(
-      'variables' => array(),
+      'variables' => array('children' => NULL, 'description' => NULL),
+      'template' => 'text-format-wrapper',
     ),
     'filter_guidelines' => array(
       'variables' => array('format' => NULL),
-    ),
-    'filter_html_image_secure_image' => array(
-      'variables' => array('image' => NULL),
+      'template' => 'filter-guidelines',
     ),
   );

It's always nice to see hook_theme entries die.

+++ b/core/modules/filter/filter.module
@@ -865,11 +862,23 @@ function filter_process_format($element) {
+  $filter_tips_link = array(
+    '#theme' => 'link',
+    '#text' => t('More information about text formats'),
+    '#path' => 'filter/tips',
+    '#options' => array(
+        'attributes' => array('target' => '_blank'),
+        'html' => FALSE,
+    ),
+    '#prefix' => '<p>',
+    '#suffix' => '</p>',
+  );
+
   $element['format']['help'] = array(
     '#type' => 'container',
-    '#theme' => 'filter_tips_more_info',
     '#attributes' => array('class' => array('filter-help')),
     '#weight' => 0,
+    $filter_tips_link,
   );

Why define $filter_tips_link here? Let's just add to the help array.

+++ b/core/modules/filter/filter.module
@@ -1126,32 +1114,84 @@ function filter_dom_serialize_escape_cdata_element($dom_document, $dom_element,
+  // Remove any classes added by template_preprocess().
+  // @todo Remove line below after http://drupal.org/node/1938430 is resolved.
+  $variables['attributes']['class'] = array();
+  $variables['attributes']['class'][] = 'filter-guidelines-item';
+  $variables['attributes']['class'][] = 'filter-guidelines-' . $format->format;
+  $variables['attributes'] = new Attribute($variables['attributes']);
+

I really don't like the look of this, wiping out the class attribute. I'd much rather see an array_diff type approach here.

+++ b/core/modules/filter/filter.module
@@ -1756,28 +1796,25 @@ function _filter_html_image_secure_process($text) {
-    // Replace an invalid image with an error indicator.
-    theme('filter_html_image_secure_image', array('image' => $image));
+    // Allow modules and themes to replace an invalid image with an error
+    // indicator. See filter_filter_secure_image_alter().
+    drupal_alter('filter_secure_image', $image);
   }

This is an excellent move here. Nice.

+++ b/core/modules/filter/templates/filter-tips.html.twig
@@ -0,0 +1,55 @@
+ * - long: (optional) Whether the passed-in filter tips contain extended
+ *   explanations, i.e. intended to be output on the path 'filter/tips'
+ *   (TRUE), or are in a short format, i.e. suitable to be displayed below a
+ *   form element. Defaults to FALSE.
+ *

Since the long variable is optional in the sense of the API/theme hook call, I suggest removing 'optional' here. Also, instead of the "Only set the class if the type is long" comment inline below, maybe we note that "...passed-in filter tips contain extended explanations" "...and should receive identifying class attributes..." "i.e. intended to be output..."

+++ b/core/modules/filter/templates/filter-tips.html.twig
@@ -0,0 +1,55 @@
+    {% if tip.list|length %}
+      <ul class="tips">
+      {% for item in tip.list %}
+        {# Only set the class if the type is long. #}
+        <li{{ item.attributes }}>{{ item.tip }}</li>
+      {% endfor %}
+      </ul>

If not removed altogether, maybe this comment should read something like "Class attributes are set in preprocess when long is TRUE." or something like that, since this isn't an instruction, but a note about what (class) may or may not exist.

I'll take care of the changes, thanks for the review @steveoliver!

Reminder to myself - look at #1222260: Remove theme_filter_tips_more_info() from core as well.

Add testing steps, note API change

array_diff didn't work in this case, but I found a nice approach in views_preprocess_html()!

Also tweaked whitespace in filter-tips.html.twig, one line was not indented properly (off by a space) - see the interdiff.

As for this from #35:

Also, instead of the "Only set the class if the type is long" comment inline below, maybe we note that "...passed-in filter tips contain extended explanations" "...and should receive identifying class attributes..." "i.e. intended to be output..."

I think that is covered by the 'tips' description:

tips: Descriptions and a CSS ID in the form of 'module-name/filter-id' (only used when 'long' is TRUE)…

Also changed the drupal_alter() to the new-style \Drupal::moduleHandler()->alter().

And fixing up theme_filter_tips_more_info() replacement per #1222260-18: Remove theme_filter_tips_more_info() from core to use l() instead of #theme link.

#39: 1898416-39.patch queued for re-testing.

Trying to reroll 1898416-39.patch.

Rerolled 1898416-39.

Changing status

#2: drupal-filter_twig-1898416-2.patch queued for re-testing.

rerunning tests

attempted to profile several patches, but got errors:

$ git apply 1898416-38.patch 
      error: patch failed: core/modules/filter/filter.api.php:130
      error: core/modules/filter/filter.api.php: patch does not apply

$ git apply 1898416-30.patch 
      error: patch failed: core/modules/filter/filter.api.php:130
      error: core/modules/filter/filter.api.php: patch does not apply

$ git apply drupal-filter_twig-1898416-19.patch 
      error: patch failed: core/modules/filter/filter.admin.inc:236
      error: core/modules/filter/filter.admin.inc: patch does not apply
      error: patch failed: core/modules/filter/filter.module:74
      error: core/modules/filter/filter.module: patch does not apply
      drupal-filter_twig-1898416-19.patch:515: new blank line at EOF.
      +

$ git apply drupal-filter_twig-1898416-17.patch 
      error: patch failed: core/modules/filter/filter.admin.inc:236
      error: core/modules/filter/filter.admin.inc: patch does not apply
      error: patch failed: core/modules/filter/filter.module:74
      error: core/modules/filter/filter.module: patch does not apply
      drupal-filter_twig-1898416-17.patch:514: new blank line at EOF.
      +

I incorrectly resolved conflict during rebase of first reroll (44). Rerolled again.

profiling...
latest patch applied properly

profiling output for: 1898416-48.patch

=== 8.x..8.x compared (519fe99884ff9..519ffe1be784d):

ct  : 71,226|71,226|0|0.0%
wt  : 466,475|472,063|5,588|1.2%
cpu : 419,918|419,082|-836|-0.2%
mu  : 16,002,240|16,002,240|0|0.0%
pmu : 16,130,312|16,130,312|0|0.0%

<a href="http://drupal8.local/xhprof-kit/xhprof/xhprof_html/index.php?source=drupal-perf&url=%2F&run1=519fe99884ff9&run2=519ffe1be784d&extra=8.x..8.x">Profiler output</a>

=== 8.x..profiling-1898416 compared (519fe99884ff9..519ffe7b01009):

ct  : 71,226|71,226|0|0.0%
wt  : 466,475|493,290|26,815|5.7%
cpu : 419,918|420,133|215|0.1%
mu  : 16,002,240|16,002,112|-128|-0.0%
pmu : 16,130,312|16,129,608|-704|-0.0%

<a href="http://drupal8.local/xhprof-kit/xhprof/xhprof_html/index.php?source=drupal-perf&url=%2F&run1=519fe99884ff9&run2=519ffe7b01009&extra=8.x..profiling-1898416">Profiler output</a>

---
ct = function calls, wt = wall time, cpu = cpu time used, mu = memory usage, pmu = peak memory usage
The drush command 'rr' could not be found.  Run `drush cache-clear drush` to clear the commandfile cache if you have installed new extensions.    [error]
'all' cache was cleared in self                                                                                                                   [success]

MANY APOLOGIES
It turns out, the profiling instructions I received at the DrupalCon Code Sprint was incomplete.
I am going to be re-running all my profiling :-/

Looks like #1985528: Convert filter_tips_long() to Controller made the changes in filter.pages.inc unnecessary. Re-rolling without that file seems to apply.

Remove sandbox link

Couple of nitpicks that could help with some of those errors. @bradwade unassigning you for now, but you are welcome to give this a crack and pick it back up, same to you @ezeedub.

@ernie-g you got profiling results, so thank you for that! Would be great to know what bit's were missing if you care to share so others don't make the mistake.

+++ b/core/modules/filter/filter.module
@@ -1011,32 +989,88 @@ function filter_dom_serialize_escape_cdata_element($dom_document, $dom_element,
+  $format = $variables['format'];
+  // Remove default 'filter-guidelines' class from attributes, but leave other
+  // classes.
+  // @todo: Remove this hack once http://drupal.org/node/1938430 lands.
+  $key = array_search('filter-guidelines', $variables['attributes']['class']->value());
+  if ($key !== FALSE) {

The hack is no longer needed.

+++ b/core/modules/filter/filter.module
@@ -1011,32 +989,88 @@ function filter_dom_serialize_escape_cdata_element($dom_document, $dom_element,
+  }
+  $variables['attributes']['class'][] = 'filter-guidelines-item';
+  $variables['attributes']['class'][] = 'filter-guidelines-' . $format->format;
+  $variables['attributes'] = new Attribute($variables['attributes']);

No need to call new Attribute() on $variables['attributes'] because it's lazy instantiated.

I had to re-roll the patch since you can't apply it to 8.x head. I applied it to an older commit (from May 28) and then do a git pull and fixed a couple of conflicts with head. I wasn't able to figure out how to do an interdiff for this. I will attach and make your edit suggestions in the next comment.

@Joel, I had made the fixes you suggested from #58

Thank you @drupalninja99 that looks great. Tagging, this should get another round of profiling. I think from what I heard the issue @ernie-g ran into was the missing full node block on the page he was testing, though that isn't really necessary now that all templates are in core.

Also needs manual testing, so if anybody can confirm the markup is what it should be that would be great.
See manual testing steps in the issue summary.

Manual testing complete, markup looked fine and user experiece was as expected.

Notes for the steps listed in the issue summary:

1. N/A
2. div with class="text-format-wrapper" contains children elements which are wrappers for the summary (initially hidden), the main textarea, and the format selection.
3. Tips for each format are generated by filter-guidelines.html.twig.
4. On the "Compose tips" page (filter/tips), the tips are generated by filter-tips.html.twig.
5. N/A
6. N/A
7. On node render, the image was replaced with the red X icon.

Looks like we lost a couple tags, adding back.

#60: filter-1898416-60.patch queued for re-testing.

expecting this will fail. tagging for needs reroll

Working on reroll

reroll to current head

#68 Passed and ready for review, testing or rework.

I checked this out and it looks good. I'm leaving this as needs review because I think this needs profiling.

Scenario 1

Stark
/filter/tips

=== 8.x..8.x compared (51ca5414b31d9..51ca5476d8061):

ct  : 33,633|33,633|0|0.0%
wt  : 391,355|390,148|-1,207|-0.3%
cpu : 362,404|362,029|-375|-0.1%
mu  : 32,909,544|32,909,544|0|0.0%
pmu : 32,992,344|32,992,344|0|0.0%

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

=== 8.x..1898416-filter-twig compared (51ca5414b31d9..51ca54a312a65):

ct  : 33,633|34,088|455|1.4%
wt  : 391,355|390,256|-1,099|-0.3%
cpu : 362,404|362,621|217|0.1%
mu  : 32,909,544|32,992,448|82,904|0.3%
pmu : 32,992,344|33,075,056|82,712|0.3%

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

Scenario 2

Stark
/node/add/article

=== 8.x..8.x compared (51ca5a4995c23..51ca5ac5912c3):

ct  : 56,043|56,043|0|0.0%
wt  : 559,402|559,750|348|0.1%
cpu : 512,161|510,582|-1,579|-0.3%
mu  : 37,589,096|37,589,096|0|0.0%
pmu : 37,764,360|37,764,360|0|0.0%

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

=== 8.x..1898416-filter-twig compared (51ca5a4995c23..51ca5b0eb2b08):

ct  : 56,043|56,684|641|1.1%
wt  : 559,402|563,129|3,727|0.7%
cpu : 512,161|514,665|2,504|0.5%
mu  : 37,589,096|37,810,336|221,240|0.6%
pmu : 37,764,360|37,985,168|220,808|0.6%

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

+++ b/core/modules/filter/templates/filter-guidelines.html.twig
@@ -0,0 +1,28 @@
+ *
+ * @todo Remove striptags when auto_escape is resolved
+ *   http://drupal.org/node/1712444.

Remove this @todo, that node can deal with itself.

+++ b/core/modules/filter/templates/filter-guidelines.html.twig
@@ -0,0 +1,28 @@
+ * @see template_preprocess()

Remove this as well.

+++ b/core/modules/filter/templates/filter-tips.html.twig
@@ -0,0 +1,54 @@
+ * @todo Reimplement once http://drupal.org/node/1778624 gets resolved.
...
+ * @see template_preprocess()

Remove these as well

+++ b/core/modules/filter/templates/text-format-wrapper.html.twig
@@ -0,0 +1,21 @@
+ * @see template_preprocess()

And this.

Made changes from #72. I wasn't sure if you wanted to remove the strip tags call or just the @todo doc. I did remove the strip tags call bc I figured it would no longer be needed once the referenced ticket is resolved.

@drupalninja99 Not the striptags or you will have to add back in the check_plain($format->name) to the preprocess because we don't want to ship with less security.

I have added striptags back.

Great thank you @drupalninja99

Needs a reroll

curl https://drupal.org/files/filter-1898416-75.patch | git apply --check
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 11621  100 11621    0     0   5883      0  0:00:01  0:00:01 --:--:--  7138
error: patch failed: core/modules/filter/filter.module:79
error: core/modules/filter/filter.module: patch does not apply

Here is the re-rolled patch

cleaning up tags

I applied the patch and tested it. It worked well!

+++ b/core/modules/filter/filter.admin.jsundefined
@@ -33,7 +33,7 @@ Drupal.behaviors.filterStatus = {
-        Drupal.tableDrag['filter-order'].restripeTable();
+        Drupal.tableDrag['edit-filters-order'].restripeTable();

Is this change correct? When editing a filter format afaics the id of the table is still filter-order.

I downloaded the patch, and, with the help of @jenlampton, ran through all of the steps listed in the "How to test" section. Everything went accordingly, looks great.

Hmm, yeah the JS change seems unnecessary, but needs confirmation.

I think that the reordering of filters got converted to #type table in #1938904: Convert filter theme tables to table #type.

Regarding the JS change: I looked at the interdiff in #11 and that is where the #type table conversion was started and the JS was changed. The #type table conversion was removed in #28 here after being committed in #1938904: Convert filter theme tables to table #type and in #1938904-3: Convert filter theme tables to table #type the ID was fixed so that the JS didn't need to be changed.

So we need to remove the change to filter.admin.js, and there are a few other clean-ups to be done:

1. +++ b/core/modules/filter/filter.module
   @@ -1145,27 +1127,80 @@ function filter_dom_serialize_escape_cdata_element($dom_document, $dom_element,
   +  $output = '';
   

   We can remove this unnecessary $output = '' line in template_preprocess_filter_tips(), this is never used.

2. +++ b/core/modules/filter/filter.module
   @@ -1145,27 +1127,80 @@ function filter_dom_serialize_escape_cdata_element($dom_document, $dom_element,
   +        $tiplist[$tip_key]['attributes']['class'] =  array('filter-' . str_replace("/", "-", $tip['id']));
   

   Extra space after = sign should be removed.

3. +++ b/core/modules/filter/templates/filter-guidelines.html.twig
   @@ -0,0 +1,24 @@
   + * - format: Contains information about the current text format, including the
   + *   following:
   + *   - format.name: The name of the text format.
   + *   - format.format: The machine name of the text format, e.g. 'basic_html'.
   

   We can remove the "format." prefixes from the documentation of these variables. Indenting in the docblock/api.d.o docs for variables means drill down/"add a dot".

4. +++ b/core/modules/filter/templates/filter-tips.html.twig
   @@ -0,0 +1,51 @@
   +{#
   +/**
   + * @file
   + * Default theme implementation for a set of filter tips.
   + *
   + * Available variables:
   + * - tips: Descriptions and a CSS ID in the form of 'module-name/filter-id'
   + *   (only used when 'long' is TRUE) for each filter in one or more text
   + *   formats.
   + * - long: A flag indicating whether the passed-in filter tips contain extended
   + *   explanations, i.e. intended to be output on the path 'filter/tips'
   + *   (TRUE), or are in a short format, i.e. suitable to be displayed below a
   + *   form element. Defaults to FALSE.
   + *
   + * @see template_preprocess_filter_tips()
   + *
   + * @ingroup themeable
   + */
   +#}
   +{% if multiple %}
   +  <h2>{{ 'Text Formats'|t }}</h2>
   +{% endif %}
   

   We should document the 'multiple' variable in the docblock.

Took care of the issue in #84.

Thank you @drupalmonkey!

Back to RTBC, been profiled @ #71

One more thing…

+++ b/core/modules/filter/filter.module
@@ -1130,27 +1112,79 @@ function filter_dom_serialize_escape_cdata_element($dom_document, $dom_element,
+  $multiple = count($tips) > 1;
...
+  $variables['multiple'] = $multiple;

Perhaps I'm missing something but couldn't we just do $variables['multiple'] = count($tips) > 1; Creating the $multiple variable seems unnecessary.

Ah you're right I think a while back that var had some if check but no longer does, though either it's not needed.

Removed the variable assignment per #88.

#90: filter-1898416-90.patch queued for re-testing.

Updated issue summary.

Thanks @drupalmonkey! Back to RTBC then.

#90: filter-1898416-90.patch queued for re-testing.

Needs a reroll

git ac https://drupal.org/files/filter-1898416-90.patch
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 11124  100 11124    0     0  10966      0  0:00:01  0:00:01 --:--:-- 13354
error: patch failed: core/modules/filter/filter.module:941
error: core/modules/filter/filter.module: patch does not apply

Re-rolled, should be back to RTBC if passes.

Looks good, thanks @joelpittet!

At this point I'm thinking that there should probably be some test coverage for the refactoring of theme_filter_html_image_secure_image() into an alter hook.

Re-roll.

#98: 1898416-98-filter-twig.patch queued for re-testing.

#98: 1898416-98-filter-twig.patch queued for re-testing.

The patch shrunk by a few KB so I think we need to look at another reroll.

Ok, I'm on it.

Ok, I'm on it.

Re-added missing files.

I think we're good here.

Remove suggested commit message, Dreditor gets it right :)

109: 1898416-109-filter-twig.patch queued for re-testing.

109: 1898416-109-filter-twig.patch queued for re-testing.

Re-rolled.

Glad to see this!

A few questions:

1. +++ b/core/modules/filter/filter.api.php
   @@ -24,6 +24,28 @@ function hook_filter_info_alter(&$info) {
   +function hook_filter_secure_image_alter(&$image) {
   

   The original intent was to have a single implementation for "theming" a placeholder image only, and the theme system was the natural place to put it.

   However, I agree that it's rather an abuse of a theme function, so the conversion into an alter hook is OK for now.

   We can think about better ways in a follow-up issue, if necessary.

2. +++ b/core/modules/filter/filter.module
   @@ -770,6 +770,8 @@ function filter_process_format($element) {
   +    '#prefix' => '<p>',
   +    '#suffix' => '</p>',
   

   Do we really need a wrapping paragraph here? That paragraph will get into your way when you try to re-style the text format selector widget.

   Can we remove the paragraph? The .filter-help class on the container should be sufficient?

3. +++ b/core/modules/filter/templates/filter-tips.html.twig
   @@ -0,0 +1,52 @@
   +  {% if multiple %}
   +    <div class="compose-tips">
   +  {% endif %}
   ...
   +    {% if multiple %}
   +      </div>
   +    {% endif %}
   ...
   +  {% if multiple %}
   +    </div>
   +  {% endif %}
   

   As a themer, the conditional output of the wrapper elements (and lack thereof) always resulted in a unexpected bad surprise when testing the design as anonymous user.

   I wonder whether we want to remove these 'multiple' conditionals as part of this conversion to (1) simplify the template and (2) prevent nasty surprises?

4. +++ b/core/modules/filter/templates/filter-tips.html.twig
   @@ -0,0 +1,52 @@
   +    {% if multiple %}
   +      <div{{ tip.attributes }}>
   +      <h3>{{ tip.name }}</h3>
   +    {% endif %}
   

   The 'multiple' condition here looks wrong to me -- the wrapping container + heading is only used for the long filter tips; i.e., the condition should be 'long' instead.

   That said, it would be great if we could split these two completely different representations (short/list vs. long/page) into two separate templates. Having them in a single never really made sense to me.

5. +++ b/core/modules/filter/templates/filter-tips.html.twig
   @@ -0,0 +1,52 @@
   +    {% if tip.list|length %}
   +      <ul class="tips">
   +      {% for item in tip.list %}
   +        <li{{ item.attributes }}>{{ item.tip }}</li>
   +      {% endfor %}
   +      </ul>
   +    {% endif %}
   

   The (short) filter tips are just a list.

   Following similar image/link conversions, shouldn't we use the regular list template instead of inventing a new one?

   Obviously, this case is a bit special, since the resulting output/markup depends on the data being passed in (it's only a list if there are multiple tips). I don't know how we handled such cases elsewhere?

@sun - thanks for your review and especially your feedback on the removal of theme_filter_html_image_secure_image() :)

My gut feeling at this point is that points 2-5 are a bit out of scope for a straight conversion that's purposefully not changing any markup. I would rather those changes be discussed in a follow-up issue, and for the record they all sound like very good ideas to me.

Going to see about adding some test coverage for the new alter hook.

Just wanted to mention that #5 from #118 sounds along the lines of #1778624: rework theme_filter_tips to use the new Attributes, and call theme('item_list) while we're at it so maybe it can be handled there. For the rest I can open a follow-up issue.

We already have test coverage for the alter hook in \Drupal\filter\Tests\FilterHtmlImageSecureTest::testImageSource(). Attaching a failing patch that shows what happens when the alter hook is removed.

So with that out of the way I decided to do some manual testing and profiling and review the patch again.

I manually tested /filter/tips and /node/add/article and compared markup via visual diff and DaisyDiff.

There were a couple slight discrepancies in the markup, fixed here (one of which is the removal of the wrapping paragraph mentioned in #118.2). I also noticed that we don't need filter.pages.inc at all (it just contained the now defunct theme_filter_tips()) so that's gone, and cleaned up a couple references to theme_filter_tips() as well.

The follow-up issue to discuss @sun's points in #118 is #2167277: Update filter.module markup and templates.

And below is some fresh profiling data, looks good to me!

/node/add/article

=== 8.x..8.x compared (52c96d7c36f9e..52c96df6ca13b):

ct  : 74,067|74,067|0|0.0%
wt  : 250,474|250,599|125|0.0%
cpu : 234,151|234,951|800|0.3%
mu  : 17,563,864|17,563,864|0|0.0%
pmu : 17,686,232|17,686,232|0|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=52c96d7c36f9e&...

=== 8.x..filter-1898416-122 compared (52c96d7c36f9e..52c96dcbc5e6a):

ct  : 74,067|74,392|325|0.4%
wt  : 250,474|251,228|754|0.3%
cpu : 234,151|235,669|1,518|0.6%
mu  : 17,563,864|17,675,128|111,264|0.6%
pmu : 17,686,232|17,798,080|111,848|0.6%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=52c96d7c36f9e&...

/filter/tips

=== 8.x..8.x compared (52c96b69a1c07..52c96ba78d568):

ct  : 33,923|33,923|0|0.0%
wt  : 122,927|123,001|74|0.1%
cpu : 115,004|115,005|1|0.0%
mu  : 11,603,520|11,603,520|0|0.0%
pmu : 11,713,952|11,713,952|0|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=52c96b69a1c07&...

=== 8.x..filter-1898416-122 compared (52c96b69a1c07..52c96bbf45f87):

ct  : 33,923|34,486|563|1.7%
wt  : 122,927|123,488|561|0.5%
cpu : 115,004|115,556|552|0.5%
mu  : 11,603,520|11,652,976|49,456|0.4%
pmu : 11,713,952|11,763,968|50,016|0.4%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=52c96b69a1c07&...

Update conversion status table.

Reviewing the code, this is looking really good and the profiling is fairly decent too.

One thing I spotted:

+++ b/core/modules/filter/templates/filter-guidelines.html.twig
@@ -0,0 +1,24 @@
+  <h4 class="label">{{ format.name|striptags }}</h4>

striptags != checkPlain. Closest would be |escape but I've yet to compare the two for their merits. Probably better to set checkPlain in the preprocess?

Sounds good.

The check_plain() was already there in preprocess, since that's deprecated now I changed it to String::checkPlain() (new hunk anyway so fair game). I'm not sure where the striptags came from because it wasn't there before as far as I can see:

$output .= '<h4 class="label">' . check_plain($format->name) . '</h4>';

Great, had another look at this patch and I think it's now good to go. Markup comparison + Profiling + Code Review + Testbot = RTBC

Posting markup before/after, should have done this above. Posting as files because they're pretty big chunks of markup.

I was wrong, the escaping in filter-guidelines needs another look.

Updated patch to add necessary escaping.

I checked Twig_Extension_Core::twig_escape_filter() which is what the escape filter ({{ string|escape }}) goes through.

Relevant code path:

return htmlspecialchars($string, ENT_QUOTES | ENT_SUBSTITUTE, $charset);

Where $charset is the default, UTF-8. So it looks like escape is a safe substitute for String::checkPlain() with the only difference being the addition of ENT_SUBSTITUTE:

Replace invalid code unit sequences with a Unicode Replacement Character U+FFFD (UTF-8) or &#FFFD; (otherwise) instead of returning an empty string.

So here's a patch that does that and also clarifies that format.name is potentially unsafe.

Great so it's String::checkPlain()++

Back to RTBC

Great work!

Committed and pushed to 8.x. Thanks!

I'm a noob and was interested in finding out what replaces theme_() functions in D8 so I decided to read the patch for this issue, and I noticed these lines:

+++ b/core/modules/filter/filter.module
@@ -1391,32 +1419,31 @@ function _filter_html_image_secure_process($text) {
+  t('hey hey');
+  t('hey hey');
+  t('hey hey');
+  t('hey hey');
+  t('hey hey');

My initial impression is that these were added for debugging, but they appear to have been committed to core. I'm going to assume this was unintentional and mark this as "Needs work". Sorry :( Feel free to change it back to "Fixed" if these were intentionally left in.

mparker17: you are correct, it seems this was missed when comitting. Good catch. I'll make a patch to remove them.

Here's a patch to remove the debugging strings (or whatever they were).

My fault! I was using that to debug the sanitization. Thanks @mparker17 and @mikl.

Edit: crossposted. We don't normally RTBC our own patches, but +1 to RTBC.

+1 to RTBC... I've applied the patch and it does remove all t('hey hey') lines from core.

mparker17@mcomp7:Projects/test_d8 (8.x ✓) % rgrep "t('hey hey')" .
./core/modules/filter/filter.module:1442:  t('hey hey');
./core/modules/filter/filter.module:1443:  t('hey hey');
./core/modules/filter/filter.module:1444:  t('hey hey');
./core/modules/filter/filter.module:1445:  t('hey hey');
./core/modules/filter/filter.module:1446:  t('hey hey');
./core/modules/filter/filter.module:1447:  t('hey hey');
mparker17@mcomp7:Projects/test_d8 (8.x ✓) % git apply --index ~/Downloads/2014-01-25-remove-debugging-strings.patch
mparker17@mcomp7:Projects/test_d8 (8.x ✗) % rgrep "t('hey hey')" .                                                           
mparker17@mcomp7:Projects/test_d8 (8.x ✗) % 

Ha. I am clearly losing it. ;P Good catch!

Unfortunately not back from testbot yet (in fact, not even queued) so I'll check again tomorrow.

136: 2014-01-25-remove-debugging-strings.patch queued for re-testing.

Yay, it appears to have passed testing!

There we go. :) Thanks for the quick turnaround, and for catching it in the first place!

Committed and pushed to 8.x.