Task

Convert filter.module theme functions to Twig templates.

Remaining

  • Patch needs review
  • Manual testing (steps below)

Theme function name/template path Conversion status
theme_filter_admin_format_filter_order Converted to #type table (and committed) in #1938904: Convert filter theme tables to table #type
theme_filter_admin_overview Already converted to #type table in #80855: Add element #type table and merge tableselect/tabledrag into it
theme_filter_guidelines converted
theme_filter_html_image_secure_image Changed to an alter hook (see API changes section)
theme_filter_tips converted
theme_filter_tips_more_info Removed in #1222260: Remove theme_filter_tips_more_info() from core
theme_text_format_wrapper converted

Testing steps

  1. Add an article node (node/add/article)
  2. The body field should be wrapped in text-format-wrapper.html.twig.
  3. The set of filter guidelines for each text format should be output by filter-guidelines.html.twig.
  4. The list of filter tips should be output by filter-tips.html.twig.
  5. Using the Basic HTML format, click the image button in the editor toolbar and add an image hosted on another site.
  6. Save the node.
  7. When viewing the node, the external image you added should be replaced with a red X.

See also /filter/tips.

API changes

The patch removes theme_filter_html_image_secure_image() and replaces it with an alter hook (hook_filter_secure_image_alter()). Part of the Twig conversion is removing theme functions, and unlike other theme functions, theme_filter_html_image_secure_image():

  • Doesn't return a string.
  • Alters the variables (a DOMElement to boot) by reference.
Files: 
CommentFileSizeAuthor
#136 2014-01-25-remove-debugging-strings.patch660 bytesmikl
PASSED: [[SimpleTest]]: [MySQL] 63,087 pass(es). View
#131 interdiff.txt943 bytesCottser
#131 1898416-131.patch14.02 KBCottser
PASSED: [[SimpleTest]]: [MySQL] 59,578 pass(es). View

Comments

c4rl’s picture

Issue tags: +Twig

Tagging

disasm’s picture

Status: Active » Needs review
FileSize
14 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: failed to create checkout database. View

Attached is a patch

Status: Needs review » Needs work

The last submitted patch, drupal-filter_twig-1898416-2.patch, failed testing.

Cottser’s picture

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.

kerasai’s picture

Status: Needs work » Needs review
Issue tags: +SprintWeekend2013
FileSize
1.78 KB
14.2 KB
FAILED: [[SimpleTest]]: [MySQL] 52,988 pass(es), 15 fail(s), and 261 exception(s). View

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.

kerasai’s picture

FileSize
24.08 KB
29.7 KB

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:
formats.jpg

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

joelpittet’s picture

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

Status: Needs review » Needs work

The last submitted patch, drupal-filter_twig-1898416-5.patch, failed testing.

kerasai’s picture

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.

joelpittet’s picture

Assigned: Unassigned » joelpittet
joelpittet’s picture

Status: Needs work » Needs review
FileSize
15.07 KB
15.43 KB
FAILED: [[SimpleTest]]: [MySQL] 52,608 pass(es), 167 fail(s), and 439 exception(s). View

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

Status: Needs review » Needs work

The last submitted patch, drupal-filter_twig-1898416-11.patch, failed testing.

steveoliver’s picture

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

Cottser’s picture

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

joelpittet’s picture

Status: Needs work » Needs review
FileSize
6.34 KB
16.5 KB
FAILED: [[SimpleTest]]: [MySQL] 53,229 pass(es), 0 fail(s), and 86 exception(s). View

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.

Status: Needs review » Needs work

The last submitted patch, drupal-filter_twig-1898416-15.patch, failed testing.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
1.22 KB
16.46 KB
PASSED: [[SimpleTest]]: [MySQL] 53,353 pass(es). View

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

Cottser’s picture

Status: Needs review » Needs work

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

joelpittet’s picture

Status: Needs work » Needs review
FileSize
3.36 KB
16.46 KB
PASSED: [[SimpleTest]]: [MySQL] 53,202 pass(es). View

Caught a few more too, thanks @Cottser

Cottser’s picture

Status: Needs review » Needs work

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.

Cottser’s picture

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

joelpittet’s picture

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

Cottser’s picture

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.

Cottser’s picture

Issue summary: View changes

