Skip to content

Improved responses to different requests on static resources in built-in web server #8215

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

Closed
wants to merge 2 commits into from

Conversation

vedranmiletic
Copy link
Contributor

We use PHP's built-in web server for teaching HTTP requests and responses. These two patches handle some edge cases of interest to us.

  • Respond without body to HEAD request on a static resource
  • Respond with HTTP status 405 to DELETE/POST/PUT/PATCH request on a static resource

@vedranmiletic
Copy link
Contributor Author

I will fix those failing tests and get back with an improved version.

@vedranmiletic vedranmiletic force-pushed the master branch 2 times, most recently from 84c9371 to ba5daea Compare March 20, 2022 21:54
@ramsey
Copy link
Member

ramsey commented May 25, 2022

Hi, @vedranmiletic! This PR is waiting on some feedback from you before we can merge it.

As @cmb69 indicated:

The origin server MUST generate an Allow header field in a 405 response containing a list of the target resource's currently supported methods.

https://datatracker.ietf.org/doc/html/rfc7231#section-6.5.5

Does the CLI server already do this?

@github-actions
Copy link

There has not been any recent activity in this PR. It will automatically be closed in 7 days if no further action is taken.

@github-actions github-actions bot added the Stale label Jul 25, 2022
@github-actions github-actions bot closed this Aug 1, 2022
@bukka
Copy link
Member

bukka commented Aug 4, 2022

This might be worth to leave for a bit longer open

@bukka bukka reopened this Aug 4, 2022
@github-actions github-actions bot added SAPI: cli and removed Stale labels Aug 4, 2022
@vedranmiletic
Copy link
Contributor Author

Sorry for the delay. I will do the requested changes soon.

@vedranmiletic
Copy link
Contributor Author

Great CI results. Test incoming.

vedranmiletic and others added 2 commits August 7, 2022 15:29
Co-authored-by: Marin Martuslović <marin.martuslovic@student.uniri.hr>
…resource

Co-authored-by: Marin Martuslović <marin.martuslovic@student.uniri.hr>
@bukka
Copy link
Member

bukka commented Aug 18, 2022

Just FYI I messaged internals if this could be added before the first 8.2 RC.

@andypost
Copy link
Contributor

it could went in as bugfix to support 405 status

@bukka
Copy link
Member

bukka commented Aug 28, 2022

Merged via 7065a22 and 4f50905 . Thanks!

@bukka bukka closed this Aug 28, 2022
@bukka
Copy link
Member

bukka commented Aug 28, 2022

@andypost I think it's probably more for master even though it's probably somewhere between feature and bug.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants