Skip to content

Conversation

@cweiske
Copy link
Contributor

@cweiske cweiske commented Apr 21, 2017

Up to now only GET and POST requests could be handled with Phar::webPhar(),
which is insufficient for today's REST APIs.
This patch expands the list of supported HTTP methods.

Resolves: https://bugs.php.net/bug.php?id=51918

@nikic nikic added the Feature label May 1, 2017
@nikic
Copy link
Member

nikic commented May 1, 2017

This seems quite reasonable. @weltling @krakjoe Any objections to merging this into 7.0?

|| !strcmp(SG(request_info).request_method, "GET")
|| !strcmp(SG(request_info).request_method, "HEAD")
|| !strcmp(SG(request_info).request_method, "POST")
|| !strcmp(SG(request_info).request_method, "PUT")
Copy link
Contributor

Choose a reason for hiding this comment

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

Any particular reason to change the condition order? GET and POST were probably the common case, so would profit from returning earlier from the condition anyway.

Thanks.

@weltling
Copy link
Contributor

weltling commented May 1, 2017

@nikic seems fine for 7.0 here as well. The condition order is not critical, might be just checked.

Thanks.

@cweiske
Copy link
Contributor Author

cweiske commented May 2, 2017

I ordered it alphabetically because it made sense to have it easy to read, but I'll change that to GET+POST first.

@cweiske
Copy link
Contributor Author

cweiske commented May 2, 2017

@weltling @nikic - changed the order, added PATCH and OPTIONS methods.

@krakjoe
Copy link
Member

krakjoe commented May 2, 2017

LGTM

@cweiske the target branch is wrong, target 7.0 please.

Up to now only GET and POST requests could be handled with Phar::webPhar(),
which is insufficient for today's REST APIs.
This patch expands the list of supported HTTP methods.
@cweiske cweiske changed the base branch from PHP-7.1.4 to PHP-7.0 May 2, 2017 10:25
@cweiske
Copy link
Contributor Author

cweiske commented May 2, 2017

Based my patch on PHP-7.0.

@weltling
Copy link
Contributor

weltling commented May 2, 2017

Merged with c0c0871.

Thanks.

@weltling weltling closed this May 2, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants