Add a flag to disable all request listeners #909

dunglas · 2017-01-12T16:50:08Z

Q	A
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#880, #781, #782, #805
License	MIT
Doc PR	todo

To disable all API Platform request listeners (read, deserialize and validate), you have to create a new request attribute (for instance by setting the defaults key in a route definition) called api_request to false.

ping @lyrixx, @kbsali, @gpenverne.

dunglas · 2017-01-12T16:51:21Z

src/Bridge/Symfony/Validator/EventListener/ValidateListener.php

@@ -45,13 +45,17 @@ public function __construct(ValidatorInterface $validator, ResourceMetadataFacto
    public function onKernelView(GetResponseForControllerResultEvent $event)
    {
        $request = $event->getRequest();
+        if ($request->isMethodSafe(false) || $request->isMethod(Request::METHOD_DELETE)) {


This change isn't really related, but is a minor performance improvement.

lyrixx · 2017-01-12T16:56:42Z

Actually, in my context, I did not want to remove all listeners. juste the deserialization. I guess you will understand better my use case will "real code"

    /**
     * @Route(
     *     name="api_envs_add_member",
     *     path="/api/v2/envs/{env}/members",
     * )
     * @Method("POST")
     * @Security("is_granted('CONFIGURE', env)")
     */
    public function addMemberAction(Request $request, AbstractEnv $env)
    {
        $envMember = new EnvMember($this->getUser(), $env);

        $form = $this->container->get('form.factory')
            ->createNamed('', EnvMemberType::class, $envMember)
            ->handleRequest($request)
        ;

        $violations = $this->get('validator')->validate($envMember);

        if (0 !== count($violations)) {
            // use ApiPlatform\Core\Bridge\Symfony\Validator\Exception\ValidationException;
            throw new ValidationException($violations);
        }

        $this->get('profiler.server_sharing')->approve($this->getUser(), $env, $envMember->getEmail());

        // Fake until https://github.com/api-platform/core/issues/880 is fixed
        $request->attributes->set('_api_resource_class', AbstractEnv::class);
        $request->attributes->set('_api_item_operation_name', 'add member');

        return $env;
    }

dunglas · 2017-01-12T16:58:30Z

It removes only "request listeners" (write listeners), so it will do the trick (the serialization process will still work).

dunglas · 2017-01-12T17:00:45Z

Unrelated but:

        // Fake until https://github.com/api-platform/core/issues/880 is fixed
        $request->attributes->set('_api_resource_class', AbstractEnv::class);
        $request->attributes->set('_api_item_operation_name', 'add member');

It works when attributes are set manually but not when the router sets them? Weird.

lyrixx · 2017-01-12T17:05:54Z

It works when attributes are set manually but not when the router sets them? Weird.

Actually it's a big HACK

I get some data, in JSON that is not related to the Env (I mean, it's not a serialization of an env)
I process this thaht
I tell to API platfrom it an Env
I return the env ;)

so in practice

curl -X POST /../env-id/members -d "{email="xxx", ...}"
return the env serialized ;)

dunglas · 2017-01-12T17:23:37Z

Got it, sorry about that. This PR should fix your issue.

Simperfit

If you have some time to add that 0.05% of coverage , coveralls will be happy :).

Simperfit · 2017-01-12T21:05:55Z

src/Util/RequestAttributesExtractor.php

+        if (null === $apiRequest = $request->attributes->get('_api_request')) {
+            $result['request'] = true;
+        } else {
+            $result['request'] = (bool) $apiRequest;


What could be the value of this ? true / false or just false ?

true / false, 1 / 0 (when using a route to define the attribute).

I don't like this cast. It lets through more than what we want to support. We should use an explicit test.

Yes me too but the cast is mandatory to support the routing component < 3.2 (before this version, types wasn't supported and the user was able to put anything that can be casted to a boolean in such cases).

teohhanhui · 2017-01-13T03:55:59Z

I'm -1 because one could just create custom routes and controllers / actions outside of the API Platform system altogether. This is an unwelcome hack.

dunglas · 2017-01-13T07:20:00Z

It doesn't sound like a hack to me. What do you suggest to fix the issue raised by @lyrixx?
If it's outside the API Platform, it's not available in the generated docs (including the Hydra one).

dunglas · 2017-01-20T09:55:17Z

ping @teohhanhui, I would like to merge this asap.

teohhanhui · 2017-01-20T11:05:37Z

I'm still -1. If we proceed with this, sooner or later we'll be adding _api_read, _api_deserialize, _api_validate, _api_write, _api_serialize, _api_response etc.

This is a misfeature. If we want to allow for more flexibility, we should add extension points (if they are not already available) and/or the possibility to decorate the listeners. For example, I'm able to do what was requested in #781 by decorating the DeserializeListener. I suspect #880 can use a similar solution.

dunglas · 2017-01-24T20:59:41Z

@teohhanhui wanting to deal with the request "manually" without any preprocessing done by the framework is a very common use case and one of the most wanted feature (see the list of referenced tickets). Indeed it can already be done using extension points, but having to decorate services to do a so common operation is too much work for the developper IMO.
And it's not consistent, because we already allow to skip the post processing (using api_respond), so we need the same feature for the preprocessing.

I'm still -1. If we proceed with this, sooner or later we'll be adding _api_read, _api_deserialize, _api_validate, _api_write, _api_serialize, _api_response etc.

It's not gonna to happen. The point here is to skip entirely the preprocessing.

teohhanhui · 2017-01-25T04:57:34Z

Uhh, okay. I didn't notice there's api_respond. It should be fine then...

teohhanhui · 2017-01-25T04:58:56Z

src/Util/RequestAttributesExtractor.php

+        if (null === $apiRequest = $request->attributes->get('_api_request')) {
+            $result['request'] = true;
+        } else {
+            $result['request'] = (bool) $apiRequest;


I don't like this cast. It lets through more than what we want to support. We should use an explicit test.

teohhanhui · 2017-01-25T05:02:31Z

tests/EventListener/DeserializeListenerTest.php

@@ -61,6 +61,24 @@ public function testDoNotCallWhenRequestNotManaged()
        $listener->onKernelRequest($eventProphecy->reveal());
    }

+    public function testDoNotCallWhenRequestRequestIsFalse()


Perhaps testDoNotCallWhenRequestSkipRequestProcessing?

testDoNotCallWhenRequestSkipProcessing seems more accurate

It's only request processing that's being skipped, not all processing. In a similar fashion we could say "skip response processing".

Thinking more about it, we have api_respond, not api_response. So maybe it should be api_receive? @dunglas

And the corresponding term would be "skip receive"?

api_receive

Love it. I'll update the PR.

teohhanhui · 2017-01-25T05:03:03Z

tests/EventListener/ReadListenerTest.php

@@ -39,6 +39,24 @@ public function testNotAnApiPlatformRequest()
        $listener->onKernelRequest($event->reveal());
    }

+    public function testRequestIsFalse()


I think we should try to standardize the test function names (like above)...

dunglas · 2017-02-11T11:31:05Z

Attribute renamed _api_receive and new functional and unit tests added.

dunglas commented Jan 12, 2017

View reviewed changes

Simperfit approved these changes Jan 12, 2017

View reviewed changes

Simperfit reviewed Jan 12, 2017

View reviewed changes

teohhanhui suggested changes Jan 25, 2017

View reviewed changes

This was referenced Feb 10, 2017

_api_x_operation_name to bound them all api-platform/api-platform#228

Closed

[Question] Exposing custom endpoint #936

Closed

dunglas added 3 commits February 11, 2017 11:33

Add a flag to disable all request listeners

971e911

Fix CS

d73a9c4

Change the flag name to _api_receive

013ae68

dunglas force-pushed the disable_request_listener branch from f13c4c4 to 013ae68 Compare February 11, 2017 10:58

dunglas added 2 commits February 11, 2017 12:29

Add functional tests for custom operations

3404d0b

Fix CS

fa9cb3d

Improve coverage

cde03ca

soyuka approved these changes Feb 11, 2017

View reviewed changes

dunglas merged commit d65c84b into api-platform:master Feb 12, 2017

dunglas deleted the disable_request_listener branch February 12, 2017 18:33

soyuka pushed a commit to soyuka/core that referenced this pull request Mar 7, 2017

Add a flag to disable all request listeners (api-platform#909)

1a1d2bb

soyuka pushed a commit to soyuka/core that referenced this pull request Mar 13, 2017

Add a flag to disable all request listeners (api-platform#909)

de5e4c3

soyuka pushed a commit to teohhanhui/api-platform-core that referenced this pull request Mar 16, 2017

Add a flag to disable all request listeners (api-platform#909)

efa7036

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

Add a flag to disable all request listeners (api-platform#909)

e216b33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a flag to disable all request listeners #909

Add a flag to disable all request listeners #909

dunglas commented Jan 12, 2017

dunglas Jan 12, 2017

lyrixx commented Jan 12, 2017

dunglas commented Jan 12, 2017 •

edited

dunglas commented Jan 12, 2017 •

edited

lyrixx commented Jan 12, 2017

dunglas commented Jan 12, 2017

Simperfit left a comment

Simperfit Jan 12, 2017

dunglas Jan 12, 2017

teohhanhui Jan 25, 2017

dunglas Jan 25, 2017 •

edited

teohhanhui commented Jan 13, 2017

dunglas commented Jan 13, 2017

dunglas commented Jan 20, 2017

teohhanhui commented Jan 20, 2017

dunglas commented Jan 24, 2017

teohhanhui commented Jan 25, 2017

teohhanhui Jan 25, 2017

teohhanhui Jan 25, 2017

Simperfit Jan 25, 2017

teohhanhui Jan 25, 2017

teohhanhui Jan 25, 2017

teohhanhui Jan 25, 2017

dunglas Jan 25, 2017

teohhanhui Jan 25, 2017

dunglas commented Feb 11, 2017

Add a flag to disable all request listeners #909

Add a flag to disable all request listeners #909

Conversation

dunglas commented Jan 12, 2017

Choose a reason for hiding this comment

lyrixx commented Jan 12, 2017

dunglas commented Jan 12, 2017 • edited

dunglas commented Jan 12, 2017 • edited

lyrixx commented Jan 12, 2017

dunglas commented Jan 12, 2017

Simperfit left a 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 Jan 25, 2017 • edited

Choose a reason for hiding this comment

teohhanhui commented Jan 13, 2017

dunglas commented Jan 13, 2017

dunglas commented Jan 20, 2017

teohhanhui commented Jan 20, 2017

dunglas commented Jan 24, 2017

teohhanhui commented Jan 25, 2017

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dunglas commented Feb 11, 2017

dunglas commented Jan 12, 2017 •

edited

dunglas commented Jan 12, 2017 •

edited

dunglas Jan 25, 2017 •

edited