Improve the ExistsFilter behavior (to cohabit with the SearchFilter) #2243

epourail · 2018-10-07T13:57:06Z

Q	A
Bug fix?	yes (for GraphQL)
New feature?	yes
BC breaks?	yes
Deprecations?	yes
Tests pass?	yes
Fixed tickets	#2231
License	MIT
Doc PR	api-platform/docs#613

Improve the behavior of the ExistsFilter filter to be declared according to the following syntax:

on a REST collection endpoints: /dummies?exists[<property>]=true/false
on the GraphQL endpoint: dummies(exists: {<property>: "true/false"}) { ... }

The name of the query parameter is controlled via the bundle configuration:

api_platform:
  collection:
    exists_parameter_name: 'not_null'

It allows to use an ExistsFilter and a SearchFilter on the same property via the GraphQL endpoint.

Improve ExistsFilter
PHPUnit
Behat
Documentation

…equest api-platform/core#2243.

alanpoulain · 2018-10-11T12:58:32Z

Why notnull_parameter_name and not exists_parameter_name (I would call it exists_filter_parameter_name but I think you chose this to be consistent with the order filter, didn't you?).

alanpoulain · 2018-10-11T13:02:52Z

You need to make sure the previous way of using the exists filter is still working, but deprecate it.

alanpoulain · 2018-10-11T13:03:32Z

src/Bridge/Doctrine/Orm/Filter/ExistsFilter.php

@@ -30,11 +34,24 @@
 * Request: GET /products?brand[exists]
 * Interpretation: filter products which have a brand
 *
- * @author Teoh Han Hui <teohhanhui@gmail.com>
+ * @author Julien Verger <julien.verger@gmail.com>


Please add you after the previous author.

src/Bridge/Doctrine/Orm/Filter/ExistsFilter.php

alanpoulain · 2018-10-11T13:06:10Z

src/Bridge/Doctrine/Orm/Filter/ExistsFilter.php

+    /**
+     * @param RequestStack|null $requestStack No prefix to prevent autowiring of this deprecated property
+     */
+    public function __construct(ManagerRegistry $managerRegistry, $requestStack = null, string $existsParameterName = 'exists', LoggerInterface $logger = null, array $properties = null)


Add $existsParameterName after $logger to prevent BC.

And you can reuse the existing constant: $existsParameterName = self::QUERY_PARAMETER_KEY

src/Bridge/Doctrine/Orm/Filter/ExistsFilter.php

dunglas · 2018-10-12T08:20:14Z

src/Bridge/Doctrine/Orm/Filter/ExistsFilter.php

 */
 class ExistsFilter extends AbstractContextAwareFilter
 {
-    const QUERY_PARAMETER_KEY = 'exists';


Should be kept (or it's a BC break)

dunglas · 2018-10-12T08:20:57Z

src/Bridge/Doctrine/Orm/Filter/ExistsFilter.php

+    /**
+     * @param RequestStack|null $requestStack No prefix to prevent autowiring of this deprecated property
+     */
+    public function __construct(ManagerRegistry $managerRegistry, $requestStack = null, string $existsParameterName = 'exists', LoggerInterface $logger = null, array $properties = null)


And you can reuse the existing constant: $existsParameterName = self::QUERY_PARAMETER_KEY

dunglas · 2018-10-12T08:21:48Z

src/Bridge/Doctrine/Orm/Filter/ExistsFilter.php

-    protected function filterProperty(string $property, $value, QueryBuilder $queryBuilder, QueryNameGeneratorInterface $queryNameGenerator, string $resourceClass, string $operationName = null)
+    public function apply(QueryBuilder $queryBuilder, QueryNameGeneratorInterface $queryNameGenerator, string $resourceClass, string $operationName = null, array $context = [])
+    {
+        if (isset($context['filters']) && !isset($context['filters'][$this->existsParameterName])) {


the first isset is useless and can be removed.

the first isset is needed @dunglas if I remember correctly, that how we determine if it should be forwarded to the parent class (used before the usage of the AbstractContextAwareFilter); there is the same code in the OrderFilter (see #1866)

dunglas · 2018-10-12T08:23:32Z

src/Bridge/Doctrine/Orm/Filter/ExistsFilter.php

+            return;
+        }
+
+        if (!isset($context['filters'][$this->existsParameterName]) || !\is_array($context['filters'][$this->existsParameterName])) {


the first isset can be removed (you already tested the existence of the key previously), and you can then merge both if:

if (!\is_array($context['filters'][$this->existsParameterName] ?? null)) {

src/Bridge/Doctrine/Orm/Filter/ExistsFilter.php

dunglas · 2018-10-12T08:26:09Z

tests/Bridge/Doctrine/Orm/Filter/ExistsFilterTest.php


        $this->assertEquals([
-            'description[exists]' => [


We need to keep this test to be sure that the old behavior still work. You should use a PHPUnit's data provider to test both behaviors instead.

I kept the test and create a new one.

dunglas · 2018-10-12T08:27:07Z

It will be a nice improvement, thanks for working on this!

epourail · 2018-10-12T20:31:17Z

Why notnull_parameter_name and not exists_parameter_name (I would call it exists_filter_parameter_name but I think you chose this to be consistent with the order filter, didn't you?).

@alan Yes I keep the same logic (behavior and naming) as the order filter.
I decided to name it notnull_parameter_name because, at the end, it is a IS NOTNULL/IS NULL check (and keep the default value exists to reference the previous logic)

antograssiot · 2018-10-29T21:32:44Z

src/Bridge/Doctrine/Orm/Filter/ExistsFilter.php

 */
-class ExistsFilter extends AbstractContextAwareFilter
+final class ExistsFilter extends AbstractContextAwareFilter


you can only add a @final annotation, this is a BC break even if it was forgotten in the first place, people might have extended this class
https://symfony.com/doc/current/contributing/code/bc.html#changing-classes

…equest api-platform/core#2243.

epourail

@alanpoulain @dunglas Concerning this PR, I set in standby the development due to lack of time and unable to fix the behat tests.
Would you take it from here (to follow and close) ?

src/Bridge/Doctrine/Orm/Filter/ExistsFilter.php

epourail · 2018-11-01T12:34:00Z

tests/Bridge/Doctrine/Orm/Filter/ExistsFilterTest.php


        $this->assertEquals([
-            'description[exists]' => [


I kept the test and create a new one.

alanpoulain · 2018-12-18T14:16:00Z

@epourail I am planning to finish your PR. I am working on a PR and it takes a lot of time. That's why I can't right now. If anyone is willing to take it before me go ahead.

alanpoulain · 2019-03-05T20:19:12Z

This PR has been rebased, tests are fixed.
I have chosen to rename the parameter exists_parameter_name to not lose our users (even if the comment of @epourail is true).

soyuka

Looks really good, nice that this is still supporting the old syntax!

alanpoulain · 2019-03-06T19:15:58Z

Thank you @epourail! Nice one.

…equest api-platform/core#2243.

…equest api-platform/core#2243. (#613)

teohhanhui · 2019-03-08T16:55:41Z

We've come full circle then! When I did the ExistsFilter, it was using this syntax at first lol...

And then this happened: #906 (comment) 🙈

Remove a BC break introduced by #2243

epourail changed the title ~~Fix exists filter behavior~~ Improve the ExistsFilter behavior (to cohabit with the SearchFilter) Oct 7, 2018

epourail changed the title ~~Improve the ExistsFilter behavior (to cohabit with the SearchFilter)~~ Improve the ExistsFilter behavior (to cohabit with the SearchFilter) Oct 7, 2018

alanpoulain added [WIP] Work In Progress GraphQL labels Oct 7, 2018

epourail force-pushed the fix-exists-filter-behavior branch 7 times, most recently from acc3675 to 2c206b8 Compare October 9, 2018 16:05

epourail pushed a commit to epourail/fork-apiplatform-docs that referenced this pull request Oct 9, 2018

This PR documents the new ExistFilter behavior proposed in the pull r…

8dbbc00

…equest api-platform/core#2243.

epourail mentioned this pull request Oct 9, 2018

Update the ExistsFilter behavior api-platform/docs#613

Merged

alanpoulain self-assigned this Oct 10, 2018

alanpoulain removed the [WIP] Work In Progress label Oct 10, 2018

alanpoulain reviewed Oct 11, 2018

View reviewed changes

src/Bridge/Doctrine/Orm/Filter/ExistsFilter.php Outdated Show resolved Hide resolved

alanpoulain reviewed Oct 11, 2018

View reviewed changes

src/Bridge/Doctrine/Orm/Filter/ExistsFilter.php Show resolved Hide resolved

dunglas requested changes Oct 12, 2018

View reviewed changes

antograssiot reviewed Oct 29, 2018

View reviewed changes

epourail force-pushed the fix-exists-filter-behavior branch 3 times, most recently from 5b13d41 to 09c0715 Compare November 1, 2018 12:02

epourail pushed a commit to epourail/fork-apiplatform-docs that referenced this pull request Nov 1, 2018

This PR documents the new ExistFilter behavior proposed in the pull r…

49a2270

…equest api-platform/core#2243.

epourail force-pushed the fix-exists-filter-behavior branch from 190a979 to 3ee15ff Compare November 6, 2018 15:02

epourail force-pushed the fix-exists-filter-behavior branch from 3ee15ff to 671e4d6 Compare November 13, 2018 09:01

epourail force-pushed the fix-exists-filter-behavior branch from 671e4d6 to 8239fe3 Compare November 24, 2018 17:30

epourail commented Dec 18, 2018

View reviewed changes

alanpoulain force-pushed the fix-exists-filter-behavior branch 3 times, most recently from 092fb40 to fd548d6 Compare March 5, 2019 20:15

alanpoulain requested review from dunglas and antograssiot March 5, 2019 20:19

soyuka approved these changes Mar 6, 2019

View reviewed changes

Update the ExistsFilter behavior

3110c9b

alanpoulain force-pushed the fix-exists-filter-behavior branch from fd548d6 to 3110c9b Compare March 6, 2019 16:48

alanpoulain merged commit 19c08e0 into api-platform:master Mar 6, 2019

alanpoulain pushed a commit to epourail/fork-apiplatform-docs that referenced this pull request Mar 7, 2019

This PR documents the new ExistFilter behavior proposed in the pull r…

9f40429

…equest api-platform/core#2243.

alanpoulain pushed a commit to api-platform/docs that referenced this pull request Mar 7, 2019

This PR documents the new ExistFilter behavior proposed in the pull r…

9d3d5a5

…equest api-platform/core#2243. (#613)

meyerbaptiste added a commit to meyerbaptiste/core that referenced this pull request Mar 22, 2019

Remove a BC break introduced by api-platform#2243

2cdf879

meyerbaptiste added a commit to meyerbaptiste/core that referenced this pull request Mar 25, 2019

Remove a BC break introduced by api-platform#2243

7f761a6

meyerbaptiste added a commit to meyerbaptiste/core that referenced this pull request Mar 25, 2019

Remove a BC break introduced by api-platform#2243

ce7dfd4

meyerbaptiste added a commit to meyerbaptiste/core that referenced this pull request Mar 25, 2019

Remove a BC break introduced by api-platform#2243

8e1ee34

soyuka added a commit that referenced this pull request Mar 26, 2019

Merge pull request #2640 from meyerbaptiste/fix_bc_break

6980627

Remove a BC break introduced by #2243

Uh oh!

Improve the ExistsFilter behavior (to cohabit with the SearchFilter) #2243

Improve the ExistsFilter behavior (to cohabit with the SearchFilter) #2243

Uh oh!

Conversation

epourail commented Oct 7, 2018 • edited by alanpoulain Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alanpoulain commented Oct 11, 2018

Uh oh!

alanpoulain commented Oct 11, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dunglas commented Oct 12, 2018

Uh oh!

epourail commented Oct 12, 2018

Uh oh!

antograssiot Oct 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

epourail left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alanpoulain commented Dec 18, 2018

Uh oh!

alanpoulain commented Mar 5, 2019

Uh oh!

soyuka left a comment

Choose a reason for hiding this comment

Uh oh!

alanpoulain commented Mar 6, 2019

Uh oh!

teohhanhui commented Mar 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

epourail commented Oct 7, 2018 •

edited by alanpoulain

Loading

antograssiot Oct 29, 2018 •

edited

Loading

teohhanhui commented Mar 8, 2019 •

edited

Loading