Notice: Undefined index: value in Drupal\views\Plugin\views\filter\NumericFilter->acceptExposedInput()

Adding a new patch as #116 does not address the feedback in #120, and the patch #121 reverts the wrong line.

I is not clear if #128 is related to this patch as I was unable to reproduce this error - but given the comment includes with "didn't fix the issue I have" implies this issue existed before the patch and so I think it is unrelated. @DiDebru can you please confirm?

Looks like we can remove the expected errors from the baseline too.

Queued tests for 129 on 9.5 as this patch will not apply to 9.5 - and will check 10.1 / 10.0 on this patch

I've made some investigation and I like to propose other solution.

Why it happens?

The warning is triggered in NumericFilter::acceptExposedInput() and it called twice. With grouped filter first time there is just an integer (e.g. 1) in input that triggers the warning, the second time input gets expected input as array with keys ['value', 'type', 'min', 'max']. It looks strange as it is expected to have the same input on each call.

First call in ViewExposedForm::submitForm() that triggers warning:

    // Form input keys that will not be included in $view->exposed_raw_data.
    $exclude = ['submit', 'form_build_id', 'form_id', 'form_token', 'exposed_form_plugin', 'reset'];
    $values = $form_state->getValues();
    foreach (['field', 'filter'] as $type) {
      /** @var \Drupal\views\Plugin\views\ViewsHandlerInterface[] $handlers */
      $handlers = &$form_state->get('view')->$type;
      foreach ($handlers as $key => $info) {
        if ($handlers[$key]->acceptExposedInput($values)) {
          $handlers[$key]->submitExposed($form, $form_state);
        }
        else {
          // The input from the form did not validate, exclude it from the
          // stored raw data.
          $exclude[] = $key;
        }
      }
    }

Second time in ViewExecutable:_build() without warning

    $handlers = &$this->$key;
    foreach ($handlers as $id => $data) {

      if (!empty($handlers[$id]) && is_object($handlers[$id])) {
        $multiple_exposed_input = [0 => NULL];
        if ($handlers[$id]->multipleExposedInput()) {
          $multiple_exposed_input = $handlers[$id]->groupMultipleExposedInput($this->exposed_data);
        }
        foreach ($multiple_exposed_input as $group_id) {
          // Give this handler access to the exposed filter input.
          if (!empty($this->exposed_data)) {
            if ($handlers[$id]->isAGroup()) {
              $converted = $handlers[$id]->convertExposedInput($this->exposed_data, $group_id);
              $handlers[$id]->storeGroupInput($this->exposed_data, $converted);
              if (!$converted) {
                continue;
              }
            }
            $rc = $handlers[$id]->acceptExposedInput($this->exposed_data);
            $handlers[$id]->storeExposedInput($this->exposed_data, $rc);
            if (!$rc) {
              continue;
            }
          }
          $handlers[$id]->setRelationship();
          $handlers[$id]->query($this->display_handler->useGroupBy());
        }
      }
    }

So for me there are two questions:

