Views filters groups adding and removing is broken

Confirmed.

Thank you for reporting the issue. I consider this as a major bug, at least.
Also updating some tags.

There are several bugs:

* The actual add functionality does not work (even if you show the button itself)
* The button is hidden from the no-JS variant

Given that the functionality is pretty useless so this is a huge regression compared to d7.

There are bugs in the javascript and php files too.

I found it in javascript and i will post a patch file today, but in php file, concrete in core/modules/views_ui/lib/Drupal/views_ui/Form/Ajax/RearrangeFilter.php, we can't use logic like in Drupal 7.

For example, there is no $form_state['clicked_button'] element and there are more bugs.

It's useless but it's a self-contained bug in the views UI, so major.

In this patch I fixed javascript bug for click on "Create new filter group" link.
It work now but there is a problem because it doesn't trigger ajax. It opens new page with JSON output and when I go back and click again on rearrange the filters, new filter group is shown. I can't figure out why this happens.

Please test it and if someone has idea how to solve the problem, that would be great.

The JS error is because event is not a defined variable in that scope. function definition needs to be changed to clickAddGroupButton: function (event) { no need for return false;.

I don't know why the ajax stuff doesn't trigger though.

The input clicked is not ajax enabled so it wouldn't do anything.

Still doesn't work but at least no JS error.

this.addGroupButton.trigger('mousedown').trigger('submit'); doesn't work. I putted this.addGroupButton.click(); and then event.preventDefault(); and then it fire button.

Also, by adding class 'use-ajax', button for add group get class 'ajax-processed' but on submit it still open page with JSON ouput.

I noticed that other buttons on view has class 'drupal-ajax-processed'.

About the json output we have the same behavior for d7 using .click(). If we add the #ajax attribute with a 'path' for example, we obtain the class 'drupal-ajax-processed' added to the input. Also the form submit is executed as expected but for the moment this is where I am. Maybe this info is helpful .

@marthinal Your post was very helpful. Thank you.

I needed to change some other lines in PHP too, like $form_state['view']->addFormToStack('rearrange-filter', $form_state['display_id']); and it wirks for me now.

Same problem appears with removing filter groups.

1. +++ b/core/modules/views_ui/js/views-admin.js
   @@ -519,7 +519,7 @@
   -    clickAddGroupButton: function () {
   +    clickAddGroupButton: function (event) {
          // Due to conflicts between Drupal core's AJAX system and the Views AJAX
          // system, the only way to get this to work seems to be to trigger both the
          // mousedown and submit events.
   

   WOW, we need javascript tests!

2. +++ b/core/modules/views_ui/lib/Drupal/views_ui/Form/Ajax/RearrangeFilter.php
   @@ -199,12 +206,18 @@ public function buildForm(array $form, array &$form_state) {
   +    // Get form path
   +    $form_path = empty($form_state['path']) ? current_path() : $form_state['path'];
   +
   

   It is certainly wrong to still rely on current_path(), isn't there something on the request object which does the same?

I tried to find path on the request object but no results.

So, we need to replace current_path in whole views system, right?

// Get requestUri from request object.
    $requestUri = $this->getRequest()->getRequestUri();

I don't know is this right solution but i am getting path that I need. Please advice.

Rerolled and reviewed. JS file is fixed in HEAD now. Looks fine, only not sure why we need to remove "filter" from

-      $form_state['view']->addFormToStack('rearrange-filter', $form_state['display_id'], 'filter');
+      $form_state['view']->addFormToStack('rearrange-filter', $form_state['display_id']);

Other than that I think RTBC.

@#23

The 'filter' appears to be for a mandatory parameter of $type.

public ViewUI::addFormToStack($key, $display_id, $type, $id = NULL, $top = FALSE, $rebuild_keys = FALSE)
https://api.drupal.org/api/drupal/core%21modules%21views_ui%21src%21View...

@#23

I am seeing these console errors when clicking the "Create new filter group" and nothing happens.

POST http://sandbox/d8/d8/admin/structure/views/ajax/rearrange-filter/345/default 404 (Not Found) jquery.js?v=2.1.0:8556
jQuery.ajaxTransport.send jquery.js?v=2.1.0:8556
jQuery.extend.ajax jquery.js?v=2.1.0:8084
$.fn.ajaxSubmit jquery.form.js?v=3.5:257
Drupal.ajax.eventResponse ajax.js?v=8.0-dev:342
(anonymous function) ajax.js?v=8.0-dev:263
jQuery.event.dispatch jquery.js?v=2.1.0:4371
jQuery.event.add.elemData.handle jquery.js?v=2.1.0:4057
jQuery.event.trigger jquery.js?v=2.1.0:4286
(anonymous function) jquery.js?v=2.1.0:4828
jQuery.extend.each jquery.js?v=2.1.0:381
jQuery.fn.jQuery.each jquery.js?v=2.1.0:137
jQuery.fn.extend.trigger jquery.js?v=2.1.0:4827
$.extend.clickAddGroupButton views-admin.js?v=8.0-dev:531
jQuery.extend.proxy.proxy jquery.js?v=2.1.0:516
jQuery.event.dispatch jquery.js?v=2.1.0:4371
jQuery.event.add.elemData.handle

Uncaught object ajax.js?v=8.0-dev:552
Drupal.ajax.error ajax.js?v=8.0-dev:552
Drupal.ajax.ajax.options.complete ajax.js?v=8.0-dev:247
$.fn.ajaxSubmit.options.complete jquery.form.js?v=3.5:219
jQuery.Callbacks.fire jquery.js?v=2.1.0:3047
jQuery.Callbacks.self.fireWith jquery.js?v=2.1.0:3159
done jquery.js?v=2.1.0:8198
jQuery.ajaxTransport.send.callback

My actual path is http://sandbox/d8/admin and not http://sandbox/d8/d8/admin like it is searching for, and there are multiple paths of http://sandbox/d8/d8 in the network tab. I don't have time to debug this just yet but I think this is the main issue in my case of having Drupal installed in a subdirectory.

Here is a patch that uses getPathInfo() instead of getRequestUri and removes the subdirectory from the URL. It also adds 'filter' back.

Now on first click it does add a new filter group but on subsequent clicks it throws an error.

Okay, some work that still needs to be done.

1. The cancel button doesn't work
2. Subsequent "create new filter groups" throw the console error and don't do anything of course. POST http://sandbox/d8/admin/structure/views/ajax/rearrange-filter/345/default/filter 404 (Not Found)

Rerolled the patch

Assigning to @nod_ because maybe he can clear up why the JS isn't doing the ajax requests correctly and closing the window once called or maybe point us in the right direction here.

Selfishly this is holding up our twig conversions but also it's quite a big bug for beta as well.
#1963978: Convert theme_views_ui_build_group_filter_form() to Twig

Looked into it it's out of my hands. The no-js version of adding a group doesn't work at all.

From what I understand the group configuration form doesn't get added to "the stack". It also means that when using JS there will be a CloseDialogCommand added to the ajax commands to close everything and start fresh. The JS bug is a side effect of a PHP problem I don't know how to fix.

I was reviewing this patch with @dawhener and found the problem.

1. +++ b/core/modules/views_ui/src/Form/Ajax/RearrangeFilter.php
   @@ -202,12 +209,23 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   +    $request_uri = $this->getRequest()->getPathInfo();
   

   We don't use this anywhere

2. +++ b/core/modules/views_ui/src/Form/Ajax/RearrangeFilter.php
   @@ -202,12 +209,23 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   +    $form_path = $form_state->get('path') ? current_path() : $form_state->get('path');
   

   This line doesn't make sense to me. First, why call it twice? Second, why use the result only if it casts to FALSE? Thirdly, current_path() isn't even a function any more.

This still doesn't seem to work quite right in manual testing. After adding a filter group, I can drag items in, but the group is not saved.

@tim.plunkett Done.

@jhedstrom I think you need to drag more to the bottom. I was testing and it works... weird.

thanks!

Confirmed that dragging more than one filter all the way to the bottom will allow the group to be saved, but surely there are use-cases for only adding one filter to a new group (eg, a single filter belonging to a group joined by OR).

Also, after the group is created, there's an escaping issue on the main form:

Actually, adding a single filter works if it is dragged all the way to the bottom. I interpreted #39 as meaning more than one :)

+++ b/core/modules/views_ui/src/Form/Ajax/RearrangeFilter.php
@@ -224,7 +216,7 @@ public function buildForm(array $form, FormStateInterface $form_state) {
+        'path' => \Drupal::request()->getRequestUri(),

$this->getRequest()->getRequestUri(), I guess

@tim.plunkett Yes thanks

Added related bug about double-escaped filters.

Removed duplicated code, fixed Create new group and remove group + avoid duplicate select for the operator.

Thanks to nod_ for the help with the js for the select for the operator :)

This works for me, I'll let tim or someone else confirm it.

I'm a bit confused as the inline template changes are missing?

This part is fixed here #2469563: Double-escape on Views filters

Both nod_ and marthinal have been testing this manually in Views. I also just ran it on simplytest.me, and it fixes the bug described in the issue summary: you can now add/remove groups in the Filter rearrange dialog.

I took a look at the code. The changes are straightforward and the code looks clean to me.

No tests because it's a JS bug. Sigh.

Anyway, let's get this bug fix in!

Quick beta eval: This fixes a Major UI Bug, and there are no API changes or disruption.

#2461531: ESlint 0.18.0 compatibility and new rule

@lauriii,

I think this is the wrong patch.

This should be the right one :) Sorry for the previous one!

Back to RTBC

I'm curious whether we can write a test for at least the second half of the problem (the part in PHP).

Good question.

@marthinal and @nod_, thoughts on possible tests for mostly #47?

We can create a group filter and then remove it. We don't need js enabled for it, so maybe this is enough to cover this part...

I can work on it.

I'm using the test I have created at #2469563: Double-escape on Views filters.

@marthinal that is awesome, thanks for separating out the test and pass patches.

There are some coding standards nitpicks and one not so nit pick outside the test, would you mind tackling them? I'd RTBC this again after these are tackled.

Also thanks for moving that todo move one too!

1. +++ b/core/modules/views_ui/js/views-admin.js
   @@ -551,10 +551,7 @@
   -      this.table.find('#' + event.data.buttonId).trigger('submit');
   +      this.table.find('input#' + event.data.buttonId).trigger('mousedown').trigger('submit');
   

   Can we avoid this overly specific selector? I had this as a pet peeve in D7 because I couldn't covert theme_button to a button element because these things in panels and views.

2. +++ b/core/modules/views_ui/src/Form/Ajax/RearrangeFilter.php
   @@ -128,6 +135,9 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   +          '#ajax' => array(
   +            'path' => $this->getRequest()->getRequestUri(),
   +          )
   

   Nit: Short array syntax :s/array()/[]/ and can be one line.

3. +++ b/core/modules/views_ui/src/Form/Ajax/RearrangeFilter.php
   @@ -203,11 +213,15 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   +      '#ajax' => array(
   +        'path' => $this->getRequest()->getRequestUri(),
   +      ),
   
   +++ b/core/modules/views_ui/src/Tests/FilterUITest.php
   @@ -0,0 +1,89 @@
   +
   +
   

   Ditto

4. +++ b/core/modules/views_ui/src/Tests/FilterUITest.php
   @@ -0,0 +1,89 @@
   +class FilterUITest extends ViewTestBase {
   +
   +
   

   Nit: Extra line break.

5. +++ b/core/modules/views_ui/src/Tests/FilterUITest.php
   @@ -0,0 +1,89 @@
   +  public static $testViews = array('test_filter_in_operator_ui', 'test_filter_groups');
   

   Nit: Short array syntax s/array()/[]/

6. +++ b/core/modules/views_ui/src/Tests/FilterUITest.php
   @@ -0,0 +1,89 @@
   +  public static $modules = array('views_ui', 'node');
   ...
   +    $this->drupalCreateContentType(array('type' => 'page'));
   

   Nit: here too [].

7. +++ b/core/modules/views_ui/src/Tests/FilterUITest.php
   @@ -0,0 +1,89 @@
   +   * Tests that "Limit list to selected items" option is saved as expected when editing
   +   * the Content type filter from the UI.
   

   Nit: Wrap on 80 chars.

8. +++ b/core/modules/views_ui/src/Tests/FilterUITest.php
   @@ -0,0 +1,89 @@
   +    $admin_user = $this->drupalCreateUser(array('administer views', 'administer site configuration'));
   

   Nit: Short array syntax s/array()/[]/

9. +++ b/core/modules/views_ui/src/Tests/FilterUITest.php
   @@ -0,0 +1,89 @@
   +    $edit = array(
   +      'options[expose][reduce]' => TRUE,
   +    );
   

   Nit: Short array syntax s/array()/[]/ and can be one line.

10. +++ b/core/modules/views_ui/src/Tests/FilterUITest.php
    @@ -0,0 +1,89 @@
    +    $admin_user = $this->drupalCreateUser(array('administer views', 'administer site configuration'));
    ...
    +    $this->drupalPostForm(NULL, array(), t('Create new filter group'));
    +    $this->drupalPostForm(NULL, array(), t('Create new filter group'));
    ...
    +    $this->drupalPostForm(NULL, array(), t('Remove group 3'));
    ...
    +    $this->drupalPostForm(NULL, array(), t('Remove group 3'));
    

    Nit: Short array syntax s/array()/[]/

11. +++ b/core/modules/views_ui/src/Tests/FilterUITest.php
    @@ -0,0 +1,89 @@
    +    $this->assertNoRaw('<span>Group 3</span>', 'Group 3 has not been added yet.');
    +
    +  }
    

    Nit: Extra line break.

Any changed/new diff hunks can get the short array syntax(don't go out of the hunks or it could cause needless rerolls on other patches and make the committers and reviews cross:P)

@joelpittet sure! and thanks for reviewing.:)

1. trigger('mousedown') is needed otherwise it does not work.

Here the same situation:

    clickAddGroupButton: function (event) {
      // Due to conflicts between Drupal core's AJAX system and the Views AJAX
      // system, the only way to get this to work seems to be to trigger both the
      // mousedown and submit events.
      this.addGroupButton
        .trigger('mousedown')
        .trigger('submit');
      event.preventDefault();
    },

The rest are fixed as well.

Oh I wasn't complaining about the mousedown. I'm sure that was needed however weird that looks. I was just mentioning the need for 'input' in the CSS selector.

It makes it hard to theme buttons by hardcoding what element that class is on.

Also missing a few ending commas, I'll identify them when I get into the office on dreditor.

1. +++ b/core/modules/views_ui/js/views-admin.js
   @@ -551,10 +551,7 @@
   +      this.table.find('input#' + event.data.buttonId).trigger('mousedown').trigger('submit');
   

   So %s/input#'/#'/

2. +++ b/core/modules/views_ui/js/views-admin.js
   @@ -567,7 +564,11 @@
   +      var titleRows = $('tr.views-group-title').once('duplicateGroupsOperator');
   

   Would love the same thing here but I don't know where views-group-title is used so we can leave the tr. on here.

3. +++ b/core/modules/views_ui/src/Form/Ajax/RearrangeFilter.php
   @@ -128,6 +135,9 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   +          '#ajax' => [
   +            'path' => $this->getRequest()->getRequestUri(),
   +          ]
   

   Needs a comma at the end, and can make that one line.

4. +++ b/core/modules/views_ui/src/Form/Ajax/RearrangeFilter.php
   @@ -203,11 +213,15 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   +      '#ajax' => [
   +        'path' => $this->getRequest()->getRequestUri(),
   +      ]
   

   Same here, one line and comma at the end.

5. +++ b/core/modules/views_ui/src/Tests/FilterUITest.php
   @@ -0,0 +1,90 @@
   +    $edit = [
   +      'options[expose][reduce]' => TRUE
   +    ];
   

   One line, OR at least if not needs a comma after TRUE.

1. Done. ( Sorry added while working on it :) )

2. Without removing tr for the moment.

3. Done.

4. Done.

5. Done

Thanks!

Awesome thank you @marthinal

I'm still seeing #38 with this patch. Do we have an issue to fix it?

+++ b/core/modules/views_ui/src/Form/Ajax/RearrangeFilter.php
@@ -128,7 +135,8 @@ public function buildForm(array $form, FormStateInterface $form_state) {
+          '#ajax' => ['path' => $this->getRequest()->getRequestUri()],

@@ -197,18 +205,20 @@ public function buildForm(array $form, FormStateInterface $form_state) {
+      '#ajax' => ['path' => $this->getRequest()->getRequestUri()],

We use 'url' now so these lines are same as '#ajax' => ['url' => NULL],, right? The 'use-ajax-submit' class with trigger('click') could also work here.

I'm not sure the answer to that @olli, so I'm needs reviewing this to see if someone knows?

For the moment rerolled... Not sure if we should use url instead of path. So needs review and this is a quick change anyways...

I can see info about it and I think that using url we avoid to call getRequest() method.

      // Assign default settings. When 'url' is set to NULL, ajax.js submits the
      // Ajax request to the same URL as the form or link destination is for
      // someone with JavaScript disabled. 

Thanks for #83. Seems to make sense about url.

in #70 @joelpittet said

Any changed/new diff hunks can get the short array syntax(don't go out of the hunks or it could cause needless rerolls on other patches and make the committers and reviews cross:P)

1. +++ b/core/modules/views_ui/src/Form/Ajax/RearrangeFilter.php
   @@ -120,7 +127,7 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   -        $form['remove_groups'][$id] = array(
   +        $form['remove_groups'][$id] = [
   

   we do not need to change this for the patch to work.

2. +++ b/core/modules/views_ui/src/Form/Ajax/RearrangeFilter.php
   @@ -197,18 +205,20 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   -        '#attributes' => array('class' => array('views-remove-checkbox')),
   +        '#attributes' => ['class' => ['views-remove-checkbox']],
   

   same.

3. +++ b/core/modules/views_ui/src/Form/Ajax/RearrangeFilter.php
   @@ -197,18 +205,20 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   -    $form['actions']['add_group'] = array(
   +
   +    $form['actions']['add_group'] = [
   

   same.

4. +++ b/core/modules/views_ui/src/Tests/FilterUITest.php
   @@ -73,6 +73,26 @@ public function testFiltersUI() {
   +        // Create 2 new groups.
   

   nit, comment indent is off.

Done!

Thanks! :)

@marthinal thank you for the remaining cleanup!

There's no confirmation that #38 is solved or is a followup.

@alexpott, you're right it does work but there just seems to be a bug left with two drop areas, not sure if that is a template level thing or just JS duplicating dom elements... so yes this does still needs work.

Will tag this as novice for someone with javascript skills can look at why this is the case.

When you create a new group there are two drop cells and the second one is where the filter needs to be dropped into that second group.

@joelpittet I think this is because the description is hidden. And not sure if it should be hidden...

Hidden:

Not hidden:

Interesting @marthinal could you post a patch with it unhidden? We can test it further. I don't know if it's supposed to be hidden and why that would effect the drop areas. Maybe a class name collision or something.

@joelpittet Before adding it to the patch I prefer to know if we want to hide it.

views-admin.js

      // Get rid of the explanatory text around the operator; its placement is
      // explanatory enough.
      this.operator.find('label').add('div.description').addClass('visually-hidden');

I was discussing with @lauriii about it and maybe we should open a new issue about this problem and fix this one.

@joelpittet Before adding it to the patch I prefer to know if we want to hide it.

views-admin.js

      // Get rid of the explanatory text around the operator; its placement is
      // explanatory enough.
      this.operator.find('label').add('div.description').addClass('visually-hidden');

I was discussing with @lauriii about it and maybe we should open a new issue about this problem and fix this one.

I created a follow up for broken dragging thing. I don't see how it would fit she scope of this issue. #2493945: Views filters dragging in the grouping view is broken

@marthinal Since you mentioned that unhiding it fixed the drag drop groups I was hoping you'd post a patch so we could review your findings. But yes this patch as it is now makes things kinda work more than it does so moving it to a follow-up seems reasonable considering the amount of time it took to get this far.

There is already solution suggested on the other issue and it relates to problem in the tabledrag - so no need to worry about that here :)

Okay lets fix the tabledrag in the other issue.

This issue addresses a major bug and is allowed per https://www.drupal.org/core/beta-changes. Committed d982616 and pushed to 8.0.x. Thanks!

This is also broken in the current release of Drupal 7, as described in the original bug report.

Looks like this is actually a conflict with the hide_submit module:
https://www.drupal.org/node/1939426

The original bug report was always about Drupal 8 (the early comments in this issue refer to it as a regression compared to Drupal 7).

Also, Views is not in Drupal 7 core.

I think you're probably right that the problem you're experiencing is covered by the issue you linked to. (In my experience, filter groups do work fine in Views for Drupal 7 in the general case.)

I'm creating a site in Drupal 8.1.8 and the filter grouping is not working for me. I can create the filter group and add items to it but it does not save. Is this broken again, or are there other modules that cause this to break?

@certifiedgeek you may have a new issue. First search open issues and if not found, open a new one and post a link here to that issue

#2493945: Views filters dragging in the grouping view is broken

@certifiedgeek
Same here with 8.1.10, but i have a lot of custom code and contrib modules (in various un-stable states) which could be causing it.
No errors in the JS console or watchdog to indicate a problem.

same here :( totally broken even after update to 8.2 ...

Same for me running Drupal 8.2.3

Same problem has comment #106 on drupal 8.2.3:
I can create new group, add field to it but after applying my changes, nothing is saved. Coming back to it, I find only one group...

Same for me -> filter groups are gone

Same for me --> #112
But with drupal 8.2.4 :(

Does anyone reopen this issue or do we have to create a new issue? I'm not waiting as long as my comment predecessors :)

There are a couple of issues that relate to this already in the queue, but if you don't see it in this list please create a new one.

#2215857: Behaviors get attached to removed forms
#1939426: I cannot Create new filter group
#2244489: Required exposed grouped filter remembers values when they should not
#2233695: optional exposed grouped filters on a View that requires exposed filters save their default values as remembered values when they shouldn't

Or any of these:
https://www.drupal.org/project/issues/drupal?text=exposed+grouped+filter...

I have the same issue.
That seems to be bound to drag and drop.
If you disabled it (Show row weights), you will be able to do everything you want to.

In Drupal 8.3.2 it also doesn't save when I add groups or rearrange filters.

Edit: I found a workaround. Click 'show row weights' and instead of dragging, just choose the group with a drop down. It seems to work then.

Hi All,

In Drupal 8.4.2, It also has same issue. Also, I have faced another issue like condition not working as expected.
Kindly find the test case.

I have to filter the node which is linked with term 1 or term 2, To archive this, I have created the view with filter condition like below.

Filter criteriaFilter criteria
Content: Published (= Yes)
AND
Content: filed 1(= term 1)
OR
Content: filed 2 (= term 2)

The output sql query like below:
WHERE (((node__field_1.field_1_target_id = '12')) AND ((node__field_field_2.field__field_2_target_id = '39'))) AND (node_field_data.status = '1')

Kindly anyone gives me the suggestion to archive the same.
Thanks in advance.

Here to confirm that the workaround in #118 works for now.

why is this marked as fixed?

I have 8.4.3 and filter groups have 2 issues:
- the UI is messed up, can't drag a filter to a new group when first created - but, it looks like it works after rearrange is first saved
- the resulting sql is wrong (OR is not ORing)

perhaps this is fixed in newer version but i think 8.4 is current version.

http://take.ms/JWQfM

It is NOT fixed on D.8.4.5.

Not to bump an old post but the workaround for fixing the broken sql "(OR is not ORing)" appears to be choosing the "Reduce duplicates" option for each of the filters in the OR group.

@inversed. Where is the "Reduce duplicates" option? Never seen this. Are you referring to Distinct? Thanks.

This is still broken in 8.5.6. This has been an issue for years. It's enough to challenge one's faith in Drupal 8.

Can confirm, as @tedwyer said this is still broken in 8.5.6

It is also broken in 8.6.0 when trying to drag & drop. You can still create the groups using row weights instead. See #2838909: New filter groups are not saved, specifically #7.

I'll confirm that the workaround in #118 (using `show row heights` instead) works and that the UI is still broken.