Add the ability to declare empty item operations by soyuka · Pull Request #3079 · api-platform/core

soyuka · 2019-09-18T11:59:45Z

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

soyuka · 2019-09-18T12:00:06Z

     * @return object
     */
-    public function __invoke($data)
+    public function __invoke($data = null)


Should I maybe do a new controller instead of changing this?

Yes, a new one that directly throw a 404 (so we can simplify the metadata).

dunglas · 2019-09-18T15:07:54Z


            $resourceMetadata = $resourceMetadata->withItemOperations($this->createOperations($methods));
        } else {
+            if (!$itemOperations) {


You must explicitly check for a get operation (only PUT could be defined for instance).

how can this be done without links to the Router? I can move the method guess from ApiLoader to metadata first, but when using a route_name, we won't be able to check the method without retrieving the route from the RouteCollection right?

when using a route_name, we won't be able to check the method without retrieving the route from the RouteCollection right?

Applying the 80/20 rule, I'd say let's do nothing when we encounter such edge cases?

teohhanhui · 2019-09-18T15:37:13Z

            $resourceMetadata = $resourceMetadata->withItemOperations($this->createOperations($methods));
        } else {
+            if (!$itemOperations) {
+                $itemOperations = ['placeholder' => ['method' => 'GET', 'read' => false, 'serialize' => false, 'status' => 404]];


The operation name should be get instead of placeholder.

Can't we use 'output' => false here?

I wanted to be able to differentiate the get operation but sure can do!

to be able to differentiate the get operation

I know. I'm against that. 😆

I think there should be nothing special about it. Just a good / safe default when the get item operation is missing.

soyuka · 2019-09-20T07:21:19Z

+{
+    public function __invoke()
+    {
+        return new Response(null, 404);


Would you prefer to throw an HttpException here?

Yes please!

Yes, it will trigger the custom error page system etc

dunglas · 2019-09-21T10:52:29Z

+        if ($this->needsDefaultGetOperation($resourceMetadata)) {
+            $resourceMetadata = $resourceMetadata->withItemOperations(array_merge(
+                $resourceMetadata->getItemOperations(),
+                ['get' => ['method' => 'GET', 'read' => false, 'serialize' => false, 'output' => ['class' => false], 'controller' => NotFoundAction::class]])


I think that serialize can be omitted, because the controller throw anyway.

dunglas · 2019-09-24T11:04:23Z

Could you open a docs PR for that @soyuka?

teohhanhui · 2019-09-24T11:18:04Z

+        if ($this->needsDefaultGetOperation($resourceMetadata)) {
+            $resourceMetadata = $resourceMetadata->withItemOperations(array_merge(
+                $resourceMetadata->getItemOperations(),
+                ['get' => ['method' => 'GET', 'read' => false, 'output' => ['class' => false], 'controller' => NotFoundAction::class]])


I believe you've made a mistake here, it's supposed to be null:

core/src/Metadata/Resource/Factory/InputOutputResourceMetadataFactory.php

Lines 95 to 97 in 81d276c

if (false === $attribute) {

return ['class' => null];

}

teohhanhui · 2019-09-24T11:18:13Z

        }

-        if (null === ($inputOrOutput['class'] ?? null)) {
+        if (false === ($inputOrOutput['class'] ?? false)) {


Should be reverted. See https://github.com/api-platform/core/pull/3079/files#r327557730

teohhanhui · 2019-09-24T11:19:58Z

+        $itemOperations = $resourceMetadata->getItemOperations();
+
+        foreach ($itemOperations as $itemOperation) {
+            if ('GET' === ($itemOperation['method'] ?? false)) {


Why not ?? null instead?

Actually, on second thought, this check is unsafe. There might be a custom GET item operation.

The only safe option seems to me to be for the user to declare this get operation themselves:

/** * @ApiResource( * itemOperations={ * "get"={ * "method"="GET", * "controller"=NotFoundAction::class, * "read"=false, * "output"=false, * }, * }, * ) */

Which to me seems like we should revert and make this a doc entry instead, while providing the NotFoundAction. (I'm of the opinion that we don't need another shortcut to declare this.)

We could document the custom get operation with one that disables it! I'm fixing what you said!

We could document the custom get operation with one that disables it!

What do you mean?

Anyway, I've given it more thought and it's still unsafe anyway. We don't know that there's not already a GET item operation because of route_name. So I really think we must revert this.

see api-platform/docs#886

you said it yourself it's not recommended to use route_name?

Yes, but if there's an operation using route_name which is the GET item operation, we could never tell, and we would be creating this get item operation with NotFoundAction. And there's nothing the user could do to fix that.

…y-item-operations" This reverts commit c277aeb, reversing changes made to 1088a5d.

Revert #3079

soyuka commented Sep 18, 2019

View reviewed changes

soyuka force-pushed the feature/empty-item-operations branch from b65fb5d to 373c57a Compare September 18, 2019 12:00

soyuka mentioned this pull request Sep 18, 2019

WIP: Ability to remove the item operation #2954

Closed

dunglas reviewed Sep 18, 2019

View reviewed changes

teohhanhui reviewed Sep 18, 2019

View reviewed changes

soyuka force-pushed the feature/empty-item-operations branch from 373c57a to 57236d6 Compare September 20, 2019 07:20

soyuka commented Sep 20, 2019

View reviewed changes

soyuka force-pushed the feature/empty-item-operations branch from 57236d6 to b026ae7 Compare September 20, 2019 07:52

soyuka mentioned this pull request Sep 20, 2019

Releases 2.4.3+ break support for using the default messenger serializer with the Messenger component #3065

Closed

soyuka force-pushed the feature/empty-item-operations branch 7 times, most recently from f5acf5d to 2f3ea1d Compare September 21, 2019 08:59

dunglas reviewed Sep 21, 2019

View reviewed changes

soyuka added 3 commits September 21, 2019 13:36

Ability to declare empty item operations

142f7da

JsonSchema: fix input metadata when disabled

15e996a

JsonSchema: better command output sanitizing

5aa6f20

soyuka force-pushed the feature/empty-item-operations branch from 2f3ea1d to 5aa6f20 Compare September 21, 2019 11:40

soyuka merged commit c277aeb into api-platform:master Sep 21, 2019

teohhanhui reviewed Sep 24, 2019

View reviewed changes

soyuka mentioned this pull request Sep 26, 2019

Fix disabled operation metadata #3093

Closed

teohhanhui added a commit to teohhanhui/api-platform-core that referenced this pull request Sep 26, 2019

Revert "Merge pull request api-platform#3079 from soyuka/feature/empt…

8de3f81

…y-item-operations" This reverts commit c277aeb, reversing changes made to 1088a5d.

teohhanhui mentioned this pull request Sep 26, 2019

Revert #3079 #3096

Merged

soyuka added a commit that referenced this pull request Sep 27, 2019

Merge pull request #3096 from teohhanhui/revert-3079

115cf92

Revert #3079

lukasluecke mentioned this pull request Sep 30, 2019

How to disable REST completely? #2796

Closed

ArnoudThibaut mentioned this pull request Oct 31, 2019

WIP : Allow to disable rest listeners #3230

Closed

3 tasks

Uh oh!

Uh oh!

Conversation

soyuka commented Sep 18, 2019

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dunglas commented Sep 24, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

teohhanhui Sep 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

teohhanhui Sep 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

teohhanhui Sep 24, 2019 •

edited

Loading

teohhanhui Sep 24, 2019 •

edited

Loading