Task

Use Twig instead of PHPTemplate

Remaining

  • Replace all tpl.php files with .html.twig equivalents
  • Replace all theme functions with .html.twig equivalent templates
  • Add new preprocess functions for the .html.twig equivalent templates
  • Update all hook_theme definitions

#1898472: [meta] Convert views_ui module to Twig
#1757550: [Meta] Convert core theme functions to Twig templates

Files: 
CommentFileSizeAuthor
#27 views-filter-reorder-elements.png182.35 KBjoelpittet
#27 views-ui-reorder-filters-top.png257.98 KBjoelpittet
#27 1963982-twig-views-ui-rearrange-filter-form-27.patch9.61 KBjoelpittet
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,868 pass(es). View
#27 interdiff.txt1.04 KBjoelpittet
#26 1963982-twig-views-ui-rearrange-filter-form-26.patch9.61 KBlauriii
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,866 pass(es). View
#26 interdiff-1963982-25-26.txt1.1 KBlauriii
#25 1963982-twig-views-ui-rearrange-filter-form-25.patch9.59 KBlauriii
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,866 pass(es). View
#25 interdiff-1963982-24-25.txt658 byteslauriii
#20 twig-views-ui-rearrange-filter-form-1963982-20.patch9.99 KBlauriii
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,209 pass(es). View
#9 1963982-9-twig-views-ui-rearrange-filter-form.patch10.05 KBjoelpittet
PASSED: [[SimpleTest]]: [MySQL] 54,624 pass(es). View
#9 interdiff.txt1.77 KBjoelpittet
#7 1963982-7-twig-views-ui-rearrange-filter-form.patch9.78 KBjoelpittet
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/views/views_ui/views_ui.theme.inc. View
#7 interdiff.txt3.88 KBjoelpittet
#4 twig-views-ui-rearrange-filter-form-1963982-4.patch9.06 KBjoelpittet
PASSED: [[SimpleTest]]: [MySQL] 54,272 pass(es). View
#2 twig-views-ui-rearrange-filter-form-1963982-2.patch9.05 KBjoelpittet
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/views/views_ui/views_ui.theme.inc. View

Comments

Cottser’s picture

Issue tags: +Needs manual testing

Tagging.

joelpittet’s picture

Status: Active » Needs review
FileSize
9.05 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/views/views_ui/views_ui.theme.inc. View

Split from meta.

Status: Needs review » Needs work

The last submitted patch, twig-views-ui-rearrange-filter-form-1963982-2.patch, failed testing.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
9.06 KB
PASSED: [[SimpleTest]]: [MySQL] 54,272 pass(es). View

whoops, removed a '+'

thedavidmeister’s picture

Status: Needs review » Needs work

Review for #4:

+ * Default theme implementation for views ui rearrange filter form.

Views UI

+ * - grouping: A boolean for if there is more than one group.

We're not including PHP data types in Twig template docs.

-      $row[] = array('colspan' => 5, 'data' => $instructions . drupal_render($form['remove_groups'][$group_id]));
+      $row[] = array('colspan' => 5, 'data' => $instructions . $form['remove_groups'][$group_id]);

Does this really work? $instructions would be a string and presumably $form['remove_groups'][$group_id] is a renderable array, right?

+ '#theme' => 'link',

We're not supposed to be doing this unless #1922454: Gut l(), fix theme('link'), have a working framework for consistently generated links inside and outside of Twig lands. Don't know if we need to change anything, but it's worth noting that this patch is essentially waiting on that.

+{% endif %}
+<div class="scroll">
+  {% if grouping %}

Shouldn't the div be indented two spaces?

This issue needs manual steps for testing to be added to the issue summary and for somebody to follow the steps and verify the markup manually.

Cottser’s picture

Thanks @thedavidmeister. I think the div is fine, it's not inside any Twig tags. I agree about the render array, the separate strings (or whatever they are) can probably be converted to an array inside 'data' (with weights if needed) instead of concatenating.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
3.88 KB
9.78 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/views/views_ui/views_ui.theme.inc. View

Yeah that $instructions concatenation was a blunder thanks @thedavidmeister

Left theme link conversion because it's only really active links that are affected there. Let me know if my assumption is wrong?

Status: Needs review » Needs work

The last submitted patch, 1963982-7-twig-views-ui-rearrange-filter-form.patch, failed testing.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
1.77 KB
10.05 KB
PASSED: [[SimpleTest]]: [MySQL] 54,624 pass(es). View

hmmm, how did that get in there, fixed array line length as well.

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

The last submitted patch, 1963982-9-twig-views-ui-rearrange-filter-form.patch, failed testing.

joelpittet’s picture

Status: Needs work » Needs review
Issue tags: +Needs manual testing, +Twig, +VDC
thedavidmeister’s picture

#7 -

Left theme link conversion because it's only really active links that are affected there. Let me know if my assumption is wrong?

Currently theme_link() skips the active class handling and also this sanitization:

  // Remove all HTML and PHP tags from a tooltip. For best performance, we act only
  // if a quick strpos() pre-check gave a suspicion (because strip_tags() is expensive).
  if (isset($options['attributes']['title']) && strpos($options['attributes']['title'], '<') !== FALSE) {
    $options['attributes']['title'] = strip_tags($options['attributes']['title']);
  }
tim.plunkett’s picture

+++ b/core/modules/views/views_ui/views_ui.theme.incundefined
@@ -276,22 +294,62 @@ function theme_views_ui_rearrange_filter_form(&$vars) {
+          '#theme' => 'link',

Normally in render arrays we use '#type' => 'link'

Otherwise, this looks great!

thedavidmeister’s picture

#13 - '#type' => 'link' is for form elements I thought.

tim.plunkett’s picture

No, it's a render element. All render elements are also form elements, but they can be used on their own. #type => container, for example.

Cottser’s picture

Status: Needs review » Postponed

It sounds like this will likely be converted in a similar manner to #1984850: Convert FilterPluginBase::build_group_form to use table rendering and remove theme function, postponing until we have an issue created for that separate conversion.

Cottser’s picture

Issue summary: View changes

Remove sandbox link

Cottser’s picture

joelpittet’s picture

Status: Postponed » Active

Unpostpoing as there has been a while waiting, let's do this conversion for now.

lauriii’s picture

Assigned: Unassigned » lauriii
Issue tags: +Drupalaton 2014
lauriii’s picture

Assigned: lauriii » Unassigned
Status: Active » Needs review
FileSize
9.99 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,209 pass(es). View
joelpittet’s picture

Status: Needs review » Needs work

@lauriii have a look at #1963980: Convert theme_views_ui_expose_filter_form() to Twig Would you mind doing the same here? Removing the preprocess entirely if possible, and attempt at filling out the @todos?

lauriii’s picture

Assigned: Unassigned » lauriii

Yeah, I'll take look on that.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
6.2 KB
9.3 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,866 pass(es). View

Thanks @lauriii here's the changes since #20

Mind doing an manual testing?

joelpittet’s picture

This fixes at least the remove button to show up because #theme link doesn't exist but #type link does.

For some reason the the action buttons have an extra "Create new filter group". I'm not really sure why at the moment...

This patch fixes all of the Twig autoescaping issues with this theme. See screenshot.


@laurii Want to take a stab at getting that action link to do what it did before (I assume hide or JS hide...)
And maybe a markup comparison?

lauriii’s picture

FileSize
658 bytes
9.59 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,866 pass(es). View
86.69 KB
164.82 KB
122.84 KB

The patch applied here solves issue described on #24.

I did markup comparison and found few differences:
1) Default group filter is visible all the time because its missing .tabledrag-hide. Patch applied here should fix this.

Before:

After:

2) .views-messages element is visible even if there's no messages.

Create new filter group functionality doesn't work for some reason, so I couldn't test what happens when there's multiple groups.

lauriii’s picture

FileSize
1.1 KB
9.61 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,866 pass(es). View

@joelpittet found some more changes on the markup so here's a fix for them:
1) Empty markup have changed so I rolled it back for how it used to be.
2) Elements on the remove col wasn't arranged correctly.

joelpittet’s picture

Assigned: lauriii » Unassigned
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs manual testing
FileSize
1.04 KB
9.61 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,868 pass(es). View
257.98 KB
182.35 KB

Ok so we fixed the escaping issues with this function, which is a plus.

I manually fixed the before patch markup &gt; &lt; to accurately to a comparison.
@lauriii fixed the minor ordering issue with a #weight and and the empty markup was moved back to where it was because #empty didn't seem to work on the #type table so no need to make that change in this conversion issue.

I've just added a couple minor changes still, removed Javascript:void() from the link to match and got #instructions to show up in the markup with #markup.

Markup Review Screenshots:

Cottser’s picture

Adding a related issue for the existing filter group bug.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

  • webchick committed 3ad2872 on 8.0.x
    Issue #1963982 by lauriii, joelpittet: Convert...

Status: Fixed » Closed (fixed)

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