Adding the HEAD command #33
Conversation
src/Command/HeadCommand.php
Outdated
| public function onHeadFollows(MultiLineResponse $response) | ||
| { | ||
| $headers = []; | ||
| array_map(function ($line) use (&$headers) { |
There was a problem hiding this comment.
Could you please make an array_reduce of this?
return array_reduce($response->getLines(), function ($headers, $line) {
// if-statement here
return $headers
})
|
Thanks @elabz for your PR! Looks good to me 👍 Only a small comment on the code though. If you could change that + adding unit tests? |
src/Command/HeadCommand.php
Outdated
| @@ -52,15 +52,14 @@ public function __invoke() | |||
| */ | |||
| public function onHeadFollows(MultiLineResponse $response) | |||
| { | |||
| $headers = []; | |||
| array_map(function ($line) use (&$headers) { | |||
| return array_reduce($response->getLines(), function ($headers, $line) { | |||
| preg_match('/^([^\:]+)\:\s*(.*)$/', $line, $matches); | |||
There was a problem hiding this comment.
Unfortunately, we do seem to have a genuine problem with multi-line fields within multi-line responses. news.php.net may be quite a bit busier than most, but they seem to have spliced super-long lines with \n\t ins some cases and \n\s in others.
Have you seen a lot of those spliced super-long lines out there in the wild? Any ideas on how to address that issue (if it indeed needs to be addressed)?
Thanks!
There was a problem hiding this comment.
It may be safer to return the head as a string like the pear nntp class does and leave the name/value pair parsing to a wrapper class as not all users will need the data converted like this.
There was a problem hiding this comment.
Fair enough @thebandit , I will refactor. Will leave the parsing of the full header to the application. It will essentially become a twin of the body command
There was a problem hiding this comment.
Well see what @robinvdvleuten has to say as it's his project (I'm just an end-user like yourself). I was just offering a suggestion as it looks like this project tries to mimic the output of the pear class when possible.
|
@robinvdvleuten , I am just curious if the two failing tests (appveyor, styleci) were supposed to pass - the errors don't appear to be related to what I've added. If you see otherwise, please advise. |
|
@elabz they are indeed unrelated to your PR. The StyleCI fixes need to be applied by me and the AppVeyor is failing because of some weird Windows issue I need to look in to. |
|
@elabz thanks for the effort! I'll merge it in 👍 |
Hi Robin, sorry this PR is a bit rushed. Unit tests are not my strong point, tho I could add the test for HeadCommand in time. Perhaps you might want to take a look at the implementation before we test anyway. Let me know if you have comments / suggestions
Cheers,
D.