Better HTTP headers #2039

jewome62 · 2018-06-25T15:16:08Z

Q	A
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	yes, i add one
Tests pass?	no, but i work on this
Fixed tickets	#1912
License	MIT
Doc PR	none

soyuka · 2018-06-25T15:31:12Z

src/EventListener/WriteListener.php


-    public function __construct(DataPersisterInterface $dataPersister)
+    public function __construct(DataPersisterInterface $dataPersister, IriConverterInterface $iriConverter)


this is a BC break

The second argument must be made optional. 😄

But we can trigger a deprecation error if it's not provided.

haha sorry yeah I should've say what you said in addition 😆

teohhanhui · 2018-06-25T16:00:24Z

src/EventListener/RespondListener.php

+            'X-Frame-Options' => 'deny',
+        ];
+
+        if($request->isMethod('POST') || $request->isMethod('PUT')){


Should be the same for PATCH, I think...

teohhanhui · 2018-06-25T16:02:46Z

src/EventListener/WriteListener.php


-    public function __construct(DataPersisterInterface $dataPersister)
+    public function __construct(DataPersisterInterface $dataPersister, IriConverterInterface $iriConverter)


The second argument must be made optional. 😄

But we can trigger a deprecation error if it's not provided.

teohhanhui · 2018-06-25T16:04:14Z

src/EventListener/WriteListener.php

@@ -57,6 +60,7 @@ public function onKernelView(GetResponseForControllerResultEvent $event)
                }

                $event->setControllerResult($persistResult ?? $controllerResult);
+                $request->attributes->set('_iri_item', $this->iriConverter->getIriFromItem($controllerResult));


Maybe _api_write_item_iri?

teohhanhui · 2018-06-25T16:44:25Z

src/EventListener/WriteListener.php

@@ -25,10 +26,12 @@
 class WriteListener


Also, I think we could mark this class as final? It was never designed to be extended in the first place...

(Strictly speaking, it'd be a BC break... But I highly doubt it'd bother anyone. I don't remember if we've done something like this before. Do we just add the @final in PHPDoc?)

or @internal ?

Let's add @final in the PHPDoc.

Let's leave this to #2046 then. 😄

jewome62 · 2018-06-25T18:37:17Z

OK OK, I confess, I fucked up, I did not think and I pushed my work not finished by putting WIP tag

DO NOT SHOOT ME ! I HAVE A WIFE AND CHILDREN !

;)

jewome62 · 2018-06-25T23:44:00Z

@teohhanhui
All is good here,
The fail test became from last release of Symfony and affect master too

See: #2043

teohhanhui · 2018-06-26T12:20:56Z

src/EventListener/RespondListener.php

+            'X-Frame-Options' => 'deny',
+        ];
+
+        if ($request->isMethod('POST') || $request->isMethod('PUT') || $request->isMethod('PATCH')) {


Please use \in_array here.

teohhanhui · 2018-06-26T12:22:52Z

src/EventListener/WriteListener.php

@@ -25,10 +26,12 @@
 class WriteListener


Let's add @final in the PHPDoc.

teohhanhui · 2018-06-26T12:48:56Z

The error messages need to be rephrased. I'll give some suggestions, if somebody doesn't get it done before I do.

teohhanhui · 2018-06-26T17:43:44Z

src/EventListener/RespondListener.php

+                    $headers['Location'] = $request->attributes->get('_api_write_item_iri');
+                }
+            } else {
+                @trigger_error(sprintf('No request attribute from `_api_write_item_iri` key is deprecated since API Platform 2.3 and will not be supported in API Platform 3, an string should always be returned. see deprecated into %s constructor for more details.', WriteListener::class), E_USER_DEPRECATED);


Not sure if we should have this deprecation error here, but:

Not setting the _api_write_item_iri request attribute is deprecated since API Platform 2.3 and will not be supported in API Platform 3. See %s for the default implementation.

jewome62 · 2018-06-27T08:05:04Z

@teohhanhui I make the fix.

Anyway, @dunglas said me that we are trying to reduce the dependencies in the Platform API classes, so normally I will remove the deprecated ones to make sure that it remains optional definitively.

teohhanhui

Can you squash the commits? 😄

teohhanhui · 2018-06-27T12:53:16Z

src/EventListener/WriteListener.php

@@ -25,10 +26,12 @@
 class WriteListener


Let's leave this to #2046 then. 😄

jewome62 · 2018-06-28T13:13:33Z

I have rebase from master

teohhanhui · 2018-06-28T14:05:21Z

src/EventListener/WriteListener.php

 use ApiPlatform\Core\DataPersister\DataPersisterInterface;
 use Symfony\Component\HttpKernel\Event\GetResponseForControllerResultEvent;

 /**
 * Bridges persistense and the API system.
 *
+ * @final


Should be removed.

soyuka · 2018-07-13T14:12:49Z

@dunglas may you merge this please? I rebased it.

teohhanhui · 2018-07-19T17:42:51Z

Thanks @jewome62! 🎉

soyuka reviewed Jun 25, 2018

View reviewed changes

teohhanhui suggested changes Jun 25, 2018

View reviewed changes

teohhanhui reviewed Jun 25, 2018

View reviewed changes

jewome62 force-pushed the feature/better-http-headers branch 3 times, most recently from 28621b0 to 5c8abe3 Compare June 25, 2018 20:05

jewome62 changed the title ~~[WIP] Better HTTP headers~~ Better HTTP headers Jun 25, 2018

teohhanhui suggested changes Jun 26, 2018

View reviewed changes

teohhanhui reviewed Jun 26, 2018

View reviewed changes

teohhanhui suggested changes Jun 27, 2018

View reviewed changes

src/EventListener/WriteListener.php

@@ -25,10 +26,12 @@

class WriteListener

Copy link

Contributor

teohhanhui Jun 27, 2018

Choose a reason for hiding this comment

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

Let's leave this to #2046 then. 😄

jewome62 force-pushed the feature/better-http-headers branch 2 times, most recently from bd333a9 to d8a707d Compare June 28, 2018 13:11

jewome62 force-pushed the feature/better-http-headers branch from d8a707d to 592891e Compare June 28, 2018 13:47

soyuka approved these changes Jun 28, 2018

View reviewed changes

jewome62 force-pushed the feature/better-http-headers branch from 592891e to d361494 Compare June 28, 2018 14:02

teohhanhui reviewed Jun 28, 2018

View reviewed changes

jewome62 force-pushed the feature/better-http-headers branch from d361494 to 7507d90 Compare June 28, 2018 14:06

teohhanhui approved these changes Jun 28, 2018

View reviewed changes

soyuka force-pushed the feature/better-http-headers branch 2 times, most recently from 66898c9 to 7f046fd Compare July 13, 2018 13:48

Add Iri by listener

cb26a49

teohhanhui force-pushed the feature/better-http-headers branch from 7f046fd to cb26a49 Compare July 19, 2018 17:30

teohhanhui merged commit 797cea6 into api-platform:master Jul 19, 2018

jewome62 deleted the feature/better-http-headers branch July 30, 2018 09:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Better HTTP headers #2039

Better HTTP headers #2039

jewome62 commented Jun 25, 2018 •

edited

Loading

soyuka Jun 25, 2018

teohhanhui Jun 25, 2018

soyuka Jun 25, 2018

teohhanhui Jun 25, 2018

teohhanhui Jun 25, 2018

teohhanhui Jun 25, 2018

teohhanhui Jun 25, 2018 •

edited

Loading

jewome62 Jun 25, 2018

teohhanhui Jun 26, 2018

jewome62 Jun 26, 2018

jewome62 Jun 27, 2018

teohhanhui Jun 27, 2018

jewome62 commented Jun 25, 2018 •

edited

Loading

jewome62 commented Jun 25, 2018

teohhanhui Jun 26, 2018

teohhanhui Jun 26, 2018

teohhanhui commented Jun 26, 2018

teohhanhui Jun 26, 2018

jewome62 commented Jun 27, 2018

teohhanhui left a comment

teohhanhui Jun 27, 2018

jewome62 commented Jun 28, 2018

teohhanhui Jun 28, 2018

soyuka commented Jul 13, 2018

teohhanhui commented Jul 19, 2018


		public function __construct(DataPersisterInterface $dataPersister)
		public function __construct(DataPersisterInterface $dataPersister, IriConverterInterface $iriConverter)

Better HTTP headers #2039

Better HTTP headers #2039

Conversation

jewome62 commented Jun 25, 2018 • 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

teohhanhui Jun 25, 2018 • 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

jewome62 commented Jun 25, 2018 • edited Loading

jewome62 commented Jun 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teohhanhui commented Jun 26, 2018

Choose a reason for hiding this comment

jewome62 commented Jun 27, 2018

teohhanhui left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jewome62 commented Jun 28, 2018

Choose a reason for hiding this comment

soyuka commented Jul 13, 2018

teohhanhui commented Jul 19, 2018

jewome62 commented Jun 25, 2018 •

edited

Loading

teohhanhui Jun 25, 2018 •

edited

Loading

jewome62 commented Jun 25, 2018 •

edited

Loading