Add conversion summary table

joelpittet’s picture

Assigned: joelpittet » Unassigned

Unassigning, and backing away slowly.

Cottser’s picture

Assigned: Unassigned » Cottser

Will give it a try, thanks @joelpittet!

Cottser’s picture

Issue tags: +Needs manual testing

Tagging.

Cottser’s picture

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

Cottser’s picture

Status: Needs work » Needs review
FileSize
13.24 KB
FAILED: [[SimpleTest]]: [MySQL] 54,266 pass(es), 1 fail(s), and 0 exception(s). View

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

Status: Needs review » Needs work

The last submitted patch, 1898416-28.patch, failed testing.

Cottser’s picture

Assigned: Cottser » Unassigned
Status: Needs work » Needs review
Issue tags: +JavaScript
FileSize
6.91 KB
15.13 KB
PASSED: [[SimpleTest]]: [MySQL] 54,206 pass(es). View

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

Cottser’s picture

Issue summary: View changes

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

Status: Needs review » Needs work
Issue tags: -JavaScript, -Needs manual testing, -Twig, -SprintWeekend2013

The last submitted patch, 1898416-30.patch, failed testing.

Cottser’s picture

Status: Needs work » Needs review
Issue tags: +JavaScript, +Needs manual testing, +Twig, +SprintWeekend2013

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

Cottser’s picture

Assigned: Unassigned » steveoliver

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

steveoliver’s picture

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

steveoliver’s picture

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

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.

Cottser’s picture

Assigned: Unassigned » Cottser

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

Cottser’s picture

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

Cottser’s picture

Issue summary: View changes

Add testing steps, note API change

Cottser’s picture

Assigned: Cottser » Unassigned
Status: Needs work » Needs review
FileSize
3.71 KB
15.11 KB
PASSED: [[SimpleTest]]: [MySQL] 55,437 pass(es). View

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

Cottser’s picture

FileSize
784 bytes
15.02 KB
FAILED: [[SimpleTest]]: [MySQL] 51,139 pass(es), 2,636 fail(s), and 1,154 exception(s). View

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.

c4rl’s picture

Title: Convert filter module to Twig » filter.module - Convert theme_ functions to Twig

Per #1757550-44: [Meta] Convert core theme functions to Twig templates, retitling to indicate this issue applies to theme_ functions, which are lower in priority than PHPTemplate conversion issues.

intergalactic overlords’s picture

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

Status: Needs review » Needs work
Issue tags: +JavaScript, +Needs manual testing, +Twig, +SprintWeekend2013

The last submitted patch, 1898416-39.patch, failed testing.

bradwade’s picture

Assigned: Unassigned » bradwade

Trying to reroll 1898416-39.patch.

bradwade’s picture

FileSize
20.88 KB
FAILED: [[SimpleTest]]: [MySQL] 51,867 pass(es), 2,650 fail(s), and 1,158 exception(s). View

Rerolled 1898416-39.

bradwade’s picture

Status: Needs work » Needs review

Changing status

hanpersand’s picture

Status: Needs review » Needs work
Issue tags: +JavaScript, +Needs manual testing, +Twig, +SprintWeekend2013

The last submitted patch, 1898416-44.patch, failed testing.

jerdavis’s picture

Status: Needs work » Needs review

rerunning tests

ernie-g’s picture

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.
      +
bradwade’s picture

FileSize
15.05 KB
FAILED: [[SimpleTest]]: [MySQL] 50,580 pass(es), 2,625 fail(s), and 1,150 exception(s). View

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

ernie-g’s picture

profiling...
latest patch applied properly

ernie-g’s picture

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]
ernie-g’s picture

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

Status: Needs review » Needs work

The last submitted patch, 1898416-48.patch, failed testing.

ezeedub’s picture

Issue tags: -Needs manual testing, -SprintWeekend2013
FileSize
12.33 KB
FAILED: [[SimpleTest]]: [MySQL] 52,021 pass(es), 2,766 fail(s), and 1,197 exception(s). View

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.

ezeedub’s picture

Status: Needs work » Needs review
ezeedub’s picture

Issue summary: View changes

Remove sandbox link

Status: Needs review » Needs work

The last submitted patch, filter-1898416-55.patch, failed testing.

