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

Added support for numeric header names #149

Merged
merged 2 commits into from
Apr 28, 2020
Merged

Added support for numeric header names #149

merged 2 commits into from
Apr 28, 2020

Conversation

nickdnk
Copy link
Contributor

@nickdnk nickdnk commented Apr 23, 2020

Hello

As discussed at Nyholm/psr7-server#35, here are some changes that seem to solve the problem.

This may be a premature PR, but it was the easiest way for me to demonstrate a cohesive solution with tests that works. @Zegnat did most of this, I just put it together.

Copy link
Collaborator

@Zegnat Zegnat left a comment

Choose a reason for hiding this comment

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

Thank you for opening this PR and getting the ball rolling! I have left some comments.

The change requests for the extra trailing commas in the tests are a bit unnecessary maybe. We do not by default run php-cs-fixer on tests. But I see no reason not to try and stay consistent.

src/MessageTrait.php Outdated Show resolved Hide resolved
src/MessageTrait.php Outdated Show resolved Hide resolved
tests/RequestTest.php Outdated Show resolved Hide resolved
tests/RequestTest.php Outdated Show resolved Hide resolved
@Nyholm
Copy link
Owner

Nyholm commented Apr 24, 2020

Thank you.

If this library (or the PSR specification) was PHP 7.2+. It would have a type hint for string on withAddedHeader. In fact, the PSR specify that the $header should be a string.

I dont think it is a good idea to convert an integer to a string. I think it is the callee (nyholm/psr7-server in this case) that should make sure it si giving a string to the withAddedHeader function.

@Nyholm
Copy link
Owner

Nyholm commented Apr 24, 2020

Hm.. I see. So if we do ->withAddedHeader('0', 'foo'), we will convert that to an int because it is used as an array key.. hm.

EDIT: It is true for all ints https://3v4l.org/b3DRV

Copy link
Owner

@Nyholm Nyholm left a comment

Choose a reason for hiding this comment

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

Im happy with this PR.

Sorry for my comment before. I had to read the full linked issue.

Please just add a comment.

src/MessageTrait.php Show resolved Hide resolved
@nickdnk
Copy link
Contributor Author

nickdnk commented Apr 24, 2020

Squashed.

@Nyholm Nyholm requested a review from Zegnat April 24, 2020 12:56
Copy link
Collaborator

@Zegnat Zegnat left a comment

Choose a reason for hiding this comment

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

Test passes and I couldn’t think of any more methods to test with it. So LGTM! 🚀

@nickdnk
Copy link
Contributor Author

nickdnk commented Apr 28, 2020

Hey guys

Can we merge this in and update the dev-dependencies of https://github.com/Nyholm/psr7-server/pull/36/files? The tests over there require this component to be updated as psr7-server uses it for tests which fail on that PR because of this.

@Nyholm
Copy link
Owner

Nyholm commented Apr 28, 2020

Yes, This should be merged. Thank you

@nickdnk
Copy link
Contributor Author

nickdnk commented Apr 28, 2020

I just added another test of getHeader() and getHeaderLine() with a value of 0, just to future-proof this edge-case.

@Nyholm Nyholm merged commit 3f71c19 into Nyholm:master Apr 28, 2020
@nickdnk
Copy link
Contributor Author

nickdnk commented May 11, 2020

Hey guys

Can we get this released so we can complete Nyholm/psr7-server#36? I cannot fix the errors I get in my API without this, unless I switch to a different PSR7 implementation which I would like to avoid having to toy with at the moment.

@Nyholm @Zegnat

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.

3 participants