Add filter locator and deprecate filter collection #1099

meyerbaptiste · 2017-05-04T09:22:36Z

Q	A
Bug fix?	no
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	N/A
License	MIT
Doc PR	N/A

First step before #1082 and #1083, we have to refactor the FilterCollection to avoid circular references with new filters.
Our solution is to use the PSR11 and a service locator. Thanks to http://symfony.com/blog/new-in-symfony-3-3-service-locators.

soyuka

This is really nice work ❤️

soyuka · 2017-05-04T09:41:26Z

src/Api/FilterCollectionFactory.php

+            try {
+                $filters[$filter] = $filterLocator->get($filter);
+            } catch (\Throwable $e) {
+            }


Shouldn't we use if ($filterLocator->has($filter)) {} instead of catching?

soyuka · 2017-05-04T09:44:14Z

src/Bridge/Doctrine/Orm/Extension/FilterExtension.php


-    public function __construct(ResourceMetadataFactoryInterface $resourceMetadataFactory, Fil
8000
terCollection $filters)
+    public function __construct(ResourceMetadataFactoryInterface $resourceMetadataFactory, $filterLocator)


There is no such typing as ContainerInterface | FilterCollection in php right? Maybe we could document this new argument though.

same goes in the other classes with the same change

This works only for try/catch with PHP 7.1. I will add some PHPDoc.

I just made the change. Is it good for you like this?

👍 I hope that phpstan is fine with it and yeah this is better for future developers IMO. Thanks!

soyuka · 2017-05-04T09:46:42Z

src/Metadata/Extractor/YamlExtractor.php

