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.

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

Support for request method override #258

Closed
wants to merge 2 commits into from
Closed

Support for request method override #258

wants to merge 2 commits into from

Conversation

pfk84
Copy link
Contributor

@pfk84 pfk84 commented Apr 29, 2024

SAPI: Added support for overriding the request method via either the X-HTTP-Method-Override header or _method "magic" parameter. This is useful due to known shortcomings of PHP itself not passing parameters for other methods than POST.

Example use case, email address change process:

  1. Send out token to new email address via POST to /change-email
  2. Confirm change by passing token + code to via PUT to /change-email

Currently not possible as the parsed body is empty (ReactPHP mimicks PHP's behaviour in that case). This also allows to use e.g. the DELETE verb in HTML form requests: via <input type="hidden" name="_method" value="DELETE" />

SAPI: Added support for overriding the request method via either the X-HTTP-Method-Override header or _method "magic" parameter. This is useful due to known shortcomings of PHP not passing any parameters for other methods than POST.
@pfk84 pfk84 marked this pull request as ready for review April 29, 2024 13:55
Reduced override methods to PUT, PATCH & DELETE
@SimonFrings SimonFrings added the new feature New feature or request label May 7, 2024
@SimonFrings
Copy link
Contributor

Hey @pfk84, thanks for your contribution 👍

First of all, I really like the quality of your pull request, I can see a functionality like this coming in handy. I also talked with @clue about this feature and we think this might be better suited to use as a (global) middleware. Not everyone is in need for a feature like this (which might also lead to some unwanted behavior in some situations) and with middleware we don't force this behavior on every user (which is currently the case being part of the internal SapiHandler).

As a matter of fact, we're currently working towards refactoring the handler namespace, which will pave the way to introduce more handler and middleware (PSR-15 support) to the project. I can see this being something like a "MethodOverwriteMiddleware" in the future, but for now I think you should use a separate middleware.

With all that said, I'll close this ticket for now, but I still appreciate your thoughts and the amount of work that went into this! ❤️

@SimonFrings SimonFrings closed this May 7, 2024
@pfk84
Copy link
Contributor Author

pfk84 commented May 7, 2024

Thanks for having a look. I tend to agree but still think that the X-HTTP-Method-Override is a standard header that can be supported without fearing to add unwanted behaviour, at least as long it is done before the request reaches the route handlers.

I tried doing it as a global Middleware before submitting this PR but it would unfortunately not work - when are these changes your are talking about to be expected?

I really need this for my REST API project, asap... Btw. when using X with the built-in webserver PUT etc. with body/params just works fine but then again I can't use it that way due to the file upload limitations...

@pfk84
Copy link
Contributor Author

pfk84 commented May 7, 2024

Seems like my middleware works indeed; maybe I got confused because the log still shows 'POST' in that case...

@SimonFrings
Copy link
Contributor

SimonFrings commented May 7, 2024

Seems like my middleware works indeed [...]

@pfk84 Great to hear! Can you share a code snippet of your middleware in here in case others stumble upon the same topic looking for an answer for this? :)

@pfk84
Copy link
Contributor Author

pfk84 commented Sep 17, 2024

Current version, not heavily tested though...

https://gist.github.com/pfk84/2340d6e84a724ce244ffe8740bfee637

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants