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
#23 interdiff.txt673 bytesjoelpittet
#23 1963980-twig-views_ui-expose-filter-form-23.patch3.89 KBjoelpittet
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,528 pass(es). View
#22 interdiff.txt943 bytesjoelpittet
#22 1963980-twig-views_ui-expose-filter-form-22.patch3.9 KBjoelpittet
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,596 pass(es). View
#17 interdiff.txt2 KBjoelpittet
#16 1963980-twig-views_ui-expose-filter-form-16.patch3.9 KBjoelpittet
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,411 pass(es). View
#16 interdiff.txt3.93 KBjoelpittet
#11 1963980-twig-views_ui-expose-filter-form-11.patch3.8 KBjoelpittet
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,162 pass(es). View
#6 interdiff.txt2.94 KBjoelpittet
#6 1963980-6-twig-views_ui-expose-filter-form.patch4.91 KBjoelpittet
PASSED: [[SimpleTest]]: [MySQL] 55,447 pass(es). View
#3 twig-views-ui-expose-filter-form-1963980-3.patch4.91 KBjoelpittet
PASSED: [[SimpleTest]]: [MySQL] 54,154 pass(es). View
#3 interdiff.txt728 bytesjoelpittet
#2 twig-views-ui-expose-filter-form-1963980-2.patch4.91 KBjoelpittet
PASSED: [[SimpleTest]]: [MySQL] 54,316 pass(es). View

Comments

Cottser’s picture

Issue tags: +Needs manual testing

Tagging.

joelpittet’s picture

Status: Active » Needs review
Issue tags: -Needs manual testing
FileSize
4.91 KB
PASSED: [[SimpleTest]]: [MySQL] 54,316 pass(es). View

Split from meta.

joelpittet’s picture

Issue tags: +Needs manual testing
FileSize
728 bytes
4.91 KB
PASSED: [[SimpleTest]]: [MySQL] 54,154 pass(es). View

Don't know why the tag got removed... adding it back in and removing the whitespace.

thedavidmeister’s picture

This looks good to me from a code style perspective. Ready for somebody to start manually testing the markup for :) don't forget to write out the steps for testing in the issue summary.

The only thing is that there's an awful lot of undocumented variables with @todo in this patch - are we going to open a followup ticket for these?

alexrayu’s picture

Can't make it work in reality - the template is not getting called. 'views_ui_expose_filter_form' - is this the correct name for it? My DPM gives me the name 'views_ui_config_item_form' on the exposed filter config page.

joelpittet’s picture

FileSize
4.91 KB
PASSED: [[SimpleTest]]: [MySQL] 55,447 pass(es). View
2.94 KB

@alexrayu not sure what you mean? If you go to views for front page, and open up the filter dialog and click expose.
/admin/structure/views/view/frontpage/edit
Content: Promoted to front page status

I found some random things in the form that I don't know why they got there and added some todo cleanup.

tim.plunkett’s picture

