Generator compat improvements #1429

greg0ire · 2017-10-12T16:50:31Z

Q	A
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#1422
License	MIT
Doc PR	api-platform/docs#321

This PR should enable people to use generators when implementing DataCollectionProviderInterface

TODO

the same with the item interface, although it's not really needed?
implement the new interface everywhere we can
the doc PR

greg0ire · 2017-10-12T16:57:08Z

src/DataProvider/ChainCollectionDataProvider.php

                return $dataProvider->getCollection($resourceClass, $operationName);
            } catch (ResourceClassNotSupportedException $e) {
+                @trigger_error(sprintf('Throwing a "%s" is deprecated in favor of implementing "%s"', get_class($e), RestrictedDataProviderInterface::class), E_USER_DEPRECATED);


Following the sf style (or lack thereof?) here... yuck!

Why not ResourceClassNotSupportedException::class constant? Also, could you mention "Throwing a "..." in a data provider is deprecated..."?

Can you just make this change and we'll merge.

Oh sure I forgot. Let's see what those constants are

Oh right I understand now that I see the code in my editor, I just put the wrong class constant. I could have understood this earlier had it been legible on github :P
Fun fact: this line does not even fit on a 1920x1200 display.

Fixed @dunglas

dunglas · 2017-10-12T18:05:34Z

Looks good to me.

the same with the item interface, although it's not really needed?

We should do it at least for consistency.

greg0ire · 2017-10-12T22:39:59Z

@dunglas done, but it's no longer possible not to support something based on $id (no big deal IMO), or $context (no idea what this is about).

dunglas · 2017-10-13T06:06:36Z

Shouldn't the builtin Doctrine providers implement this new interface?

greg0ire · 2017-10-13T06:15:51Z

@dunglas they should, and the build should probably not pass because of the deprecations, should it? something seems to be wrong...

greg0ire · 2017-10-13T06:16:38Z

Oh wait no it's normal, they are not called when testing the chain providers

soyuka · 2017-10-13T07:56:32Z

tests/Bridge/Doctrine/Orm/CollectionDataProviderTest.php

-    /**
-     * @expectedException \ApiPlatform\Core\Exception\ResourceClassNotSupportedException
-     */
-    public function testThrowResourceClassNotSupportedException()


Can we keep this test anyway to ensure backward compatibility?

Nevermind I saw it in the chain!

soyuka · 2017-10-13T07:58:57Z

It's not really a bug fix or is it? I think it should target master. Anyway it's a nice improvement!

greg0ire · 2017-10-13T08:08:07Z

I should probably call supports inside the other methods and throw if it returns false, shouldn't I?

soyuka · 2017-10-13T08:13:48Z

What "other methods" do you had in mind? The chain provider continues when an exception is catched so this looks fine!

greg0ire · 2017-10-13T08:23:22Z

@soyuka getCollection and getItem... just in case someone decides to use this from their own code.

soyuka · 2017-10-13T08:28:00Z

Nope, the service exposed is the ChainDataProvider:

        <service id="api_platform.item_data_provider" class="ApiPlatform\Core\DataProvider\ChainItemDataProvider" />

greg0ire · 2017-10-13T08:45:19Z

Doesn't prevent people from just instanciating the class and misusing it, but I'm not sure you require that level of defensiveness

soyuka · 2017-10-13T09:33:12Z

Maybe we can consider flagging the doctrine data providers as Internal then. Also, they may be final (I see no reason to extend them). In the docs the recommended way is to implement the DataProviderInterface.

greg0ire · 2017-10-13T10:56:03Z

Final would be a BC break though

greg0ire · 2017-10-19T08:45:39Z

@sroze @julienfalque @soyuka can we have your stance on this? We were wondering if there wouldn't be a better way to add support for generators, without deprecating anything / introducing a new interface. Technically, right now, you can achieve that by writing something like this:

<?php
public function getCollection()
{
    // conditionally throw exception here, just like before
    return $this->getGenerator();
}

private function getGenerator();
{
    yield new Item;
}

But this feels hackish to me, I don't think we should recommend that.

soyuka · 2017-10-19T10:17:35Z

I like the clean way you added in this PR and I won't mind a deprecation on the subject especially if we remove an exception (try/catching is slow)

dunglas · 2017-10-19T10:21:42Z

ping @api-platform/core-team

soyuka · 2017-10-19T12:15:50Z

@dunglas do you consider this as a bug fix rather then a new feature?

dunglas · 2017-10-19T12:29:11Z

It introduces deprecation so it should be merged in master.

You cannot throw and return.

greg0ire · 2017-10-19T12:42:28Z

@dunglas rebased

greg0ire · 2017-10-19T12:46:59Z

As well as the docs PR

meyerbaptiste

Great feature!

soyuka · 2017-10-19T15:27:17Z

Tests fails though:

-%A  Throwing a "ApiPlatform\Core\Exception\ResourceClassNotSupportedException" is deprecated in favor of implementing "ApiPlatform\Core\DataProvider\RestrictedDataProviderInterface"
+  Throwing a "ApiPlatform\Core\Exception\ResourceClassNotSupportedException" in a data provider is deprecated in favor of implementing "ApiPlatform\Core\Exception\ResourceClassNotSupportedException"

This should make the collection data provider generator-friendly, because it will allow deferring code execution to the piece of code where the generator is iterated over instead of requiring it to be executed inside the try/catch block. Fixes api-platform#1422

greg0ire · 2017-10-19T15:35:52Z

src/DataProvider/ChainCollectionDataProvider.php

                return $dataProvider->getCollection($resourceClass, $operationName);
            } catch (ResourceClassNotSupportedException $e) {
+                @trigger_error(sprintf('Throwing a "%s" in a data provider is deprecated in favor of implementing "%s"', ResourceClassNotSupportedException::class, RestrictedDataProviderInterface::class), E_USER_DEPRECATED);


@soyuka I misunderstood your comment at first. I hope you all see how easier it would have been had each argument been on its own line: you could have commented directly under get_class($e). BTW, the sf standard is to have the message on one line, but is the sprintf part of the message? If yes, that's really a stupid rule who just made us loose precious time. It just makes life harder for everyone for no good reason.

meyerbaptiste · 2017-10-20T09:57:41Z

Thanks @greg0ire!

* GraphQL Query support (api-platform#1358) * Fix missing service when no Varnish URL is defined * [PropertyFilter] Fix whitelist comparison (api-platform#1379) * Remove wrong doc * Swagger subcollection documentation issue (api-platform#1395) * Make the requirements configurable Closes api-platform#1401 * Provide a better exception message and type Getting "Not found" on a route where you are getting an object can get really confusing. * Bump branch alias to 2.2.x-dev 2.1.x-dev is already taken by the 2.1 branch, and this should represent the next minor version anyway. * Fix tests * Reuse PriorityTaggedServiceTrait from symfony * Improve payload support and remove duplicate code in ConstraintViolationListNormalizer (api-platform#1416) * Filter Annotation implementation Parent class filters (needs test) Support for nested properties Tests Fix some comments and remove id=>id in compiler pass * Throw on abstract data providers / filters * Remove an unused var * Remove useless badges * Enable the coverage * Fix some quality issues * Add job to test upstream libs deprecations If api-platform uses upstream libs in a deprecated way, we should be aware of it. * Add job to test upstream libs deprecations If api-platform uses upstream libs in a deprecated way, we should be aware of it. * Fix missing cache tag on empty collections * Allow plain IDs with `allow_plain_identifiers` * Fix indentation for GraphQL features. * Add JSON API basic support (api-platform#785, api-platform#1036, api-platform#1175) * Clean Behat tests * Fix tags addition with an empty value * Document swagger-specific description options … and where to find them should there be newer ones. * Fix PHPUnit tests * Fix missing return statement * Support & compatibility for PHP7.2 * Add feature to update swagger context for properties * Generator compat improvements (api-platform#1429) * Add support for resource names without namespace * [SF 4.0] Make actions explicitly public * Allow phpdocumentor/reflection-docblock 4 * fix hydra documentation normalizer with subresources * Create a base collection normalizer * Fix request auto-runner * Update the changelog * Update changelog

greg0ire changed the base branch from master to 2.1 October 12, 2017 16:50

greg0ire commented Oct 12, 2017

View reviewed changes

greg0ire force-pushed the generator_compat_improvements branch from 4334e57 to 4703ced Compare October 12, 2017 17:03

greg0ire force-pushed the generator_compat_improvements branch from 4703ced to 00455fc Compare October 12, 2017 22:38

greg0ire force-pushed the generator_compat_improvements branch from 00455fc to 6b83e0c Compare October 12, 2017 22:48

dunglas requested a review from soyuka October 13, 2017 06:04

greg0ire force-pushed the generator_compat_improvements branch from 6b83e0c to abcad30 Compare October 13, 2017 06:38

soyuka reviewed Oct 13, 2017

View reviewed changes

soyuka approved these changes Oct 19, 2017

View reviewed changes

Remove useless call

2865274

You cannot throw and return.

greg0ire force-pushed the generator_compat_improvements branch from abcad30 to ad1f93b Compare October 19, 2017 12:42

greg0ire changed the base branch from 2.1 to master October 19, 2017 12:42

meyerbaptiste approved these changes Oct 19, 2017

View reviewed changes

greg0ire force-pushed the generator_compat_improvements branch from ad1f93b to 18c37ff Compare October 19, 2017 14:12

greg0ire force-pushed the generator_compat_improvements branch from 18c37ff to cecc022 Compare October 19, 2017 15:31

greg0ire commented Oct 19, 2017

View reviewed changes

meyerbaptiste added 3 commits October 20, 2017 11:09

Merge branch 'master' into generator_compat_improvements

333082c

Update ChainCollectionDataProviderTest.php

01bb31f

Update ChainCollectionDataProviderTest.php

2e28775

meyerbaptiste merged commit ad14ac0 into api-platform:master Oct 20, 2017

greg0ire deleted the generator_compat_improvements branch October 20, 2017 10:18

meyerbaptiste mentioned this pull request Oct 24, 2017

CollectionDataProviderInterface does not play well with generators #1422

Closed

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

Generator compat improvements (api-platform#1429)

beb3fce

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generator compat improvements #1429

Generator compat improvements #1429

greg0ire commented Oct 12, 2017 •

edited

Loading

greg0ire Oct 12, 2017 •

edited

Loading

soyuka Oct 13, 2017

dunglas Oct 19, 2017

greg0ire Oct 19, 2017

greg0ire Oct 19, 2017

greg0ire Oct 19, 2017

dunglas commented Oct 12, 2017

greg0ire commented Oct 12, 2017

dunglas commented Oct 13, 2017

greg0ire commented Oct 13, 2017

greg0ire commented Oct 13, 2017

soyuka Oct 13, 2017

soyuka Oct 13, 2017

soyuka commented Oct 13, 2017

greg0ire commented Oct 13, 2017

soyuka commented Oct 13, 2017

greg0ire commented Oct 13, 2017

soyuka commented Oct 13, 2017

greg0ire commented Oct 13, 2017

soyuka commented Oct 13, 2017

greg0ire commented Oct 13, 2017

greg0ire commented Oct 19, 2017 •

edited

Loading

soyuka commented Oct 19, 2017

dunglas commented Oct 19, 2017

soyuka commented Oct 19, 2017

dunglas commented Oct 19, 2017

greg0ire commented Oct 19, 2017

greg0ire commented Oct 19, 2017

meyerbaptiste left a comment

soyuka commented Oct 19, 2017 •

edited

Loading

greg0ire Oct 19, 2017

meyerbaptiste commented Oct 20, 2017

Generator compat improvements #1429

Generator compat improvements #1429

Conversation

greg0ire commented Oct 12, 2017 • edited Loading

TODO

greg0ire Oct 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dunglas commented Oct 12, 2017

greg0ire commented Oct 12, 2017

dunglas commented Oct 13, 2017

greg0ire commented Oct 13, 2017

greg0ire commented Oct 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soyuka commented Oct 13, 2017

greg0ire commented Oct 13, 2017

soyuka commented Oct 13, 2017

greg0ire commented Oct 13, 2017

soyuka commented Oct 13, 2017

greg0ire commented Oct 13, 2017

soyuka commented Oct 13, 2017

greg0ire commented Oct 13, 2017

greg0ire commented Oct 19, 2017 • edited Loading

soyuka commented Oct 19, 2017

dunglas commented Oct 19, 2017

soyuka commented Oct 19, 2017

dunglas commented Oct 19, 2017

greg0ire commented Oct 19, 2017

greg0ire commented Oct 19, 2017

meyerbaptiste left a comment

Choose a reason for hiding this comment

soyuka commented Oct 19, 2017 • edited Loading

Choose a reason for hiding this comment

meyerbaptiste commented Oct 20, 2017

greg0ire commented Oct 12, 2017 •

edited

Loading

greg0ire Oct 12, 2017 •

edited

Loading

greg0ire commented Oct 19, 2017 •

edited

Loading

soyuka commented Oct 19, 2017 •

edited

Loading