Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

Conversation

nikcorg
Copy link
Contributor

@nikcorg nikcorg commented Sep 11, 2012

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
Contributor

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

@nikcorg
Copy link
Contributor Author

nikcorg commented Sep 12, 2012

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
Contributor

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

@ircmaxell
Copy link
Contributor

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

@lstrojny
Copy link
Contributor

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

@lstrojny
Copy link
Contributor

@nikcorg could you rebase and squash your commits?

@nikcorg
Copy link
Contributor Author

nikcorg commented Sep 14, 2012

@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
Copy link

stloyd commented Sep 14, 2012

@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 =)

@nikcorg
Added support for the HTTP PATCH method
cd97072 
Added support for the HTTP PATCH method

Added test for HTTP PATCH method support
@nikcorg
Copy link
Contributor Author

nikcorg commented Sep 14, 2012

@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
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
Copy link
Contributor Author

nikcorg commented Sep 14, 2012

@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
Contributor

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

@nikcorg
Copy link
Contributor Author

nikcorg commented Sep 17, 2012

Sure thing, no worries.

php-pulls pushed a commit that referenced this pull request Sep 18, 2012
@lstrojny
Merged GitHub PR #190: Support for the HTTP PATCH method in CLI webse…
aa133ea 
…rver
@php-pulls
Copy link

Comment on behalf of lstrojny at php.net:

Merged in 5.4 and master.

@php-pulls php-pulls closed this Sep 18, 2012
@dsp
Copy link
Member

dsp commented Sep 26, 2012

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
Merge branch 'PHP-5.4' of git.php.net:php-src into PHP-5.4
0bffdd7 
* '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
Contributor

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

@dsp
Copy link
Member

dsp commented Sep 29, 2012

@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
@cjbj
Merge branch 'PHP-5.4' of https://git.php.net/repository/php-src into…
4ec1a2a 
… 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
@cjbj
Merge branch 'master' of https://git.php.net/repository/php-src
2a352f6 
* '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)
@dunglas dunglas mentioned this pull request Sep 17, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@nikcorg @lstrojny @ircmaxell @stloyd @php-pulls @dsp