Fix SapiHeader::value when value contains a : character #441
Conversation
|
@Qard first of all thank you for contributing your changes! Could you add an integration test that covers this? The lint pipeline is failing because the commit message does not adhere to conventional commits https://www.conventionalcommits.org/en/v1.0.0/ |
|
I'm not yet sure exactly how to write an integration test for it. Do you have some pointers? There seems to be very little test coverage for the globals interfaces presently, as far as I can tell. 🤔 |
Pull Request Test Coverage Report for Build 15591754978Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
|
Have a look at the existing tests https://github.com/davidcole1340/ext-php-rs/tree/master/tests/src/integration here. If you have a specific question let me know. I am currently working on increasing coverage in general, so there is still a lot missing. Integration tests don't show up in coverage report though. EDIT: Misread that. Guess you wanted to know how to test the globals... Let me check |
|
Yeah, all I found for globals was https://github.com/davidcole1340/ext-php-rs/tree/master/tests/src/integration/globals which covers almost none of the API surface, so I'm a bit unsure what a test should actually look like, given there's not really any examples there. |
|
@Qard well, looking at it seems like we would need to expand the sapi tests by a decent amount. At this point a simple unit test should cover this. I added that. Could you check if this covers your case? |
|
Yep, that covers it. I'll see about fixing the commit messages for this now. Thanks for the help! I'll have some more PRs coming over the next couple days. I want to get as much as possible copied over from these commits so I don't need to be running a fork, and so others can benefit from the changes I've made. Much of what's in there will likely need a bit more polish, or at least proper tests and docs, so I'll get through them all one at a time. 🙂 |
When SapiHeader::value() is used on a header with a value containing a : character, it cuts off after that character. This change ensures the full header value can be retrieved.
Qard
force-pushed
the
fix-sapi-header-value
branch
from
d43c0b1 to
8ab2451
Compare
|
Much appreciated. Sapi needs some love :) You can always just delegate when you don't have time to polish some parts. Not that much time either, but always happy to help. |
I forked ext-php-rs here for use in php-node as I needed to make a bunch of changes. I want to backport as much of that as possible now so ideally we don't need to maintain a fork at all. Just starting small here with this small bug I fixed.
When a header value contains a : character, which is in fact valid according to the spec, it will fail in the current code as it will only include the text between the : separating the header name and the first : within the value. This fixes that by only splitting into two pieces.