• Why NumericFilter::acceptExposedInput() is called twice?
• Why NumericFilter::convertExposedInput() that converts group input to expected format is missed in first case?(By the way I've tried to add it to first case, but it breaks the view execution)

One more strange thing found

Also I pay attention to broken $exclude variable, it declared twice in ViewExposedForm::submitForm() and isn't useful in first case.

    // Form input keys that will not be included in $view->exposed_raw_data.
    $exclude = ['submit', 'form_build_id', 'form_id', 'form_token', 'exposed_form_plugin', 'reset'];
    foreach (['field', 'filter'] as $type) {
          // ... Some code miss ...
          $exclude[] = $key;
    }

    // Rewrites upper results here.
    $exclude = ['submit', 'form_build_id', 'form_id', 'form_token', 'exposed_form_plugin', 'reset'];
    foreach ($values as $key => $value) {
      // Used last assignment only.
      if (!empty($key) && !in_array($key, $exclude)) {
        // ... Some code miss ...
      }
    }

Changes history

I've go to look for this changes in history and found that all this things above were added in this issue #2651102 → ( patch → ). The patch adds checkbox processing to form values and inside NumericFilter::acceptExposedInput() and non-working first $exclude assignment. The code before the patch:

    foreach (array('field', 'filter') as $type) {
      /** @var \Drupal\views\Plugin\views\ViewsHandlerInterface[] $handlers */
      $handlers = &$form_state->get('view')->$type;
      foreach ($handlers as $key => $info) {
        $handlers[$key]->submitExposed($form, $form_state);
      }
    }

Looks like NumericFilter::acceptExposedInput() was used to process checkboxes before call NumericFilter::submitExposed(), but:

• NumericFilter doesn't use ::submitExposed() method, it inherits empty functionality
• No core views filter has the method
• No Search API views filter has the method
• If custom module implements ::submitExposed(), it could care about checkbox processing inside
• $exclude improvements aren't working

Proposal

I propose to revert a part of issue #2651102 → described above, that causes warning and looks like doesn't have effective changes. @catch, @mikeker, welcome to comment!

This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request → as a guide.

Can confirm this issue and that the patch addresses it.
It will need test coverage though.

FWIW there are tests in the patches up until #131. I haven't really looked at the new approach but perhaps if we had an updated patch with the test from #130 and the fix from #131 we can confirm if this fixes the same issue.

Existing tests changed to handle the notice. One more notice found when group identifier doesn't match to simple exposed one and fixed.

One more small improvement to prevent possible notices if identifier is empty (looks it could happen on wrong config only), but anyway there is no sense to make additional check if parent::acceptExposedInput() returns FALSE.

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Fixed CSpell issues, but phpcs warning isn't happens locally and surprises me at all, the variable isn't unused.

FILE: ...l10/core/modules/views/src/Plugin/views/filter/NumericFilter.php
----------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------
411 | WARNING | Unused variable $value.
----------------------------------------------------------------------

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Added Updated patch and fixed CCF #137

@Akram, as far as I understand renaming "_test*" methods to "test*" means that it runs by phpunit engine itself, I afraid with the changes these methods called twice now, by phpunit engine and from ::testDateFilter() directrly.

NW on #141

Took a quick look at #drupalsouth code sprint. Looks like function testDateFilter() can be dropped then.

Took a brief look and seems there are some out of scope changes that were probably introduced in #139

I don't support an idea to beatify all the file (not the changed lines only), I think doc-comments should be added in a scope of other issue, but not in a bug. By the example from previous patches I've just add `$value = NULL;` to pass PHPCS, rebased to 11.x branch and no more changed from #137.

P.S. by the way PHPStorm not highlights the variable as unused(

Confirmed the problem in the issue summary following the steps.
Note doesn't matter the identifier name, tried a few different ones but always got the error.

Applying the patch #146 solved the issue and see it has test coverage as well.

Thanks!

The last submitted patch, 146: core-notice_in_numeric_filter-2825860-146.patch, failed testing. View results →

it seems that patch actually passes.

back to RTBC

Sometime between #130 and #131 this patch changed dramatically.

Can we elaborate why?

We had subsystem maintainer signoff on the approach in ##13 (see #113 and #120)

The changes to remove all the special keys like form build ID definitely feel wrong to me.

Can we go back to #130 and can those with the new approach elaborate why it isn't suitable?

Where did the unit test go? It was exposing several ways this could be triggered that had nothing to do with what ever is being changed in the exposed form handler I suspect might not being addressed based on a skim of the current patch.

@larowlan, I've made research of the issue and tried to provide explanation in #131. If anyone have some exact questions - you are welcome.

@neclimdul Do you propose just to add the Unit test from #130?

Think the confusion is that in 129-130 the size was about 8KB

Then your post went to around 2KB with an explanation

But then in 139 it jumped up to 13KB

Was anything lost in that jump back down to 5KB, if tests were lost could we add back?

May help to hide all patches but the right one (making sure it matches the issue summary, didn't check). Also quickest solution is turn to an MR

Taking a look at this - aim to restore the test improvements from #130. Will move this to an MR per smustgrave suggestion.

xurizaemon → closed merge request !7141

Ignore !7141 please. !7145 should be the tests from #130 and the solution approach from #131.

Noting @larowlan comments in #151 I planned today at DrupalSouth to also produce an MR with the original approach which had maintainer agreement, that's not done yet.

xurizaemon → closed merge request !7145

Removing old patches from display, MR here is approach + tests from #130 updated.

Interdiff is not cooperating with me trying to get an interdiff from 130 to the MR, so I'll summarise as best I can from visual review:

• Change from 130 to core/phpstan-baseline.neon moves to core/.phpstan-baseline.php
• In FilterDateTest.php we pass form options as strings not integers now ($this->getSession()->getPage()->findField($filter_identifier)->selectOption('1'); not ...->selectOption(1);
• declare(strict_types=1); in NumericFilterTest.php

Missed status update, back to NR

Hiding patches for clarity.

Left a comment on the MR that may need to be clarified. If I'm correct that it's the wrong key then surprised nothing broke.

Workaround added to issue description - if step 6 from the issue description is omitted, the bug does not appear.

Reproducible on 11.x, updated repro details in the issue description to use "Authored on" as that makes it easier to verify results on an empty site (create two test nodes, set "Authored on" date back for one node). View config I tested with here. →

Test coverage can be seen https://git.drupalcode.org/issue/drupal-2825860/-/jobs/1173292

Following the issue summary believe I am able to see the issue and the MR does address it.

Also appears all feedback for this bug has been addressed so believe this one is good.

Assigning issue credit. Not crediting screenshot confirming bug, or works for me, or doesn't work for me. Trying to credit people who moved the fix and issue forward. If I make a mistake I'm sorry! Thanks.

alexpott → changed the visibility of the branch 2825860-notice-undefined-index to hidden.

Committed and pushed ba08bc0dcb to 11.x and b5f62fa4ae to 10.3.x and f78530b4a7 to 10.2.x. Thanks!

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

Re-roll of patch #48 for branch 10.3.x.

I had the same issues, applied the patch from #175 and it worked correctly. It seems it is still needed even after the previous merge at #170

Agreed. The work here that was committed in 10.2.x, 10.3.x, 11.x is absolutely needed, but i'm still having issues with the actual value.

Inside here:

if (!empty($info[$this->operator]['values'])) {
  switch ($info[$this->operator]['values']) {
    case 1:
      if ($value['value'] === '') {
        return FALSE;
      }
      break;

    case 2:
      if ($value['min'] === '' && $value['max'] === '') {
        return FALSE;
      }
      break;
  }
}

for grouped (checkbox?) input, $value is this:

array (
  1 => '1',
)

which causes the error still as there is no 'value' key to check against for this grouped exposed input value.

Patch 175 which is a re-roll of 48 actually checks to inspect $value to make sure that the expected keys are there which seems to be what's needed in some circumstances?

There is another issue here in relation to this though... where ViewsExposedForm::submitForm() https://git.drupalcode.org/project/drupal/-/blob/10.3.x/core/modules/vie...

seems to declare an $exclude variable at the top of the function, which is added to if acceptExposedInput() returns false but then immediately beneath this outside of the loop, it's declared again (with the same initial keys) which overrides any entries that were added by this loop!

So you can get into a weird situation where the handler for the exposed input returns false, but then beneath it can still be processed as it's no longer inside of $exclude... which in my circumstances is actually useful as I want the exposed filter value to be valid... but technically it shouldn't be doing this!?

Maybe someone with some more knowledge around this could provide some insights about the data structure of grouped $value's and how this is expected to work without the additions in patch 175...