joelpittet’s picture

Assigned: bradwade » Unassigned

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.

drupalninja99’s picture

FileSize
11.92 KB
FAILED: [[SimpleTest]]: [MySQL] 50,421 pass(es), 2,655 fail(s), and 1,158 exception(s). View

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.

drupalninja99’s picture

Status: Needs work » Needs review
FileSize
1.14 KB
11.52 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch filter-1898416-60.patch. Unable to apply patch. See the log in the details link for more information. View

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

joelpittet’s picture

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.

kerasai’s picture

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.
kerasai’s picture

Issue tags: +Novice, +Needs profiling

Looks like we lost a couple tags, adding back.

jenlampton’s picture

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

jenlampton’s picture

Issue tags: +Needs reroll

expecting this will fail. tagging for needs reroll

Status: Needs review » Needs work

The last submitted patch, filter-1898416-60.patch, failed testing.

pwieck’s picture

Assigned: Unassigned » pwieck

Working on reroll

pwieck’s picture

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

reroll to current head

pwieck’s picture

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

siccababes’s picture

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

joelpittet’s picture

Assigned: pwieck » Unassigned
Issue tags: -Novice, -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&...

joelpittet’s picture

Status: Needs review » Needs work
+++ 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.

drupalninja99’s picture

Status: Needs work » Needs review
FileSize
1.96 KB
11.34 KB
PASSED: [[SimpleTest]]: [MySQL] 58,076 pass(es). View

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.

joelpittet’s picture

Status: Needs review » Needs work

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

drupalninja99’s picture

Status: Needs work » Needs review
FileSize
11.35 KB
PASSED: [[SimpleTest]]: [MySQL] 58,059 pass(es). View
627 bytes

I have added striptags back.

joelpittet’s picture

Status: Needs review » Reviewed & tested by the community

Great thank you @drupalninja99

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

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

Status: Needs work » Needs review
FileSize
907 bytes
11.4 KB
PASSED: [[SimpleTest]]: [MySQL] 56,264 pass(es). View

Here is the re-rolled patch

jenlampton’s picture

Issue tags: -JavaScript, -Needs reroll

cleaning up tags

adamcowboy’s picture

Status: Needs review » Reviewed & tested by the community

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

alexpott’s picture

Status: Reviewed & tested by the community » Needs review
+++ 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.

eromero1’s picture

Status: Needs review » Reviewed & tested by the community

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.

Cottser’s picture

Status: Reviewed & tested by the community » Needs review

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.

Cottser’s picture

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

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.

drupalmonkey’s picture

Assigned: Unassigned » drupalmonkey
drupalmonkey’s picture

Assigned: drupalmonkey » Unassigned
Status: Needs work » Needs review
FileSize
2.86 KB
10.89 KB
PASSED: [[SimpleTest]]: [MySQL] 58,119 pass(es). View

Took care of the issue in #84.

joelpittet’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Novice

Thank you @drupalmonkey!

Back to RTBC, been profiled @ #71

Cottser’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: +Novice

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.

joelpittet’s picture

Status: Needs review » Needs work

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.

drupalmonkey’s picture

Status: Needs work » Needs review
FileSize
865 bytes
10.86 KB
PASSED: [[SimpleTest]]: [MySQL] 58,530 pass(es). View

Removed the variable assignment per #88.

Status: Needs review » Needs work
Issue tags: -Novice, -Twig

The last submitted patch, filter-1898416-90.patch, failed testing.

Cottser’s picture

Status: Needs work » Needs review
Issue tags: +Novice, +Twig

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

Cottser’s picture

Issue summary: View changes

Updated issue summary.

Cottser’s picture

Status: Needs review » Reviewed & tested by the community

Thanks @drupalmonkey! Back to RTBC then.

kim.pepper’s picture

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

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

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

Status: Needs work » Needs review
Issue tags: -Novice, -Needs reroll
FileSize
10.84 KB
PASSED: [[SimpleTest]]: [MySQL] 58,469 pass(es). View

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

Cottser’s picture

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

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.

mikl’s picture

FileSize
7.61 KB
FAILED: [[SimpleTest]]: [MySQL] 52,409 pass(es), 2,098 fail(s), and 227 exception(s). View

Re-roll.