+++ b/core/modules/views/views_ui/views_ui.theme.incundefined
@@ -75,43 +75,58 @@ function theme_views_ui_view_info($variables) {
+  $variables['form_description'] = $form['form_description'];
+  $variables['expose_button'] = $form['expose_button'];
+  $variables['group_button'] = $form['group_button'];
+  unset($form['form_description']);
+  unset($form['expose_button']);
+  unset($form['group_button']);

See my comment in #1963978-12: Convert theme_views_ui_build_group_filter_form() to Twig, I think we should do a similar thing here.

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.

joelpittet’s picture

Status: Active » Needs review
FileSize
3.8 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,162 pass(es). View

Here's a new patch now that we have without() filter we don't need the preprocess.

I gave it a manual test locally and it seems to work out nicely. Could use a second pair of eyes.

longwave’s picture

Status: Needs review » Reviewed & tested by the community

Patch looks good, also manually tested successfully.

There is some code in FilterPluginBase that seems like it should belong in the template, but I guess this is a separate issue:

       // Increase the width of the left (operator) column.
      $form['operator']['#prefix'] = '
'; $form['operator']['#suffix'] = '
'; $form['value']['#prefix'] = '
'; $form['value']['#suffix'] = '
';
alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/views_ui/templates/views-ui-expose-filter-form.html.twig
@@ -0,0 +1,68 @@
+ * - group_button: @todo.
...
+ * - operator: @todo.
+ * - value: @todo.
+ * - use_operator: @todo.
+ * - has_left_column: @todo.
...
+ * - more: @todo.

Shouldn't we do?

+++ b/core/modules/views_ui/templates/views-ui-expose-filter-form.html.twig
@@ -0,0 +1,68 @@
+{% if form.use_operator is not empty %}
...
+{% if form.operator['#type'] %}

Shouldn't these if's be the same? Looks very weird to me. That said this weirdness is in the original .tpl.php

joelpittet’s picture

@longwave Want to give the other postponed one a go? It's very similar to this one. Likely easier to start from scratch mostly as I did here. #1963982: Convert theme_views_ui_rearrange_filter_form() to Twig

Sorry for leaving all the @todos in there.

joelpittet’s picture

@alexpott re #13.2 I tried to match the originals. is not empty allows for 0's where as the second one just checks for a truthy value.

The check for {{ form.required }} isn't needed because it doesn't have a wrapper div to concern itself with.

{{ form.use_operator }} could be a 0. Though maybe unlikely I wasn't confident in that decision. If someone else can confirm that 0's can be safely disregarded (aka !empty() vs isset()) Then I'd be glad to change it. #type was !empty() which is simply {% if var %}

joelpittet’s picture

Status: Needs work » Needs review
FileSize
3.93 KB
3.9 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,411 pass(es). View

Tried to fill out the @todos to the best of my ability (non-existent docs before).

joelpittet’s picture

FileSize
2 KB

Sorry that interdiff was useless, here it is again.

longwave’s picture

I wonder if we should just remove this template. The form structure is complicated and very variable depending on the filter in use and the various options that it may have, and I can't see why anyone would need to override this form in their theme; also it seems like it depends on a lot of #prefix and #suffix attributes that are simply hardcoded. The existing template logic can be moved to FilterPluginBase.

Also, the output already appears to be subtly broken, before this patch - edit the default Content view, click the "Published status" filter, and change "Filter type to expose" to Grouped. Perhaps this is due to the SafeMarkup feature, though.

joelpittet’s picture

I couldn't see any difference before and after this patch. Likely don't need this template file, and that was the original intent of postponing, but I just want to get rid of the theme function. Removing this template will just further break the layout even more than it already is.

I'm fine with it being here and if someone wants to abstract it out entirely that's fine too, but the goal is just to get it converted so it's easier to maintain a a template instead of HTML in PHP strings.

The amount of drupal_renders() removed alone is a win to me.

longwave’s picture

In that case I think this is fine. I am pretty sure it is safe to check form.use_operator is not empty in both the if statements as use_operator will either be a checkbox element or unset, never 0. However if we are going to stay with the 1:1 conversion and clean this up later, then the documentation needs a minor fix:

+ * - operator: The operators for how the filters value should be treated.
+ * - value: The filters available values.
+ * - use_operator: Checkbox to allow the user to expose the operator.
+ *   - #type: The operator type.

#type belongs to operator, not use_operator.

longwave’s picture

Status: Needs review » Needs work

I think this is ready for RTBC if we fix the minor documentation bug in #20.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
3.9 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,596 pass(es). View
943 bytes

@longwave thanks for the poke I forgot to make the quick fix.

joelpittet’s picture

FileSize
3.89 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,528 pass(es). View
673 bytes

Actually I agree with it being ok to remove the is not empty thanks, for got that in #22

longwave’s picture

Status: Needs review » Reviewed & tested by the community
alexpott’s picture

Status: Reviewed & tested by the community » Fixed

ok let's clean this up later...

Committed 14b58a4 and pushed to 2317865. Thanks!

akalata’s picture

longwave’s picture

Status: Fixed » Reviewed & tested by the community

Committed 14b58a4 and pushed to 2317865. Thanks!

Huh? Not committed to 8.0.x anyway, that I can see.

Cottser’s picture

Issue tags: +Twig conversion

Re-adding one of the tags for drupaltwig.org :)

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 2ff3381 and pushed to 8.0.x. Thanks!

  • alexpott committed 2ff3381 on 8.0.x
    Issue #1963980 by joelpittet | Cottser: Convert...

Status: Fixed » Closed (fixed)

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