X-HTTP-Method-Override listener #81

Wilt · 2016-08-23T16:57:23Z

X-HTTP-Method-Override listener feature.
For this issue: zfcampus/zf-apigility#175 (comment)

michalbundyra · 2016-08-23T17:05:01Z

test/HttpMethodOverrideListenerTest.php

+
+        $result = $listener($event);
+        $this->assertEquals($method, $request->getMethod());
+


Please remove empty line above.

michalbundyra · 2016-08-23T17:08:48Z

src/HttpMethodOverrideListener.php

+        if (!in_array($method, $this->methods)) {
+            return new ApiProblemResponse(new ApiProblem(
+                400,
+                'unrecognized method in X-HTTP-Method-Ovverride header'


Please change to uppercase U in unrecognized word

Wilt · 2016-08-23T17:10:25Z

PR for this issue: zfcampus/zf-apigility#175 (comment)

I think enabling this listener should be optional, feels not good to connect such a feature by default to the application so I connected the listener depending on a x_http_method_override_enabled key in the config.

Not sure exactly on what is a valid priority value for this listener. I would say as early as possible, currently set to -40 to be triggered before all other zf-campus onRoute listeners, but might earlier might be even better. I added a TODO to check this value...

michalbundyra · 2016-08-23T17:15:38Z

src/HttpMethodOverrideListener.php

+        if (! in_array($method, $this->methods)) {
+            return new ApiProblemResponse(new ApiProblem(
+                400,
+                'unrecognized method in X-HTTP-Method-Ovverride header'


U in unrecognized please

Attached listener depending on `x_http_method_override_enabled` Added $xHttpMethodOverrideEnabled to ContentNegotiationOptions (defaults to false);

Wilt · 2016-08-24T07:57:54Z

Updated the PR:

Changed all according to @webimpress his comments (thanks)
Made HttpMethodOverrideListener extend AbstractListenerAggregate
Attached listener depending on x_http_method_override_enabled
Added $xHttpMethodOverrideEnabled to ContentNegotiationOptions (defaults to false);
Added and updated unit tests

snapshotpl · 2016-08-24T08:31:04Z

@Wilt there is already existing module https://github.com/rstgroup/http-method-override

Wilt · 2016-08-24T09:00:42Z

@snapshotpl OK, thanks.
If there is no interest to implement this as part of Apigility/zf-content-negotiation, then someone can just close and I will delete my PR.

michalbundyra · 2016-08-24T11:33:13Z

src/Factory/HttpMethodOverrideListenerFactory.php

+<?php
+/**
+ * @license   http://opensource.org/licenses/BSD-3-Clause BSD-3-Clause
+ * @copyright Copyright (c) 2014-2016 Zend Technologies USA Inc. (http://www.zend.com)


Please use only 2016 year in new files.

michalbundyra · 2016-08-24T11:39:04Z

@Wilt I like it, I think will be nice to have this functionality out-of-the-box. 👍

removed trailing comma removed space

Wilt · 2016-08-24T12:46:51Z

@webimpress Thanks for the feedback.

I changed to prophecy. I don't like the fact that the get method and getHttpOverrideMethods in my test aren't recognized. Is there a workaround?

michalbundyra · 2016-08-24T12:52:42Z

test/HttpMethodOverrideListenerTest.php

+            HttpRequest::METHOD_PATCH
+        ],
+        HttpRequest::METHOD_POST => [
+        ]


please add trailing comma

michalbundyra · 2016-08-24T12:55:23Z

@Wilt do you mean your IDE doesn't recognize these methods?
You can always add comment /** @var YourClass $variable */ before variable, and if you are using PHPStorm it will be recognized properly.
But then it's not 100% true, because prophecy object is ProphecyInterface, so maybe better if your comment will be:
/** @var YourClass|ProphecyInterface $variable */

added php doc

jdukleth · 2016-08-24T21:33:10Z

Looks great. I think this should be baked into Apigility.

weierophinney

This is solid - thanks, @Wilt !

weierophinney · 2016-10-11T21:09:09Z

src/HttpMethodOverrideListener.php

+
+    /**
+     * Priority is set very high (should be executed before all other listeners that rely on the request method value).
+     * TODO: Check priority value, maybe value should be even higher??


This is an appropriate value. The OPTIONS handling (i.e., HTTP method negotiation) happens at -100. Currently, we already have versioning (for Accept headers) happening at -40 (route-based version checking happens at -41). The main thing is that it happens before the OPTIONS handling, so this works fine.

weierophinney · 2016-10-11T21:10:39Z

src/Module.php

@@ -48,6 +48,11 @@ public function onBootstrap(MvcEvent $e)
        $services->get(AcceptFilterListener::class)->attach($eventManager);
        $services->get(ContentTypeFilterListener::class)->attach($eventManager);

+        $contentNegotiationOptions = $services->get(ContentNegotiationOptions::class);
+        if ($contentNegotiationOptions->getXHttpMethodOverrideEnabled()) {
+            $services->get(HttpMethodOverrideListener::class)->attach($eventManager);


I very much like this conditional attachment.

X-HTTP-Method-Override listener

Close #81

weierophinney · 2016-10-11T21:37:33Z

Thanks, @Wilt!

Wilt · 2016-10-27T09:48:40Z

You are very welcome!

X-HTTP-Method-Override listener

4677f11

michalbundyra reviewed Aug 23, 2016
View reviewed changes

Wilt added 3 commits August 23, 2016 19:07

Changes following @webimpress his comments

9db72f2

Changes following @webimpress his comments

e36c3eb

Changes following @webimpress his comments

da71cb0

michalbundyra reviewed Aug 23, 2016
View reviewed changes

Wilt added 2 commits August 24, 2016 09:19

Small code style improvements/typo fixes

4b1f500

Made HttpMethodOverrideListener extend AbstractListenerAggregate

ac6e42e

Attached listener depending on `x_http_method_override_enabled` Added $xHttpMethodOverrideEnabled to ContentNegotiationOptions (defaults to false);

Removed useless comment

d9586c5

Wilt added 4 commits August 24, 2016 12:06

Added configuration of http-override-methods

43768f7

Added configuration of http-override-methods

30887a7

Added HttpMethodOverrideListenerFactory test

37a4d74

Fix for travis test fail (line length)

3023a4a

michalbundyra reviewed Aug 24, 2016
View reviewed changes

Wilt added 2 commits August 24, 2016 14:41

changed test to use prophecy

0277845

removed trailing comma removed space

year 2016

8bac35c

michalbundyra reviewed Aug 24, 2016
View reviewed changes

added trailing commas

dabc4e0

added php doc

weierophinney added the enhancement label Oct 11, 2016

weierophinney added this to the 1.3.0 milestone Oct 11, 2016

weierophinney approved these changes Oct 11, 2016

View reviewed changes

weierophinney self-assigned this Oct 11, 2016

weierophinney added a commit that referenced this pull request Oct 11, 2016

Merge pull request #81 from Wilt/feature-xhttp-override-listener

c3db690

X-HTTP-Method-Override listener

weierophinney added a commit that referenced this pull request Oct 11, 2016

Added CHANGELOG for #81

ed4e1f5

weierophinney added a commit that referenced this pull request Oct 11, 2016

Merge branch 'feature/81' into develop

3dc5b35

Close #81

weierophinney closed this Oct 11, 2016

weierophinney removed the enhancement label Oct 11, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

X-HTTP-Method-Override listener #81

X-HTTP-Method-Override listener #81

Wilt commented Aug 23, 2016

michalbundyra Aug 23, 2016

michalbundyra Aug 23, 2016

Wilt commented Aug 23, 2016 •

edited

Loading

michalbundyra Aug 23, 2016

Wilt commented Aug 24, 2016 •

edited

Loading

snapshotpl commented Aug 24, 2016

Wilt commented Aug 24, 2016 •

edited

Loading

michalbundyra Aug 24, 2016

michalbundyra commented Aug 24, 2016

Wilt commented Aug 24, 2016

michalbundyra Aug 24, 2016

michalbundyra commented Aug 24, 2016

jdukleth commented Aug 24, 2016

weierophinney left a comment

weierophinney Oct 11, 2016

weierophinney Oct 11, 2016

weierophinney commented Oct 11, 2016

Wilt commented Oct 27, 2016 •

edited

Loading


		$result = $listener($event);
		$this->assertEquals($method, $request->getMethod());

X-HTTP-Method-Override listener #81

X-HTTP-Method-Override listener #81

Conversation

Wilt commented Aug 23, 2016

michalbundyra Aug 23, 2016

Choose a reason for hiding this comment

michalbundyra Aug 23, 2016

Choose a reason for hiding this comment

Wilt commented Aug 23, 2016 • edited Loading

michalbundyra Aug 23, 2016

Choose a reason for hiding this comment

Wilt commented Aug 24, 2016 • edited Loading

snapshotpl commented Aug 24, 2016

Wilt commented Aug 24, 2016 • edited Loading

michalbundyra Aug 24, 2016

Choose a reason for hiding this comment

michalbundyra commented Aug 24, 2016

Wilt commented Aug 24, 2016

michalbundyra Aug 24, 2016

Choose a reason for hiding this comment

michalbundyra commented Aug 24, 2016

jdukleth commented Aug 24, 2016

weierophinney left a comment

Choose a reason for hiding this comment

weierophinney Oct 11, 2016

Choose a reason for hiding this comment

weierophinney Oct 11, 2016

Choose a reason for hiding this comment

weierophinney commented Oct 11, 2016

Wilt commented Oct 27, 2016 • edited Loading

Wilt commented Aug 23, 2016 •

edited

Loading

Wilt commented Aug 24, 2016 •

edited

Loading

Wilt commented Aug 24, 2016 •

edited

Loading

Wilt commented Oct 27, 2016 •

edited

Loading