Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

php_cli_server: ensure a single Date header is present #12363

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
coppolafab wants to merge 3 commits into php:PHP-8.1 from coppolafab:PHP-8.1

Conversation

Copy link
Contributor

@coppolafab coppolafab commented Oct 5, 2023

Currently the PHP Development Server appends a Date header in the response, despite already set from user code.

Steps to reproduce

index.php:
<?php header('Date: 1985年3月25日 00:20:00 GMT');

terminal 1:
$ php -S localhost:8080 index.php

terminal 2:

$ curl -i localhost:8080
HTTP/1.1 200 OK
Host: localhost:8080
Date: 2023年10月05日 07:45:57 GMT
Connection: close
X-Powered-By: PHP/8.2.10
Date: 1985年3月25日 00:20:00 GMT
Content-type: text/html; charset=UTF-8

I have added a check condition before append the header, and a test file.

Copy link
Member

@nielsdos nielsdos left a comment

Choose a reason for hiding this comment

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

Thanks for the patch @coppolafab . This mostly looks good, I just have some nits and a remark about the case sensitivity.

coppolafab reacted with laugh emoji
Copy link
Member

@nielsdos nielsdos left a comment

Choose a reason for hiding this comment

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

Looks almost perfect, just some remark

Copy link
Member

@nielsdos nielsdos left a comment
edited
Loading

Choose a reason for hiding this comment

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

LGTM, thanks!
I'll let the CI finish before merging, and I'll merge tomorrow or so.
EDIT: Windows CI failure is spurious

Copy link
Member

nielsdos commented Oct 6, 2023

Merged manually in 8.1 and above, thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nielsdos nielsdos nielsdos approved these changes

Assignees
No one assigned
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /