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

Changing PHP_EOL to "\r\n" #2142

Closed
wants to merge 1 commit into from
Closed

Conversation

sheldonjuncker
Copy link
Contributor

The HTTP standard specifies that CR LF ("\r\n") should be used for separating elements in an HTTP request/response.
https://www.w3.org/Protocols/rfc2616/rfc2616-sec2.html#sec2.2

Using PHP_EOL could potentially break some applications as PHP_EOL is not guaranteed to be "\r\n".

The HTTP standard specifies that CR LF ("\r\n") should be used for separating elements in an HTTP request/response.
https://www.w3.org/Protocols/rfc2616/rfc2616-sec2.html#sec2.2

Using PHP_EOL could potentially break some applications.
@coveralls
Copy link

Coverage Status

Coverage increased (+0.001%) to 97.956% when pulling 23b8c90 on sheldonjuncker:3.x into c138554 on slimphp:3.x.

@akrabat
Copy link
Member

akrabat commented Feb 6, 2017

Looks good, but you also need to fix the tests

@sheldonjuncker
Copy link
Contributor Author

Yeah, the tests passed on my Windows system where PHP_EOL is "\r\n"! I've fixed the tests, so I guess I create a new pull request and close this one?

@akrabat
Copy link
Member

akrabat commented Feb 6, 2017

Just push to your branch and this PR will update

@sheldonjuncker
Copy link
Contributor Author

When I first made changes, I made them on my fork's master branch instead of the branch that I created specifically for this pull request. (I'm new to contributing to others' repos.)

I think it'll be easier if I just create a pull request for the correct branch.

@akrabat
Copy link
Member

akrabat commented Feb 6, 2017

@sheldonjuncker
Copy link
Contributor Author

Thanks for the help! I'll check it out.

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

Successfully merging this pull request may close these issues.

None yet

3 participants