This error occurs when an exposed sort has been added to a view, and an exposed filter is subsequently added. When the filter identifier is validated, Drupal\views\Plugin\views\display\DisplayPluginBase->isIdentifierUnique() runs through all handlers and the line 

$id != $key && $identifier == $handler->options['expose']['identifier']

throws the notice since exposed sorts don't have this option.

This seemed to be an issue in D7 views as well (see #1456836: AJAX HTTP error: Undefined index: identifier in is_identifier_unique()).

A straight forward fix would be to check if that option is set but there's probably a nicer solution, potentially only iterating over handlers that have the identifier option in the first place?

CommentFileSizeAuthor
#11 2882031-10.patch2.82 KBlendude
#10 2882031-10-TEST_ONLY.patch2.1 KBlendude
#10 2882031-10-TEST_ONLY.patch2.1 KBlendude
#10 interdiff-2882031-7-10.txt4.4 KBlendude
#7 2882031-views-ui-exposed-sort-plugin-bug-failing-test-7.patch3.18 KBacbramley
#3 2882031-views-ui-exposed-sort-plugin-bug-failing-test-3.patch2.62 KBacbramley
#2 2882031-views-ui-exposed-sort-plugin-bug-failing-test.patch2.5 KBacbramley

Comments

acbramley created an issue. See original summary.

acbramley’s picture

acbramley commented
Version: 8.3.x-dev » 8.4.x-dev
Status: Active » Needs review
StatusFileSize
new 2882031-views-ui-exposed-sort-plugin-bug-failing-test.patch2.5 KB

Here's the failing test.

acbramley’s picture

acbramley commented
StatusFileSize
new 2882031-views-ui-exposed-sort-plugin-bug-failing-test-3.patch2.62 KB

This time in the correct location and namespace...

Status: Needs review » Needs work

The last submitted patch, 3: 2882031-views-ui-exposed-sort-plugin-bug-failing-test-3.patch, failed testing.

lendude’s picture

lendude
Primary language Dutch
Location Amsterdam
commented

@acbramley++

Nice to see test coverage for this!

The following feedback is all 'perfect world' type feedback and is not to take anything away from this test:

  1. This is using the Views UI namespace to expose a bug in Views
  2. +++ b/core/modules/views_ui/tests/src/Functional/ExposedCriteriaTest.php
@@ -0,0 +1,88 @@
+  public static $modules = ['node', 'views', 'views_ui'];

    It depends on the node module, would be nice to have it use EntityTest instead. 

  3. +++ b/core/modules/views_ui/tests/src/Functional/ExposedCriteriaTest.php
@@ -0,0 +1,88 @@
+    $this->drupalGet('admin/structure/views/view/content');

    Depends on the default content View, there is a test version of that so that would be better, but this should be testable with any View, even just views.view.test_view.yml

  4. This could probably be achieved in a kerneltest.
acbramley’s picture

acbramley commented

@Lendude thanks for the review! All very valid points, working on the Kernel test now.

acbramley’s picture

acbramley commented
Status: Needs work » Needs review
StatusFileSize
new 2882031-views-ui-exposed-sort-plugin-bug-failing-test-7.patch3.18 KB

Reworked as a Kernel test, I tried also adding:

$this->assertFalse($view->display_handler->isIdentifierUnique('some_id', 'id'));

But I must be failing to understand what the function is used for as it was returning TRUE.

Status: Needs review » Needs work

The last submitted patch, 7: 2882031-views-ui-exposed-sort-plugin-bug-failing-test-7.patch, failed testing.

robloach’s picture

robloach
he/him
commented

Your workaround solution was something like this?

          else {
            if ($id != $key && isset($handler->options['expose']['identifier']) && $identifier == $handler->options['expose']['identifier']) {
              return FALSE;
            }
          }
lendude’s picture

lendude
Primary language Dutch
Location Amsterdam
commented
Version: 8.4.x-dev » 8.3.x-dev
Status: Needs work » Needs review
StatusFileSize
new interdiff-2882031-7-10.txt4.4 KB
new 2882031-10-TEST_ONLY.patch2.1 KB
new 2882031-10-TEST_ONLY.patch2.1 KB

@acbramley that assertFalse was failing because the filter was using the Broken handler.

Bit of a reroll to not use a dedicated View for this. We can easily add the needed settings in the test, and i think that makes it much clearer what we are testing here.

And I think the fix in #9 is fine.

lendude’s picture

lendude
Primary language Dutch
Location Amsterdam
commented
StatusFileSize
new 2882031-10.patch2.82 KB

Bleh and now with the fix as well.....

The last submitted patch, 10: 2882031-10-TEST_ONLY.patch, failed testing. View results

The last submitted patch, 10: 2882031-10-TEST_ONLY.patch, failed testing. View results

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

otrolopezmas’s picture

otrolopezmas
He/Him
Primary language Spanish
commented

In drupal 8.5.3 this is still happening. I re-ran the test for the patch and they pass for d8.5 with php 5.6 and php 7.

What can we do to progress with the implementation of this solution?

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Anonymous’s picture

Anonymous (not verified) commented

Patch 2882031-10.patch works like a charm on 8.6.4.
Thank you very much.

theodorosploumis’s picture

theodorosploumis
he/him
Location Greece
commented

Patch from #comment-12151207 works with Drupal 8.7.1. Thanks.

anoopjohn’s picture

anoopjohn commented
Version: 8.6.x-dev » 8.8.x-dev
Status: Needs review » Reviewed & tested by the community

I can confirm that this patch works on 8.7.10 and 8.8.x dev

anoopjohn’s picture

anoopjohn commented
Assigned: acbramley » Unassigned
lendude’s picture

lendude
Primary language Dutch
Location Amsterdam
commented

@anoopjohn thanks for looking into this one. Re-queued the tests against a somewhat more recent version of core :)

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

Adding issue credit.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Patch (to be ported)

Committed and pushed 258c4eac9c to 9.0.x and 4452522144 to 8.9.x. Thanks!

Will ask other committers about back porting to 8.8.x

  • alexpott committed 258c4ea on 9.0.x 
    Issue #2882031 by Lendude, acbramley, RobLoach: Undefined index:...

  • alexpott committed 4452522 on 8.9.x 
    Issue #2882031 by Lendude, acbramley, RobLoach: Undefined index:...
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Patch (to be ported) » Fixed

@catch +1'd the backport.

  • alexpott committed 93335fc on 8.8.x 
    Issue #2882031 by Lendude, acbramley, RobLoach: Undefined index:...

Status: Fixed » Closed (fixed)

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