mikl’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
Issue tags: -Needs tests, -Twig

The last submitted patch, 1898416-98-filter-twig.patch, failed testing.

mikl’s picture

Status: Needs work » Needs review

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

Status: Needs review » Needs work

The last submitted patch, 1898416-98-filter-twig.patch, failed testing.

danquah’s picture

Status: Needs work » Needs review

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

Status: Needs review » Needs work
Issue tags: +Needs tests, +Twig

The last submitted patch, 1898416-98-filter-twig.patch, failed testing.

Cottser’s picture

Issue tags: +Needs reroll

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

mikl’s picture

Assigned: Unassigned » mikl

Ok, I'm on it.

mikl’s picture

Ok, I'm on it.

mikl’s picture

Assigned: mikl » Unassigned
Status: Needs work » Needs review
FileSize
7.61 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion. View
mikl’s picture

FileSize
10.84 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1898416-109-filter-twig.patch. Unable to apply patch. See the log in the details link for more information. View

Re-added missing files.

mikl’s picture

Issue tags: -Needs reroll

I think we're good here.

robmc’s picture

Assigned: Unassigned » robmc
robmc’s picture

Issue summary: View changes

Remove suggested commit message, Dreditor gets it right :)

joelpittet’s picture

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

joelpittet’s picture

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

Status: Needs review » Needs work

The last submitted patch, 109: 1898416-109-filter-twig.patch, failed testing.

robmc’s picture

Assigned: robmc » Unassigned
Issue summary: View changes
kim.pepper’s picture

joelpittet’s picture

Status: Needs work » Needs review
FileSize
10.7 KB
PASSED: [[SimpleTest]]: [MySQL] 58,995 pass(es). View

Re-rolled.

sun’s picture

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?

Cottser’s picture

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

Cottser’s picture

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

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

Cottser’s picture

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.

Cottser’s picture

Assigned: Cottser » Unassigned
Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -Needs tests
Related issues: +#1938904: Convert filter theme tables to table #type, +#1778624: rework theme_filter_tips to use the new Attributes, and call theme('item_list) while we're at it
FileSize
13.67 KB
FAILED: [[SimpleTest]]: [MySQL] 59,372 pass(es), 15 fail(s), and 0 exception(s). View
571 bytes
13.87 KB
PASSED: [[SimpleTest]]: [MySQL] 59,383 pass(es). View
5 KB

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

Cottser’s picture

Issue summary: View changes

Update conversion status table.

The last submitted patch, 122: 1898416-122-fail.patch, failed testing.

joelpittet’s picture

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?

Cottser’s picture

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

Sounds good.

Cottser’s picture

Assigned: Cottser » Unassigned
Status: Needs work » Needs review
FileSize
13.86 KB
PASSED: [[SimpleTest]]: [MySQL] 59,552 pass(es). View
944 bytes

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>';
joelpittet’s picture

Status: Needs review » Reviewed & tested by the community

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

Cottser’s picture

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

Cottser’s picture

Status: Reviewed & tested by the community » Needs work

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

Cottser’s picture

Status: Needs work » Needs review
FileSize
14.02 KB
PASSED: [[SimpleTest]]: [MySQL] 59,578 pass(es). View
943 bytes

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.

joelpittet’s picture

Status: Needs review » Reviewed & tested by the community

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

Back to RTBC

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Great work!

Committed and pushed to 8.x. Thanks!

mparker17’s picture

Status: Fixed » Needs work

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.

mikl’s picture

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

mikl’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
660 bytes
PASSED: [[SimpleTest]]: [MySQL] 63,087 pass(es). View

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

Cottser’s picture

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.

Cottser’s picture

Title: filter.module - Convert theme_ functions to Twig » filter.module - Convert theme_ functions to Twig [small followup]
Issue tags: +Quick fix
mparker17’s picture

+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 ✗) % 
webchick’s picture

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.

mparker17’s picture

mparker17’s picture

Yay, it appears to have passed testing!

webchick’s picture

Title: filter.module - Convert theme_ functions to Twig [small followup] » filter.module - Convert theme_ functions to Twig
Status: Reviewed & tested by the community » Fixed

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

Committed and pushed to 8.x.

Status: Fixed » Closed (fixed)

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