phar: Support DELETE, HEAD and PUT HTTP methods in Phar::webPhar #2493

Closed
wants to merge 1 commit into
from

Conversation

4 participants
@cweiske
Contributor

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 Enhancement label May 1, 2017

@nikic

This comment has been minimized.

Show comment
Hide comment
@nikic

nikic May 1, 2017

Member

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

Member

nikic commented May 1, 2017

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

ext/phar/phar_object.c
+ || !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")

This comment has been minimized.

@weltling

weltling May 1, 2017

Contributor

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

weltling May 1, 2017

Contributor

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

This comment has been minimized.

Show comment
Hide comment
@weltling

weltling May 1, 2017

Contributor

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

Thanks.

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

This comment has been minimized.

Show comment
Hide comment
@cweiske

cweiske May 2, 2017

Contributor

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

Contributor

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

This comment has been minimized.

Show comment
Hide comment
@cweiske

cweiske May 2, 2017

Contributor

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

Contributor

cweiske commented May 2, 2017

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

@krakjoe

This comment has been minimized.

Show comment
Hide comment
@krakjoe

krakjoe May 2, 2017

Member

LGTM

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

Member

krakjoe commented May 2, 2017

LGTM

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

phar: Support DELETE, HEAD and PUT HTTP methods in Phar::webPhar
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

@cweiske

This comment has been minimized.

Show comment
Hide comment
@cweiske

cweiske May 2, 2017

Contributor

Based my patch on PHP-7.0.

Contributor

cweiske commented May 2, 2017

Based my patch on PHP-7.0.

@weltling

This comment has been minimized.

Show comment
Hide comment
@weltling

weltling May 2, 2017

Contributor

Merged with c0c0871.

Thanks.

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