@@ -32,7 +32,7 @@
    protected function extractPath(string $path)
    {
        try {
-            $resourcesYaml = Yaml::parse(file_get_contents($path));
+            $resourcesYaml = Yaml::parse(file_get_contents($path), Yaml::PARSE_KEYS_AS_STRINGS);


Hmm why this change? Did you hit a bug? This might need to be backported to 2.0 if it's the case.

I have deprecation warnings from the component without this change and tests fail...

oh okay this is because symfony 3.3 then alright! Thanks for the information.

symfony/symfony#21774

meyerbaptiste · 2017-05-04T14:50:08Z

Tests fail because the master branch fails too.

chalasr · 2017-05-04T13:10:32Z

src/Bridge/Symfony/Bundle/Resources/config/filter.xml

+
+        <service id="api_platform.filter_collection_factory" class="ApiPlatform\Core\Api\FilterCollectionFactory" public="false" />
+
+        <service id="api_platform.filters" class="ApiPlatform\Core\Api\FilterCollection" public="false">


Looks like a good candidate for <defaults public="false" />

Oh, nice, I missed this feature! Thanks!

soyuka · 2017-05-04T15:11:40Z

Tests fail because the master branch fails too.

Indeed, the Accept header isn't changing the format. This returns JSON-LD. It's weird, everything was green, then I just did composer update and it now fails.

Investigating but maybe @dunglas has a clue on this?

soyuka · 2017-05-04T16:40:41Z

composer.json

@@ -23,7 +23,7 @@
        "symfony/property-access": "^2.7 || ^3.0",
        "symfony/property-info": "^3.3@beta",
        "symfony/serializer": "^3.3@beta",
-        "willdurand/negotiation": "^2.0.3"
+        "willdurand/negotiation": "~2.2.0"


nice one maybe we can patch this in a separated pr?

Should be ~2.2.3 right?

What about >=2.0.3 <2.3.0? BTW it's ~2.2.1!

^2.0.3 <2.3

meyerbaptiste · 2017-05-04T16:41:24Z

Ok, I got it. The problem comes from the new release of willdurand/negotiation (2.3.0) and this commit: willdurand/Negotiation#92.

meyerbaptiste · 2017-05-05T08:29:12Z

Does anyone have an idea to fix/avoid that?

Generating code coverage report in PHP format ... done
Remaining deprecation notices (1)
The ApiPlatform\Core\Api\FilterCollection class is deprecated since version 2.1 and will be removed in 3.0. Implement the Psr\Container\ContainerInterface interface as service locator instead: 1x
    1x in CodeCovera
9E88
ge::start from SebastianBergmann\CodeCoverage

chalasr · 2017-05-05T10:29:12Z

@meyerbaptiste CodeCoverage loads classes which causes the FilterCollection deprecation to be triggered. What you need is to move the deprecation to the constructor:

diff --git a/src/Api/FilterCollection.php b/src/Api/FilterCollection.php
index 32b1157..76af317 100644
--- a/src/Api/FilterCollection.php
+++ b/src/Api/FilterCollection.php
@@ -15,8 +15,6 @@ namespace ApiPlatform\Core\Api;
 
 use Psr\Container\ContainerInterface;
 
-@trigger_error(sprintf('The %s class is deprecated since version 2.1 and will be removed in 3.0. Implement the %s interface as service locator instead.', FilterCollection::class, ContainerInterface::class), E_USER_DEPRECATED);
-
 /**
  * A list of filters.
  *
@@ -26,4 +24,10 @@ use Psr\Container\ContainerInterface;
  */
 final class FilterCollection extends \ArrayObject
 {
+    public function __construct($input = [], $flags = 0, $iterator_class = \ArrayIterator::class)
+    {
+        @trigger_error(sprintf('The %s class is deprecated since version 2.1 and will be removed in 3.0. Implement the %s interface as service locator instead.', self::class, ContainerInterface::class), E_USER_DEPRECATED);
+
+        parent::__construct($input, $flags, $iterator_class);
+    }
 }

Or exclude this class from code coverage, I don't use coverage at all so I can't say if it's a good idea nor help.

soyuka · 2017-05-05T13:24:49Z

Also you can mark specific tests as @group legacy and use @expectedDeprecation

meyerbaptiste · 2017-05-05T14:19:48Z

It's what I made @soyuka but the problem here comes from coverage and not tests themselves.
But I think I will follow the @chalasr's suggestion and move my trigger_error() inside my constructor to avoid the deprecation notice during the coverage.

chalasr · 2017-05-05T14:24:30Z

@soyuka comment made me think that maybe moving the @group legacy+@expectedDeprecation from the test methods to the top of the test class would solve it, unfortunately it does not. Either you have to trigger deprecations from constructors in order to don't have it triggered before construction, or the whole class should be excluded from the coverage, which I think is not a good idea as the class is still usable and well tested.

soyuka · 2017-05-10T09:50:22Z

src/Api/IdentifiersExtractor.php

@@ -76,6 +76,7 @@ public function getIdentifiersFromItem($item): array
                throw new RuntimeException(sprintf('No identifier found in "%s" through relation "%s" of "%s" used as identifier', $relatedResourceClass, $propertyName, $resourceClass));
            }
        }
+


Was fixed by 7dfdcb1

soyuka · 2017-05-10T09:51:38Z

features/main/crud.feature

