Problem/Motivation

According to the filtering documentation the labels assigned to filters can be a string or even an integer, but we found that filters with "0" as a label throw the following error:

'@root' is a reserved filter id.

Because of the way that PHP makes comparisons between the integer 0 and strings (https://www.php.net/manual/en/types.comparisons.php), the following piece of code is evaluated to true in the Filter class.

// Throw an exception if the query uses the reserved filter id for the
// root group.
if ($key == static::ROOT_ID) {
  $msg = sprintf("'%s' is a reserved filter id.", static::ROOT_ID);
  throw new \UnexpectedValueException($msg);
}

Steps to reproduce

  1. Enable jsonapi.
  2. Make a filtered request to a collection endpoint with "0" as a label of one of the filters. For example:/jsonapi/node/article?filter[0][path]=status&filter[0][value]=1

Proposed resolution

Make a strict comparison between the filters' label and the reserved filter id.

Comments

guillerecalde created an issue. See original summary.

guillerecalde’s picture

mohit.bansal623’s picture

Status: Active » Needs review
StatusFileSize
new611 bytes
abhijith s’s picture

StatusFileSize
new66.16 KB
new31.99 KB

Applied patch #3 in 8.9.x and it works fine.The error is gone after applying this patch.Adding screenshots below,

Before:
before

After patch:
after

ilgnerfagundes’s picture

Status: Needs review » Reviewed & tested by the community
StatusFileSize
new158.14 KB
new45.38 KB

I applied patch # 3 on drupal 8.9 and it's working, I'll leave prints showing

anacolautti’s picture

Hi there! Out of curiosity, as far as I see both patches have the exact same contents. Why do we have 2 patches here?

alexpott’s picture

Priority: Minor » Normal
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

Thanks for filing this bug report and for fixing the issue. Bug fixing is very valuable. In order to commit a bug fix, we need an automated test to prove that we've fixed the bug and ensure that we don't break it again in the future. For more information about writing tests in Drupal, see the following links:

  1. https://www.drupal.org/docs/testing/phpunit-in-drupal/phpunit-javascript...
  2. https://api.drupal.org/api/drupal/core%21core.api.php/group/testing/9.1.x
mohit_aghera’s picture

Version: 8.9.x-dev » 9.3.x-dev
Status: Needs work » Needs review
Issue tags: -Needs tests
StatusFileSize
new994 bytes
new1.57 KB
new994 bytes

- Adding test cases and test-only patch to replicate the bug.
- I can see the error '@root' is a reserved filter id. on local when I run the test case.

- Bump up version to 9.3.x as we are going to address bugs in that release.

mohit_aghera’s picture

StatusFileSize
new992 bytes
new1.57 KB
new655 bytes

Forgot to remove the quotes around the key from test cases.
Test case is same and still test-only patch failing on local.
Let's see how it goes.

bbrala’s picture

Status: Needs review » Needs work

Hi there, the test-only patch should be failing to prove the bug, i know it's a hassle since the testing takes so long. I wonder if perhaps PHP version has something to do with is?

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

smustgrave’s picture

Issue tags: +Bug Smash Initiative

This came up as a daily BSI

Think it would be good to determine if this is still an issue.

See we had a patch with test coverage we could add that to an MR now.

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.