Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add payload controller value resolver #3263

Merged
merged 1 commit into from
Jun 23, 2021

Conversation

julienfalque
Copy link
Contributor

@julienfalque julienfalque commented Nov 15, 2019

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tickets -
License -
Doc PR -

When creating a custom controller, the argument which is passed the deserialized payload of the request must be named $data. This can be very frustrating, especially when you don't know this requirement and you don't understand why your controller is not working properly.

This PR introduces a controller value resolver that allows to use any argument name, as long as its type declaration matches the resource class.

@dunglas
Copy link
Member

dunglas commented Nov 15, 2019

I like it!

@ro0NL
Copy link

ro0NL commented Nov 16, 2019

from #3268 , we do

<?php

use ApiPlatform\Core\Validator\ValidatorInterface;
use Sensio\Bundle\FrameworkExtraBundle\Configuration\ParamConverter;
use Sensio\Bundle\FrameworkExtraBundle\Request\ParamConverter\ParamConverterInterface;
use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\Serializer\SerializerInterface;

class ApiInputConverter implements ParamConverterInterface
{
    private $serializer;
    private $validator;

    public function __construct(SerializerInterface $serializer, ValidatorInterface $validator)
    {
        $this->serializer = $serializer;
        $this->validator = $validator;
    }

    public function apply(Request $request, ParamConverter $configuration)
    {
        $attribute = $configuration->getName();

        if ($request->attributes->has($attribute)) {
            return false;
        }

        $input = $this->serializer->deserialize($request->getContent(), $configuration->getClass(), $request->getRequestFormat());

        $this->validator->validate($input);

        $request->attributes->set($attribute, $input);

        return true;
    }

    public function supports(ParamConverter $configuration)
    {
        $options = $configuration->getOptions();

        if ($options['api_input_dto'] ?? false) {
            if (null === $configuration->getClass()) {
                throw new \LogicException('Parameter "'.$configuration->getName().'" must have a class type hint set to deserialize.');
            }

            return true;
        }

        return false;
    }
}

@dunglas
Copy link
Member

dunglas commented Nov 16, 2019

Do we really need the dependency to SensioExtraFramework? Symfony has some native Param Converter without this dependency for instance.

@ro0NL
Copy link

ro0NL commented Nov 17, 2019

AFAIK it's simply matter of setting a request attribute with deserialized/validated payload. Then we can map it to controller arguments, if needed.

To use different attributes names, i think implementing ArgumentValueResolverInterface will do indeed 👍

@julienfalque
Copy link
Contributor Author

Right, I forgot about ArgumentValueResolverInterface, I'll rework my PR.

@julienfalque julienfalque changed the title Add payload param converter Add payload controller value resolver Nov 17, 2019
@julienfalque julienfalque force-pushed the param-converter branch 5 times, most recently from 099fd09 to 9f3741b Compare November 18, 2019 07:51
@julienfalque
Copy link
Contributor Author

PR reworked.

I'd like to add some functional tests. Does it make sense to use Behat for that? I mean: what this PR introduces is more of a technical improvement than a real user feature.

@julienfalque
Copy link
Contributor Author

Added some integration tests.

@julienfalque julienfalque marked this pull request as ready for review November 1, 2020 14:15
Base automatically changed from master to main January 23, 2021 21:59
return null;
}

$context = $this->serializationContextBuilder->createFromRequest($request, false, $attributes);
Copy link
Member

@dunglas dunglas Nov 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you use the context builder? The current class should be available directly in the attributes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mmh indeed, resource_class is available in $attributes directly, but I'm not sure about input: the context builder seems to have some custom logic to retrieve it from operation configuration.

/**
* @dataProvider provideIntegrationCases
*/
public function testIntegration(Request $request, callable $controller, array $expectedArguments): void
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we add the integration tests in another file? If possible we prefer to use Behat for this kind of tests too (but maybe it would be harder to test it and these tests seem OK to maintain).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If possible we prefer to use Behat

I think it would be much harder to have reliable Behat tests and would require more work to write and maintain. Not worth it IMO.

Shouldn't we add the integration tests in another file

IMO it's fine to keep it here: all tests in this file cover the same subject. But if you prefer, I can extract it to it own file.

@alanpoulain alanpoulain added this to the 2.7 milestone Mar 18, 2021
CHANGELOG.md Outdated Show resolved Hide resolved
@alanpoulain
Copy link
Member

@julienfalque LGTM. Could you provide a documentation?

@alanpoulain alanpoulain changed the title Add payload controller value resolver feat: add payload controller value resolver Jun 23, 2021
@alanpoulain alanpoulain merged commit 26c99fa into api-platform:main Jun 23, 2021
@alanpoulain
Copy link
Member

Thank you @julienfalque.

@julienfalque
Copy link
Contributor Author

Thank you @alanpoulain @dunglas. Sorry I didn't update the documentation, I totally forgot about your request.

@julienfalque julienfalque deleted the param-converter branch July 21, 2021 08:44
LoicBoursin pushed a commit to LoicBoursin/core that referenced this pull request Aug 18, 2021
vincentchalamon pushed a commit to vincentchalamon/core that referenced this pull request Oct 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants