Deserialize in a view listener #584
Conversation
dunglas
force-pushed
the
deserializer_view_listener
branch
from
8611730 to
77c46ac
Compare
dunglas
changed the title
|
ping @api-platform/core-team I took advantage of this PR to transform the get ride of one more trait (@theofidry and @sroze will be happy) and to improve the unit test coverage. |
|
I could only peak a glance for now but looks like a good refactoring 👍👍👍 |
|
👍 |
| namespace ApiPlatform\Core\Action; | ||
|
|
||
| /** | ||
| * Empty action. Useful to trigger the kernel.view event without doing anything specific in the action |
There was a problem hiding this comment.
I don't see the value of having this. It's far too easy to create an empty action, so there's no benefit of reuse here. This should just be PostCollectionAction, as before... And we will be free to change it maybe to do something later on without the name tying it down.
There was a problem hiding this comment.
It allows to create custom operations triggering the event system with only config. It's why I've renamed it. Why not doing it now?
There was a problem hiding this comment.
Why not go all the way and remove all other actions? 😆
So we will end up with an event listener which fetches data from the data provider, and another which performs the deserialization (if applicable).
There was a problem hiding this comment.
What is the benefit of getting data from a listener (which is triggered at every request) instead of in an action?
There was a problem hiding this comment.
You're right. We should probably not try to fight against how Symfony works.
There was a problem hiding this comment.
The benefit is turtles all the way (kind of... at least, events all the way). And we sidestep the confusion of what the controller is supposed to do by consistently leaving it out of the picture (as a no-op).
There was a problem hiding this comment.
Can we try that in another PR?
|
@dunglas Just realized we do not need to change this to achieve the use case in #583 (comment). Just create a custom normalizer and call the voter from if (isset($context['object_to_populate'])) {
if (!$this->authorizationChecker->isGranted('EDIT', $context['object_to_populate'])) {
throw new AccessDeniedException();
}
} |
| $context['object_to_populate'] = $data; | ||
| } | ||
|
|
||
| $event->setControllerResult($this->serializer->deserialize($content, $resourceClass, $format, $context)); |
There was a problem hiding this comment.
What prevents this from being run on safe methods (e.g. GET)?
There was a problem hiding this comment.
(In the first implementation I was listing non-safe methods but this trick looks more extensible to me).
There was a problem hiding this comment.
I don't think we should make such an assumption, especially when this Symfony test was reverted so we're basically on shaky ground: symfony/symfony@58fcb8d
|
@teohhanhui right but IMO it's not the responsibility of a denormalizer to call the security component (its responsibility is only to hydrate an object from a document). I think this PR provides a better extension point. WDYT? |
Agreed. So I suppose point no. 2 in #583 (comment) is bad too... But let's continue that discussion in #583 |
dunglas
force-pushed
the
deserializer_view_listener
branch
from
44a46e6 to
100cabd
Compare
…ew_listener Deserialize in a view listener
Unit tests must be fixed.