Skip to content

Support for HTTP PATCH in the built-in webserver - #190

Closed
nikcorg wants to merge 1 commit into
php:masterfrom
nikcorg:http-patch-method
Closed

Support for HTTP PATCH in the built-in webserver#190
nikcorg wants to merge 1 commit into
php:masterfrom
nikcorg:http-patch-method

Conversation

@nikcorg

@nikcorg nikcorg commented Sep 11, 2012

Copy link
Copy Markdown
Contributor

I wanted to experiment using the HTTP PATCH method (http://tools.ietf.org/html/rfc5789), so I added it to the http parser for the built-in webserver. Perhaps someone else would benefit from this as well.

Somewhat related to Bug #61679 (https://bugs.php.net/bug.php?id=61679), but this PR does not add the behaviour of returning 405 for unsupported methods.

@lstrojny

Copy link
Copy Markdown
Contributor

Patch and test looks good. Would you mind adding the Method Not Allowed change as well?

@nikcorg

nikcorg commented Sep 12, 2012

Copy link
Copy Markdown
Contributor Author

I could definitely take a look at it. Well, I already did, but decided against doing it in this PR, because it requires some changes outside the parser as well. I thought it would be better to keep patches as small as possible. How would you feel if I were to submit that as a separate PR, or would you like to include it in this one?

Actually, looking at RFC 2616, chapter 5.1.1 states as follows:

An origin server SHOULD return the status code 405 (Method Not Allowed) if the method is known by the origin server but not allowed for the requested resource, and 501 (Not Implemented) if the method is unrecognized or not implemented by the origin server.

So it's really 501 which should be returned for unknown methods. But the mechanics remain unchanged.

The same section also mentions the Allow header, which can be used to list all allowed methods. But perhaps that should be left to userland code. Certainly it has nothing to do with the parser anyway.

@lstrojny

Copy link
Copy Markdown
Contributor

Separate PR is fine as well. And you are right, 501 is correct here.

@ircmaxell

Copy link
Copy Markdown
Contributor

Looks good to me. Nice, simple, to the point...

@lstrojny

Copy link
Copy Markdown
Contributor

Alright, will take of it this weekend. Checked with stas, we also merge into 5.4

@lstrojny

Copy link
Copy Markdown
Contributor

@nikcorg could you rebase and squash your commits?

@nikcorg

nikcorg commented Sep 14, 2012

Copy link
Copy Markdown
Contributor Author

@lstrojny I've never used rebase before and it looks a bit like black magic, but I'll do my best...

A bit worried after reading this:

Do not rebase commits that you have pushed to a public repository.

If you follow that guideline, you’ll be fine. If you don’t, people will hate you, and you’ll be scorned by friends and family.

@stloyd

stloyd commented Sep 14, 2012

Copy link
Copy Markdown

@nikcorg Check this: http://gitready.com/advanced/2009/02/10/squashing-commits-with-rebase.html

In your case it would be:

$ git rebase -i HEAD~2

Later just push --force and you're done =)

Added support for the HTTP PATCH method

Added test for HTTP PATCH method support
@nikcorg

nikcorg commented Sep 14, 2012

Copy link
Copy Markdown
Contributor Author

@stloyd Thanks. I got the rebase done ok (I think), but I missed the push --force, so I pulled and pushed now everything looks like it's twisted to hell and back.

Why is it adding commits when I squash them? In my git log it only shows up as one commit.

Edit: No wait, it seems to have fixed itself. Thanks a lot for your help!

@lstrojny

Copy link
Copy Markdown
Contributor

@nikcorg the pull/push applied the remote changes again on top of your repo. Have a look at the history (git log), than do git reset --hard to the squashed commit. And than do git push --force.

@nikcorg

nikcorg commented Sep 14, 2012

Copy link
Copy Markdown
Contributor Author

@lstrojny Thanks, I'll do that next time. The log looks fine now, except the commit for the squash is now a bit strange, since I rebased twice.

Oh well. You live and you learn, collecting bruises and scratches every chance you get.

@lstrojny

Copy link
Copy Markdown
Contributor

I’m a little behind my schedule. Give me a few days more and it will be merged.

@nikcorg

nikcorg commented Sep 17, 2012

Copy link
Copy Markdown
Contributor Author

Sure thing, no worries.

@php-pulls

Copy link
Copy Markdown

Comment on behalf of lstrojny at php.net:

Merged in 5.4 and master.

@php-pulls php-pulls closed this Sep 18, 2012
@dsp

dsp commented Sep 26, 2012

Copy link
Copy Markdown
Member

any reason why it was cherrpicked instead of properly merged, resulting in loosing the commit message and author?

php-pulls pushed a commit that referenced this pull request Sep 27, 2012
* 'PHP-5.4' of git.php.net:php-src: (367 commits)
  fix unix/win dir separators
  Fix bug #63173: Crash when invoking invalid array callback
  Correct the test summary
  Fixed bug #60723 (error_log error time has changed to UTC ignoring default timezo)
  Fixed bug #60723 (error_log error time has changed to UTC ignoring default timezo)
  Avoid calling select if maxfd returned by curl_multi_fdset is -1
  Fixing NEWS file
  Fixed bug #63111 (is_callable() lies for abstract static method)
  updated lib versions
  Fix folding
  fix bug #63015 (Incorrect arginfo for DOMErrorHandler)
  Bug #63000: MCAST_JOIN_GROUP on OSX is broken
  Fixed bug #61442 (exception threw in __autoload can not be catched)
  Merging PR #116
  Merged GitHub PR #190: Support for the HTTP PATCH method in CLI webserver
  updated libary versions
  split tests for the new zlib version on win
  Fixed Bug #63103 (ext\curl\tests\bug62839.phpt broken)
  update news
  Support building PHP with the native client toolchain.
  ...
@lstrojny

Copy link
Copy Markdown
Contributor

@dsp Otherwise I can’t commit the NEWS file entry with it.

@dsp

dsp commented Sep 29, 2012

Copy link
Copy Markdown
Member

@lstrojny Sure, pull the branch, add a commit on top of it, merge the branch.

php-pulls pushed a commit that referenced this pull request Oct 19, 2012
… PHP-5.4

* 'PHP-5.4' of https://git.php.net/repository/php-src:
  Merging PR #116
  Merged GitHub PR #190: Support for the HTTP PATCH method in CLI webserver
  updated libary versions
  split tests for the new zlib version on win
  Fixed Bug #63103 (ext\curl\tests\bug62839.phpt broken)
php-pulls pushed a commit that referenced this pull request Oct 19, 2012
* 'master' of https://git.php.net/repository/php-src:
  Merging PR #116
  Merged GitHub PR #190: Support for the HTTP PATCH method in CLI webserver
  updated libary versions
  split tests for the new zlib version on win
  Fixed Bug #63103 (ext\curl\tests\bug62839.phpt broken)
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.

6 participants