@@ -123,73 +123,67 @@ Feature: Create-Retrieve-Update-Delete
      "hydra:totalItems": 1,
      "hydra:search": {
        "@type": "hydra:IriTemplate",
-        "hydra:template": "/dummies{?id,id[],name,alias,description,relatedDummy.name,relatedDummy.name[],relatedDummies,relatedDummies[],dummy,relatedDummies.name,order[id],order[name],order[relatedDummy.symfony],dummyDate[before],dummyDate[after],relatedDummy.dummyDate[before],relatedDummy.dummyDate[after],dummyFloat[between],dummyFloat[gt],dummyFloat[gte],dummyFloat[lt],dummyFloat[lte],dummyPrice[between],dummyPrice[gt],dummyPrice[gte],dummyPrice[lt],dummyPrice[lte],dummyBoolean,dummyFloat,dummyPrice,description[exists],relatedDummy.name[exists],dummyBoolean[exists]}",
+        "hydra:template": "\/dummies{?dummyBoolean,dummyDate[before],dummyDate[after],relatedDummy.dummyDate[before],relatedDummy.dummyDate[after],description[exists],relatedDummy.name[exists],dummyBoolean[exists],dummyFloat,dummyPrice,order[id],order[name],order[relatedDummy.symfony],dummyFloat[between],dummyFloat[gt],dummyFloat[gte],dummyFloat[lt],dummyFloat[lte],dummyPrice[between],dummyPrice[gt],dummyPrice[gte],dummyPrice[lt],dummyPrice[lte],id,id[],name,alias,description,relatedDummy.name,relatedDummy.name[],relatedDummies,relatedDummies[],dummy,relatedDummies.name}",


Not sure what went wrong here? If this is due to random sorting maybe we should sort everything to avoid such issues in the feature?

(we discussed this IRL with @meyerbaptiste).

Before: filters were in the same order than declared in services.yml
Now: filters are in the order the configuration in the resource.

It's not a BC break (the JSON parser should not depend of the order).
I prefer to not sort the filters, it allows to detect when we change the output, it's a desirable behavior.

dunglas · 2017-05-10T13:56:10Z

src/Api/FilterCollection.php

 */
 final class FilterCollection extends \ArrayObject
 {
+    public function __construct($input = [], $flags = 0, $iterator_class = 'ArrayIterator')
+    {
+        @trigger_error(sprintf('The %s class is deprecated since version 2.1 and will be removed in 3.0. Implement the %s interface as service locator instead.', self::class, ContainerInterface::class), E_USER_DEPRECATED);


I would say Provide and implementation of "%s" instead.

dunglas · 2017-05-10T13:56:56Z

src/Api/FilterCollectionFactory.php

+{
+    private $filtersIds;
+
+    public function __construct(string ...$filtersIds)


Why not using an array?

Just a hack to have a typed array...

I prefer to use a raw array and use string[] in the PHPDoc, PHPStan will be able to check types using the PHPDoc.

Bad day for variadic functions 😭

if you really want a check you could also force it after in the code:

$filterIds = (function (string ...$filterIds) { return $filterIds; })(...$filterIds);

Or use a lib like https://github.com/beberlei/assert

We don't have to enforce this. The PHPDoc is the contract and it's an @internal class.

I'm just giving alternatives to enforce it, not saying you have to enforce it :P

meyerbaptiste · 2017-05-10T15:07:25Z

All comments are fixed.

dunglas · 2017-05-15T12:54:06Z

Thanks @meyerbaptiste

teohhanhui · 2017-05-17T10:13:28Z

Unfortunately, this must be reverted because bumping the required versions in composer.json is a major BC break. It means we suddenly drop support for a lot of people... 😞

dunglas · 2017-05-17T10:25:05Z

@teohhanhui it's not, its allowed by semver (and we do this very often in Symfony). Note that we don't force to upgrade to new major versions, just to the last minor.

teohhanhui · 2017-05-17T10:27:17Z

But if we're thinking in semver, 2.1 is not a major version. It's a minor version.

This is yet another reason (just realized this today) why I think Symfony does not really follow semver, despite their claims...

dunglas · 2017-05-17T11:06:26Z

Semver is about code API. Not about dependencies. Every major project (including Symfony, Doctrine...) do this and this is totally acceptable. It's also the only way to no being stuck with very old and versions.
It's not a big deal for users to upgrade to a new minor version of a project. If they use the dependency themselves, it must be explicitly defined in their composer.json to prevent problems.

teohhanhui · 2017-05-17T11:34:08Z

I don't know if we'll ever reach agreement about the technicality of whether bumping dependencies is a BC break, but in the spirit of semver, it'd be best if we can tag a major release when we drop support for an older minor version of Symfony.

In any case, stability flags only work at the project root level. So when we use @beta in require, that does nothing and will result in unfulfillable dependencies (because Symfony 3.3 is not released yet).

dunglas · 2017-05-17T11:37:55Z

You're right about stability flags, but it's just a temporary solution. We'll not release 2.1 before the release of Symfony 3.3.

teohhanhui · 2017-05-17T11:46:44Z

Looks like you're right about semver:

http://semver.org/#what-should-i-do-if-i-update-my-own-dependencies-without-changing-the-public-api

😄

…cator Add filter locator and deprecate filter collection

soyuka reviewed May 4, 2017

View reviewed changes

meyerbaptiste force-pushed the add_filter_locator branch 4 times, most recently from a3d11b5 to 30687bf Compare May 4, 2017 14:31

chalasr reviewed May 4, 2017

View reviewed changes

meyerbaptiste force-pushed the add_filter_locator branch from 30687bf to ee63f10 Compare May 4, 2017 15:07

soyuka reviewed May 4, 2017

View reviewed changes

This was referenced May 5, 2017

Backport #1014 (also includes #907) #1097

Merged

Fix Behat #1102

Merged

Simperfit approved these changes May 5, 2017

View reviewed changes

meyerbaptiste force-pushed the add_filter_locator branch from b6c70ea to b17b8e6 Compare May 5, 2017 14:47

soyuka mentioned this pull request May 5, 2017

Proposition: using annotation for filters #1033

Closed

meyerbaptiste force-pushed the add_filter_locator branch 7 times, most recently from 444b0a6 to fca4e6a Compare May 9, 2017 12:07

soyuka approved these changes May 10, 2017

View reviewed changes

soyuka added the [RFR] Ready for Review label May 10, 2017

soyuka reviewed May 10, 2017

View reviewed changes

chalasr approved these changes May 10, 2017

View reviewed changes

soyuka reviewed May 10, 2017

View reviewed changes

meyerbaptiste force-pushed the add_filter_locator branch from fca4e6a to 2d6e3a2 Compare May 10, 2017 13:11

dunglas reviewed May 10, 2017

View reviewed changes

meyerbaptiste force-pushed the add_filter_locator branch 3 times, most recently from 34041be to ff3f1ba Compare May 10, 2017 14:51

dunglas approved these changes May 10, 2017

View reviewed changes

soyuka mentioned this pull request May 13, 2017

have handle on GET/POST/DELETE/PUT method api-platform/api-platform#296

Closed

meyerbaptiste force-pushed the add_filter_locator branch 2 times, most recently from e9f4c29 to 9ca1d00 Compare May 15, 2017 12:04

Add filter locator and deprecate filter collection

08fc0a0

meyerbaptiste force-pushed the add_filter_locator branch from 9ca1d00 to 08fc0a0 Compare May 15, 2017 12:23

dunglas merged commit 8622477 into api-platform:master May 15, 2017

meyerbaptiste deleted the add_filter_locator branch May 15, 2017 13:11

hoangnd25 pushed a commit to hoangnd25/core that referenced this pull request Feb 23, 2018

Merge pull request api-platform#1099 from meyerbaptiste/add_filter_lo…

7a4828e

…cator Add filter locator and deprecate filter collection


		public function __construct(ResourceMetadataFactoryInterface $resourceMetadataFactory, Fil 8000 terCollection $filters)
		public function __construct(ResourceMetadataFactoryInterface $resourceMetadataFactory, $filterLocator)


		<service id="api_platform.filter_collection_factory" class="ApiPlatform\Core\Api\FilterCollectionFactory" public="false" />

		<service id="api_platform.filters" class="ApiPlatform\Core\Api\FilterCollection" public="false">

Uh oh!

Add filter locator and deprecate filter collection #1099

Add filter locator and deprecate filter collection #1099

Uh oh!

Conversation

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!

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!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!