Convert theme_views_ui_build_group_filter_form() to Twig

Split from meta

Damn it's like tag caching or something... get back in there!

Wrong twig file from split.

Reroll of #5 - please make sure I did this correctly as this is my first contribution.

Review for #7:

Awful lot of undocumented variables with @todo in the Twig template here. Not sure if this should prevent an RTBC status considering the nature of the patch but I've added a "@todo clean-up" tag.

+ * - table: A rendered table element of the group filter form..

Don't need the double dot at the end.

+ '#attributes' => array('class' => array('views-filter-groups'), 'id' => 'views-filter-groups'),

This line is part of a multi-line array and it is over 80 characters itself. Looks like it should probably be split over multiple lines itself.

+ 'class' => array('views-hidden', 'views-button-remove', 'views-groups-remove-link', 'views-remove-link'),

+ $variables['table']['#rows'][] = array('data' => $data, 'id' => 'views-row-' . $group_id, 'class' => array('draggable'));

More long arrays that could be split across multiple lines.

+ $remove_link = array(

Indentation is wrong here.

+ '#theme' => 'link',

Including this means that this patch can't be RTBC until #1922454: Gut l(), fix theme('link'), have a working framework for consistently generated links inside and outside of Twig lands.

This patch is tagged as "needs manual testing" but there are no manual testing steps included in the issue summary - these need to be written and somebody needs to do the manual testing.

Did the doc cleanup from #9 thank you @thedavidmeister! :)

+++ b/core/modules/views/views_ui/views_ui.theme.incundefined
@@ -115,40 +115,68 @@ function theme_views_ui_expose_filter_form($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']);
+
   if (isset($form['required'])) {
-    $output .= drupal_render($form['required']);
+    $variables['required'] = $form['required'];
+    unset($form['required']);
   }
...
+  if (isset($form['operator'])) {
+    $variables['operator'] = $form['operator'];
+    unset($form['operator']);
...
+  if (isset($form['value'])) {
+    $variables['value'] = $form['value'];
+    unset($form['value']);
...
+  $variables['optional'] = $form['optional'];
+  $variables['remember'] = $form['remember'];
+  unset($form['optional']);
+  unset($form['remember']);
+
+  $variables['widget'] = $form['widget'];
+  $variables['label'] = $form['label'];
+  $variables['description'] = $form['description'];
+  unset($form['widget']);
+  unset($form['label']);
+  unset($form['description']);

Instead of all of this, why not just:

$keys = array(
  'form_description',
  'expose_button',
  'group_button',
  'required',
  'operator',
  'value',
  'optional',
  'remember',
  'widget',
  'label',
  'description',
);
foreach ($keys as $key) {
  if (isset($form[$key])) {
    $variables[$key] = $form[$key];
    unset($form[$key]);
  }
}

+++ b/core/modules/views/views_ui/views_ui.theme.incundefined
@@ -168,20 +198,69 @@ function theme_views_ui_build_group_filter_form($variables) {
+  unset($form['group_items']);
+  unset($form['default_group']);
+  unset($form['default_group_multiple']);
+
+  $variables['add_group'] = $form['add_group'];
+  $variables['more'] = $form['more'];
+  unset($form['add_group']);
+  unset($form['more']);

Oh these too.

See #1984850: Convert FilterPluginBase::build_group_form to use table rendering and remove theme function. It's likely that all views ui forms will get a conversion issue. There have been a few already.

Rerolled the patch

Refactored this to clean it up from the older patch. Fixed #theme link which is now #type=>link.

Did most of the @todo's but left a couple in there. Maybe someone can test this and fix those @todo.

Rerolling the patch...

Here is #19 rerolled.

1. +++ b/core/modules/views_ui/templates/views-ui-build-group-filter-form.html.twig
   @@ -0,0 +1,71 @@
   +{{ form|without(
   +  'form_description',
   +  'expose_button',
   +  'group_button',
   +  'required',
   +  'operator',
   +  'value',
   +  'optional',
   +  'remember',
   +  'widget',
   +  'label',
   +  'description',
   +  'add_group',
   +  'more',
   +  'group_items',
   +  'default_group',
   +  'default_group_multiple'
   +  )
   +}}
   +
   +{{ table }}
   

   Just curious, what renders the actual thing then? (not all of those are printed out earlier)

2. +++ b/core/modules/views_ui/views_ui.theme.inc
   @@ -160,15 +144,57 @@ function theme_views_ui_build_group_filter_form($variables) {
   +      '#attributes' => array(
   +        'id' => 'views-remove-link-' . $group_id,
   +        'class' => array(
   +          'views-hidden',
   +          'views-button-remove',
   +          'views-groups-remove-link',
   +          'views-remove-link',
   +        ),
   +        'alt' => t('Remove this item'),
   

   Someone really has to write proper tests and use data instead.

@dawehner thanks for the review, we are having an issue right now trying to see this template.

We think it may be here:

But we can't open that link. Maybe it's broken? Is there another way to get at this form through the UI?

RE:
#24.1 The only 3 that are excluded there are in the table. I could be very wrong there as that table build up is a bit strange using #rows where as #type=>table usually doesn't specify #rows those are the child elements.

#24.2 I agree it should be using data attributes, this is however just a conversion issue we can create a follow-up to address that.

This seems to be related to #2168893: Views filters groups adding and removing is broken

Rerolled. And left the remove link structure more or less alone.

The add group button still doesn't work before or after this patch, but everything else seems to with manual testing.

Also instead of using |without on the items that are built up in the table and not printed in the template explictely I hide() on them the preprocess so they are easy to spot.

Maybe we can just put this in without that group filter things fixed? The related fix for that doesn't seem to be touching any template logic.

Whoops @todo cleanup is still a thing, gotta figure out what those things are...

This ones just needs to get green again, the bugs in the JS look to be resolved in the RTBC #2168893: Views filters groups adding and removing is broken

Whoops, poor re-roll from #30 I don't trust myself at this hour.

I start by trying to fix the syntax error.

Hope it works!
Although I think possibly errors in the test.

@dimaro thanks for getting it to not have that syntax error, I managed to make another one here I believe from the test results:

+++ b/core/modules/views_ui/templates/views-ui-build-group-filter-form.html.twig
@@ -0,0 +1,69 @@
+    'more',
+  )

This is like PHP where it doesn't like that ending comma in a function/filter.

@dimaro, Do you want to see how many fails/exceptions this fixes? If you have any idea what those twig variables with @todo actually are could you attempt filling out the ones you know? That would be a huge help! Views in D7 is lacking for their documentation mostly because it's admin facing.

Seeing as I was mucking around with the views filters, I wanted this completed so that I could properly theme it in my theme.

So basically finished this up.

forgot to include the one file in my patch.

@NickWilde thank you very much for tackling one of few theme functions left. Just a couple of questions and nitpiks:

Also thanks for finishing all those @todos, I really didn't know what half of them were:)

1. +++ b/core/modules/views_ui/js/views-admin.js
   @@ -836,14 +836,14 @@
   -          $('input.default-radios').hide();
   +          $('input.default-radios').parent().hide();
   ...
   -          $('input.default-checkboxes').show();
   +          $('input.default-checkboxes').parent().show();
   ...
   -          $('input.default-checkboxes').hide();
   +          $('input.default-checkboxes').parent().hide()
   ...
   -          $('input.default-radios').show();
   +          $('input.default-radios').parent().show();
   

   This looks a bit out of scope can you explain this change?

2. +++ b/core/modules/views_ui/templates/views-ui-build-group-filter-form.html.twig
   @@ -0,0 +1,69 @@
   + *   - multiple:  A checkbox to allow multiple filter values.
   

   Nit: Extra space before the A

3. +++ b/core/modules/views_ui/templates/views-ui-build-group-filter-form.html.twig
   @@ -0,0 +1,69 @@
   +{# Render the rest of the form elements excluding elements that are printed elsewhere. #}
   

   Should wrap this on two lines for the 80 character coding standard for comments.

4. +++ b/core/modules/views_ui/templates/views-ui-build-group-filter-form.html.twig
   @@ -0,0 +1,69 @@
   +{{ form.more }}
   \ No newline at end of file
   

   Need a new line at end.

5. +++ b/core/modules/views_ui/views_ui.theme.inc
   @@ -91,35 +91,23 @@ function template_preprocess_views_ui_view_info(&$variables) {
   +  $form_state = new FormState();
   +  $form['default_group'] = Element\Radios::processRadios($form['default_group'], $form_state, $form);
   +  $form['default_group_multiple'] = Element\Checkboxes::processCheckboxes($form['default_group_multiple'], $form_state, $form);
   +  $form['default_group']['All']['#title'] = '';
   
   @@ -128,13 +116,6 @@ function theme_views_ui_build_group_filter_form($variables) {
   -  $form_state = new FormState();
   -  $form['default_group'] = Element\Radios::processRadios($form['default_group'], $form_state, $form);
   -  $form['default_group_multiple'] = Element\Checkboxes::processCheckboxes($form['default_group_multiple'], $form_state, $form);
   -  $form['default_group']['All']['#title'] = '';
   

   Does this hunk need to change location?

6. +++ b/core/modules/views_ui/views_ui.theme.inc
   @@ -148,24 +129,26 @@ function theme_views_ui_build_group_filter_form($variables) {
   +      'default' => [
   +        'data' => [
   

   We don't need to do the array syntax changes that affect this patch size. We try to make that change on only parts of the patch that are changing so that it's easy to review the patch without doing the mental gymnastics around what is a syntax change and what is critical change part of the patch. (loose rule)

Great work and green patches are always nice to see!

@joelpittet

Well I like to have good documentation. At least when I'm editing code... writing new code I tend to document on the slim side of what I will like if I edit the area later. Nit picks are always appreciated :)

1. That really really is in needed - might be out of scope slightly but if so it should be done as a seperate patch. That change hides the div with the label and the checkbox or radio button for the default selection - without that change it was displaying two labels and either the checkbox or the radio button (in the table of options)

2. fixed

3. fixed - it was from the old patch and didn't notice the length of it.

4. fixed

5. Not really. Fixed

6. that's from inherited from the older versions, should I change that out?

Thanks;
Nick

@NickWilde thanks for the quick fix.

Re #46.5 probably my bad :P, but yes please do. You can keep the short syntax in hunks that have changed but otherwise please revert, the committers will thank you and the reviewers too:)

#46.1 Hmm, I wonder if that is being taken care of already in #2168893: Views filters groups adding and removing is broken? Or at least that sounds like the last comments were referring to? Regardless, let's take that out so it doesn't block this patch from going into core. Maybe create a follow-up issue for that and let us all know about it so we can test and push that fix in on the JS.

Couple more things, the fixes look great thanks again!

1. +++ b/core/modules/views_ui/templates/views-ui-build-group-filter-form.html.twig
   @@ -0,0 +1,72 @@
   + *   - remember: A checkbox to remember selected filter value (per user).
   

   "... selected filter values ..." or "... the selected filter value ..."? Needs plural or an article qualifier.

2. +++ b/core/modules/views_ui/views_ui.theme.inc
   @@ -91,35 +91,18 @@ function template_preprocess_views_ui_view_info(&$variables) {
   +  /* Prepare table of options */
   
   @@ -129,12 +112,12 @@ function theme_views_ui_build_group_filter_form($variables) {
   +  /*prepare default selectors*/
   

   Both of these should be inline comments. //
   And have space before the comment, uppercase the first character in the sentence and period at the end as per our coding standards.

#46.1 reverted that on the lines that was the only change
#46.2 removed and I'll look into doing it as a separate patch

#48.1 fixed (also removed another variable description as it doesn't actually exist in this form).
#48.2 fixed

Thanks;
Nick

I just regenerated the patch and got the same content. Do you think that is a testbot failure? or something I need to deal with? With those changes it is running fine in usage.
Thanks
Nick

1. indented.
2. ordered to be the same as they are printed. (and removed a couple of invalid ones while I was examining that part)
Thanks;
Nick

+++ b/core/modules/views_ui/templates/views-ui-build-group-filter-form.html.twig
@@ -0,0 +1,71 @@
+    'identifier'

oops

The latest patch says:

patch: **** malformed patch at line 75:

When I try to apply it. Did you maybe try to remove something directly from the patch file instead of creating a new patch?

It says the new html.twig file is @@ -0,0 +1,71 @@ meaning 71 lines, yet there are only 68 lines in the patch from my count.

It's possible to remove lines from a patch file but the numbers there need to change too. IMO it's not worth the hassle, let git do it.

oh good job Nick... I always use git (1/2 dozen commits make up this patch on my end). Except for this once when it was all done except that tiny change and then I go and forget to change the line numbers whooo.
fixed numbers attached.

Thank you again @NickWilde. I'd likely do that manual patch edit and post it too, no worries;) I think we just need someone to do a bit of manual testing now. It's a bit tricky since it's kind of broken. The JS patch in #2168893: Views filters groups adding and removing is broken and that other JS code you had would help the manual testing along.

We just need to confirm the markup is identical before and after this patch. And that it still works.

To that task there is one more thing I spotted:

+++ b/core/modules/views_ui/views_ui.theme.inc
@@ -150,22 +133,24 @@ function theme_views_ui_build_group_filter_form($variables) {
+          $form['group_items'][$group_id]['remove'],
           '#markup' => drupal_render($form['group_items'][$group_id]['remove']) . \Drupal::l(SafeMarkup::format('<span>@text</span>', array('@text' => t('Remove'))), Url::fromRoute('<none>', [], array('attributes' => array('id' => 'views-remove-link-' . $group_id, 'class' => array('views-hidden', 'views-button-remove', 'views-groups-remove-link', 'views-remove-link'), 'alt' => t('Remove this item'), 'title' => t('Remove this item'))))),

This looks as it would cause a dup, since we aren't converting the drupal_render in this hunk. I think the critical is dealing with this so we can remove this added line.

hmm that is a line I didn't add so I'm not actually sure on it. I'll do some manual testing and checks into removing it and functionality testing too.
(was away yesterday)

alrighty did some testing.

We just need to confirm the markup is identical before and after this patch. And that it still works.

Markup is *not* identical. I intentionally made some changes. See screenshot for the example of messed up spot that is fixed with this partch. All functionality works as well or slightly better than before - still need some javascript work but that is seperate from this.

of course it would be good if I uploaded the right patch.

I have now uploaded that JavaScript improvement that was in an earlier version of the patch to a new issue

Adding the other issue for dragging and dropping. #2493945: Views filters dragging in the grouping view is broken

Re-rolled.

Removed clearfix, no longer in head.

Removed a bunch of things from the template that weren't in the theme function anymore.

Remove the last drupal_render() early render call from there and make the unset()'s more specific so they don't accidentally remove something they shouldn't from the form.

Missed a few more drupal_render() calls. That should be the last of them.

I've done a manual test and it looks like currently without the patch that form is quite broken. Also some of the screenshots I took above are totally the wrong form (those were for reorganizing the filters, while this is for grouping exposed filters)

Attached is whitespace normalized before and after with twig debug turned on so I know it's the right template this time.

Before:

After:

Maybe someone can confirm my findings here and RTBC?

It looks like the radios are just printed twice before this patch fixes that.

I am testing this and do a little code cleanup.

@joelpittet Look good to me, radios are printed once.

I am uploading a patch including:
- Remove unused use statements at views_ui.module
- Switch to the short array syntax.

This patch went from 7K to 32K somehow...

+++ b/core/modules/views_ui/views_ui.module
@@ -24,15 +20,15 @@ function views_ui_help($route_name, RouteMatchInterface $route_match) {
-      $output .= '<p>' . t('The Views UI module provides an interface for managing views for the <a href="@views">Views module</a>. For more information, see the <a href="@handbook">online documentation for the Views UI module</a>.', array('@views' => \Drupal::url('help.page', array('name' => 'views')), '@handbook' => 'https://www.drupal.org/documentation/modules/views_ui')) . '</p>';
+      $output .= '<p>' . t('The Views UI module provides an interface for managing views for the <a href="@views">Views module</a>. For more information, see the <a href="@handbook">online documentation for the Views UI module</a>.', ['@views' => \Drupal::url('help.page', ['name' => 'views']), '@handbook' => 'https://www.drupal.org/documentation/modules/views_ui']) . '</p>';

Changing lines like this is out of scope, please revert.

Thanks for the manual testing @jmolivas.

Yeah we don't go out of our way to make changes unrelated to the task or it's a) Hard to review the changes b) Could unintentionally make patches on the same file need to re-roll. I like short syntax as much as anybody but we need to be tactical on that kind of change;)

Re-posting #77. Let me know if there is anything in #84 that made the conversion to Twig better?

@joelpittet Besides the short syntax at #84 I did also remove these unused use statements:

 use Drupal\Component\Utility\Unicode;
 use Drupal\Core\Routing\RouteMatchInterface;
 use Drupal\Core\Url;
-use Drupal\views\Views;
 use Drupal\views\ViewExecutable;
-use Drupal\views_ui\ViewUI;
 use Drupal\views\Analyzer;
-use Drupal\Core\Ajax\AjaxResponse;
-use Drupal\Core\Ajax\ReplaceCommand;
 use Drupal\Component\Utility\Xss;

@joelpittet Also thanks for reposting #77 and remove my mess ;)

@jmolivas I don't see those use lines in my version of head, maybe your working directory is behind?

This is all I see:

use Drupal\Component\Utility\SafeMarkup;
use Drupal\Core\Form\FormState;
use Drupal\Core\Render\Element;
use Drupal\Core\Url;

@joelpittet I was talking about views_ui.module I did the changes since the file was already updated by the patch, but may be this is also out of scope.

I'd say it may be out of scope because we aren't removing any of those use in the code below, but maybe @tim.plunkett has an opinion on the matter?

*Putting my hands up to look like a scale* Those removals end up in really small patch issues "Remove unused use statements in blah file". Which is also not fun for committers...

@joelpittet: Maybe not fun but needed to get fixed but if not good for a new issue, but also not good when file is already updated with a patch when this can happen.

@jmolivas maybe just add it here and we'll see if it gets kickback;)

I will and wait

joelpittet: Removing unused use statements at views_ui.module and just waiting to see if it gets kickback;)

cottser: I updated the docs trying to follow your recommendation at #100

Maybe not fun but needed to get fixed

Not really. No one will get hurt if it didn't. Very minor stuff :) That tangent wasted a couple of days...

On the plus side, the manual testing really is appreciated.

@damiankloip Rather a couple of days discussing the issue and learning about core contribution stuff than months++ sitting stagnant. It helps a crap ton just to have eyeballs!

For my motivation too!

Uploading fixes to the doc suggested by @cottser since seems like files were deleted on #102

@joelpittet agree. Eyeballs are king in these here issue queues!

Same as patch in #105 but with @Cottser's doc improvements from #107 applied. (ie no other changes and I'm not making an interdiff for those two lines of documentation)

Back to RTBC

Nice twigifying. It's really nice to be getting rid of all the calls to drupal_render(). Committed 08baa72 and pushed to 8.0.x. Thanks!