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.

Sign up for GitHub

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

Jump to bottom

Adding the HEAD command #33

Merged
merged 7 commits into from
Nov 20, 2017
Merged

Adding the HEAD command #33

Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
bf1f3cb
Add Head command interface
Nov 15, 2017
05b5a5b
Resolve merge conflicts
Nov 15, 2017
0d8fb34
Correct author
Nov 15, 2017
60b2205
Revert vendor autoload for github test environment
Nov 15, 2017
970bd2b
Lint the file I touched
Nov 15, 2017
842a4de
Refactor onHeadFollows
Nov 15, 2017
369465c
Refactor HeadCommand to return the string un-parsed
Nov 15, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 3 additions & 4 deletions src/Command/HeadCommand.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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!

Copy link

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link

Choose a reason for hiding this comment

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

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.

if (!empty($matches)) {
$headers[$matches[1]] = trim($matches[2]);
}
}, $response->getLines());

return $headers;
return $headers;
});
}

public function onNoNewsGroupCurrentSelected(Response $response)
Expand Down