Convert views/templates/views-exposed-form.tpl.php to twig

Separated from meta patch, first draft

Tagging.

Title change, to be consistent with the others.

Committed, thanks!

Moving to core queue.

add twig tag

Included the wrong patch, ignore #9

This is the functional patch and innerdiff

Looks great, Sean. Nice work!

Everything looks great except one docs nitpick:

+++ b/core/modules/views/views.theme.incundefined
@@ -936,19 +936,21 @@ function template_preprocess_views_view_row_rss(&$vars) {
 /**
- * Default theme function for all filter forms.
+ * Prepares variables for views exposed form templates.
+ *
+ * Default template: views-view-exposed-form.html.twig.
  */

Can we document the available variables here? It looks like at least $form could be added here.

+++ b/core/modules/views/views.theme.incundefined
@@ -936,19 +936,21 @@ function template_preprocess_views_view_row_rss(&$vars) {
+ * Default template: views-view-exposed-form.html.twig.

Actually, shouldn't this be views-exposed-form.html.twig?

Was not there originally, the parms include only $variables.
$form is defined within the function itself (them passed by reference).
This occurs on several of the views template_ functions and what ever we decide, we can keep it consistent throughout. Perhaps this has already been discussed...

Because it wasn't there before, I would lean towards not adding it.

Available indexes in the $variables array should always be documented even if $variables only contains an 'element' render array, please see #1913208: [policy] Standardize template preprocess function documentation. This is similar to the standards for theme functions - http://drupal.org/node/1354#themeable.

So for this preprocess function, this should suffice:

  * @param array $variables
  *   An associative array containing:
  *   - form: A render element representing the form.

Like it!
looks totally reasonable, as I go through some of the other views stuff, I'll keep this in mind.
rolling new patch and innerdiff shortly.

@webthingee nice work, you did have the "Default template: ..." bit in the right place above the variables before.

Yeah that naming of the template file is strange... not sure if it should be views-view- or just views- maybe somebody #drupal-vdc can sort that out?

That's a good point, I think I would be happy for this to change to views-view-exposed-form, then we just have views-more that breaks the convention (that could change in its conversion issue). Consistency is best :)

adding capitalization to the docblock for "Views" per @steveoliver's findings and IRC request.

holding off on testing... following latest convo

I think you need to rebase 8.x onto whatever branch you are working on, that's quite a large patch :) and #21 while you're there maybe....

@damiankloip,
yes I see that too...
I'll re-roll the whole thing once we have consensus on...
views-view-exposed-form

as that appears to be the final sticking point.

Sean,

Re #21: Go ahead with views-view-exposed-form.html.twig.

I agree consistency is best. views-view-exposed-form.html.twig.

Yes please :)

- re #26: updated views-view-exposed-form.html.twig.
- re #20: used order for docBlock

Nice work webthingee. Here are a few nitpicks/questions:

+++ b/core/modules/views/templates/views-exposed-form.html.twig
@@ -0,0 +1,85 @@
+    {% if items_per_page is defined %}
+      <div class="views-exposed-widget views-widget-per-page">
+        {{ items_per_page }}
+      </div>
+    {% endif %}
+    {% if offset is defined %}
+      <div class="views-exposed-widget views-widget-offset">
+        {{ offset }}
+      </div>
+    {% endif %}
+    <div class="views-exposed-widget views-submit-button">
+      {{ button }}
+    </div>
+    {% if reset_button is defined %}
+      <div class="views-exposed-widget views-reset-button">

'is defined' doesn't do anything unless strict variables is turned on. Closest thing I can see to if (!empty($items_per_page)) { is just {% if items_per_page %} if 0's need to be printable then {% if items_per_page is not empty %}

+++ b/core/modules/views/views.theme.inc
@@ -936,19 +936,25 @@ function template_preprocess_views_view_row_rss(&$vars) {
   $checkboxes = '';
 
   if (!empty($form['q'])) {
-    $vars['q'] = drupal_render($form['q']);
+    $variables['q'] = drupal_render($form['q']);
...........
+++ b/core/modules/views/views.theme.inc
@@ -984,24 +990,24 @@ function template_preprocess_views_exposed_form(&$vars) {
   if (isset($form['sort_by'])) {
-    $vars['sort_by'] = drupal_render($form['sort_by']);
-    $vars['sort_order'] = drupal_render($form['sort_order']);
+    $variables['sort_by'] = drupal_render($form['sort_by']);
+    $variables['sort_order'] = drupal_render($form['sort_order']);
   }
   if (isset($form['items_per_page'])) {
-    $vars['items_per_page'] = drupal_render($form['items_per_page']);
+    $variables['items_per_page'] = drupal_render($form['items_per_page']);
   }
   if (isset($form['offset'])) {
-    $vars['offset'] = drupal_render($form['offset']);
+    $variables['offset'] = drupal_render($form['offset']);
   }
   if (isset($form['reset'])) {
-    $vars['reset_button'] = drupal_render($form['reset']);
+    $variables['reset_button'] = drupal_render($form['reset']);
   }
   // This includes the submit button.
-  $vars['button'] = drupal_render_children($form);
+  $variables['button'] = drupal_render_children($form);

Can we remove drupal_render()'s here? I know we want to, but I don't know if it will explode (getting shy from all the 'invalid render key' messages on my other issues:S)

I will be updating the template name, and addressing the items above.

Had a long conversation with steveoliver on the topic of 'is defined', here's what I believe we arrived at given our key three options are... is not empty, is defined, and is not null.

If it's a string, we should use 'is not empty' allowing the value can be 0 and still pass the check.

If it's an array, and is generated by isset(), then use of 'is defined' allowing the variable (an array) to only be used if it is defined.

If it's a boolean, we would use 'is not null', which makes sure that the value is either 1 or 0, and that it is defined.

some tests...
We tested using the label in this view, because it is set to NULL by default, it HAS a value so {% if widget.label %} works because there is always a defined variable.

Using {% if items_per_page %} might result in an error because it is not set to null be default, it is only checking if it is set 'isset($form['items_per_page']', which means it could be undefined, and if the template asks to check for the variable, it would not exist (it does not default to NULL, it just doesn't exist).

@webthingee thanks for the explainations, I am quite clear on 'is not empty' but still shaky on 'is defined' because the docs and #symfony irc guys seem to indicate it doesn't do much with 'strict variables' turned off like we have it. @see http://twig.sensiolabs.org/doc/tests/defined.html and the boolean bit seems a bit strange as it should already be a boolean and not NULL.

For the rename of views-exposed-form.html.twig, it may need something like this attached patch... this may be a bit too much for the rename, so if this fails let's go back to #29.

Cus I like pain... let's see what's happens when I remove drupal_render()s and those 'is defined'.

@webthingee, Don't need to use this, I am curious if it'll pass. Also, left one drupal_render() in because it was throwing some undefined index exceptions. So if by miracle this passes, you may be able to check that one out?

And there is a tab fix on the twig file in there.

it passed.. :D

@joelputtet

#34 throws ajax errors... not sure why..
I tested #33 and that seems to work well.

@webthingee - can you provide steps to reproduce those ajax errors please?

Looks like I have some other issues as well.. the button throwing an error with the template change.
Going to take a more in-depth pass at this later today.

some confusion on the patch order..
is this apply 29, then 33 ?

~ this is a different workflow than I have used, just need to make sure i'm on the right page.

I have been looking at this for a few days now and I think we went a bit astray. (trust in earl :) ) The naming convention that already exists actually makes perfect sense.

views-view-X refers to the views templates that actually render a view (or part of the actual view).
views-X refers to the views templates that support a view, but are not a view.

so views-more.tpl and views-exposed-form.tpl are the correct/intended usage.

rolling a new patch, with a innerdiff against 8.x prior to variable, name, and changing of the naming convention.
needed a fresher start...

hope this is a good one :D

@webthingee, great work!

Tagging.

Review for #40:

+ * - widgets: An array of exposed form widgets. Each widget contains:
+ *   - label: The sanitized label of the widget, or NULL.
+ *   - id: The id of the widget, or an empty string.
+ *   - operator: The rendered select element for the operator widget,
+ *      or NULL.
+ *   - description: The sanitized description of the widget, or NULL.

According to [#1823416] we don't reference the PHP data type of variables in Twig template documentation:

Variable definitions should not include any information about the type of variable (array, object, string) since people working in template files should not have to care about type. Everything that can be printed in a Twig template will end up printing as a string.

We also shouldn't be using "is defined" to check if something can be printed:

You do not need to use 'is defined' after your variable to determine if it is available.

You also need some @see template_preprocess_views_exposed_form() style comments in the template.

@webthingee - interested in making the tweaks @thedavidmeister mentioned? If not, please unassign.

Tagging as Novice, if we don't hear back from @webthingee anyone can take a look at the tweaks mentioned in #43.

@steveoliver and I talked these over a few weeks ago. Not sure about the documentation part, I can surly clean that up, leaving the 'this or that' in place without specifying the type of variable, however, because in these views templates there are values that exist, but are not defined, it is possible to end up with some warnings and/or errors if we do not ensure that they exist && they are defined.

Maybe @steveoliver can help me out on this as I think we do need the 'is defined' for some of these varialbes, based on our conversations and walking through the code.

I have some notes in #32.

Going to try and push this forward a bit. Removed drupal_render() calls. Reverted $vars to $variables because we have an issue for that. #1963942: Change all instances of $vars to $variables

And removed the 'is defined' because that's checking if it's isset()/array_key_exists() and original was doing !empty() which is closer to {% if var %} than {% if var is defined %}

Did some doc cleanup for variable types.

Added {{ index }} which used to be $id for the widget row index, which was missing.

#46 - If there's a situation where we have documented variables in the Twig template but using them throws errors without having to do "is X" checks on them first, it's probably better to provide those variables with fallback falsey values in the preprocess to make the errors go away and keep the Twig template clean.

IMO if we're documenting a variable in the Twig template, it needs to be already usable without jumping through hoops.

Did some manual testing and code review for the patch in #47

1. I setup a simple exposed filter for a view. It appears that the new Twig related template is duplicated the form elements for the exposed filter (for me at least). See before patch screenshot and after patch screenshot:
and

2. Looks like these debug() lines were left in the patch by accident

+++ b/core/modules/views/views.theme.incundefined
@@ -936,39 +936,46 @@ function template_preprocess_views_view_row_rss(&$vars) {
+    debug($form[$info['value']]['#id']);

+++ b/core/modules/views/views.theme.incundefined
@@ -936,39 +936,46 @@ function template_preprocess_views_view_row_rss(&$vars) {
+    debug($form[$info['value']]['#id']);

3. Looks like #1963942: Change all instances of $vars to $variables is postponed and the associated patch will be re-rolled. I guess it is okay to lave $vars in there, but if we are making a new patch it seems they can be fixed now if desired.

This patch addresses the following (including some of my findings in #49):

1. Fixed issue where duplicate form fields are rendered with the submit button. There doesn't appear to be any reason to re-render form array elements since they are all being called specifically by the template.
2. Removed a couple of debug() calls left by accident in a previous patch.
3. Changed instances of $vars to $variables in the related Views theme function. #1963942: Change all instances of $vars to $variables is postponed so I don't think it hurts to change these while we are working in the area.
4. Removed views-mini-pager.html.twig template since it doesn't seem to be doing anything. The mini pagers are rendering directly from the theme function. This doesn't seem to be related to this issue and may need to be created as a new and separate issue.

#1963942: Change all instances of $vars to $variables was split into its own issue because it was making these patches harder to review. Can we please roll back the vars -> variables change?

Also, this might be of interest: #1963978-12: Convert theme_views_ui_build_group_filter_form() to Twig.

added meta

Patch that reverts $variables to $vars.

@thund3rbox something went bizarre with that patch and interdiff. The interdiff is larger than the patch. And you may have reverted a $variables back to $vars on something that was already fixed as $variables in core. Have a peak, the one on "theme_views_form_views_form" should be left alone, just reverting the ones I did on _views_exposed_form functions.

We will do that change wholesale in another patch and leave it out of the twig conversion:
#1963942: Change all instances of $vars to $variables

Thanks for taking a stab at this one, take another one:)

@joelpittet. I goofed. Here is a better patch..

This patch changes $variables to $var in instances ONLY from patch in #51. (template_preprocess_views_exposed_form function)

Someday, I'll get the hang of this... My apologies for confusion. Let me know of any issues.

EDIT: yes, the interdiff is wrong. But thanks to @Cottser's guidance, I know process for next time.

This looks RTBC to me but is effected by #1992358: [Policy] Decide on capitalisation of the "v" in "view(s)" when talking about views objects/module in documentation.

sorry, nitpick:

+ * - id: The id of the widget, or an empty string.

Should be "The ID of the widget" (capital ID).

RTBC from me after that though :)

Put the empty string back to NULL as that should render propertly now that #1975462: Allow and test for NULL and integer 0 values in Twig templates. is in.
Put the V's back to v.
And made id in the docblock ID as per #58

Thanks @thedavidmeister and @thund3rbox!

Wait, not RTBC

+ * - id: The ID of the widget, or an empty string.

no such thing as "empty string" in Twig template docs.

Tweaked from #60

cool.

cool.

Tagging for profiling.

I'll profile this one.

So if you didn't already started profiling, your lucky because now we need new ones :)

Uuurgh, looked like we ran into some duplicated variables here. Removed the unnecessary variables from preprocess and adjusted the template to use form.whatever. Docs are adjusted too.
This could use manual testing, not sure if I catched every usecase.

Few documentation issues, thx cottser :)

Did markup comparison again, passed visual diff and DaisyDiff perfectly.

Here is a profile for 3 exposed filters on a view and it's faster because we made preprocess much simpler.

Great work @Hydra and everyone on this issue. RTBC!

=== views-exposed-form-1843740-68..views-exposed-form-1843740-68 compared (519af3b0bf970..519af56893788):

ct  : 71,051|71,315|264|0.4%
wt  : 388,395|387,843|-552|-0.1%
cpu : 368,533|368,060|-473|-0.1%
mu  : 21,112,312|21,176,104|63,792|0.3%
pmu : 21,275,432|21,340,104|64,672|0.3%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=519af3b0bf970&...

=== views-exposed-form-1843740-68..views-exposed-form-1843740-68 compared (519af3b0bf970..519af59f030f2):

ct  : 71,051|71,315|264|0.4%
wt  : 388,395|385,889|-2,506|-0.6%
cpu : 368,533|366,014|-2,519|-0.7%
mu  : 21,112,312|21,176,104|63,792|0.3%
pmu : 21,275,432|21,340,104|64,672|0.3%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=519af3b0bf970&...

+1. Ready for #1987510: [meta] Convert all core *.tpl.php templates to Twig as singular patch

I assume that was meant to be closed (duplicate) like the other issues of this type.

Committed 19613d9 and pushed to 8.x. Thanks!