Conversation
added 14 commits
March 1, 2018 16:22
chadicus
requested changes
Mar 2, 2018
| @@ -15,23 +15,19 @@ util-http-php requires PHP 5.4 (or later). | |||
|
|
|||
README.md
Outdated
| ```sh | ||
| composer require traderinteractive/util-http | ||
| ``` | ||
| ##Documentation |
Contributor
There was a problem hiding this comment.
Can you fix these headers? Add a space between the ## and the title
| * [Issues](https://github.com/traderinteractive/util-http-php/issues) | ||
|
|
||
| ##Project Build | ||
| With a checkout of the code get [Composer](http://getcomposer.org) in your PATH and run: |
Contributor
There was a problem hiding this comment.
you removed the build.php so the comment below is now invalid
src/HttpException.php
Outdated
| ) { | ||
| if (!is_string($message)) { | ||
| throw new \InvalidArgumentException('$message was not a string'); | ||
| throw new InvalidArgumentException('$message was not a string'); |
Contributor
There was a problem hiding this comment.
Since you're using type hints, these exceptions will never be thrown
src/Util/Http.php
Outdated
| { | ||
| if (!is_string($rawHeaders) || trim($rawHeaders) === '') { | ||
| throw new \InvalidArgumentException('$rawHeaders was not a string'); | ||
| throw new InvalidArgumentException('$rawHeaders was not a string'); |
Contributor
There was a problem hiding this comment.
with type hints, the is_string check is not needed.
src/Util/Http.php
Outdated
| return $headers; | ||
| } | ||
|
|
||
| private static function setRequest($match, array $headers) : array |
src/Util/Http.php
Outdated
| return $headers; | ||
| } | ||
|
|
||
| private static function setResponse($match, array $headers) : array |
added 2 commits
March 5, 2018 11:25
chadicus
requested changes
Mar 5, 2018
src/HttpException.php
Outdated
| * @throws \InvalidArgumentException if $httpStatusCode is not an int | ||
| * @throws \InvalidArgumentException if $code is not an int | ||
| * @throws \InvalidArgumentException if $userMmessage is not null and is not a string | ||
| * @throws InvalidArgumentException if $message is not a string |
Contributor
There was a problem hiding this comment.
Most of these are no longer thrown
src/HttpException.php
Outdated
| throw new \InvalidArgumentException('$code was not an int'); | ||
| } | ||
|
|
||
| if ($userMessage !== null && !is_string($userMessage)) { |
Contributor
There was a problem hiding this comment.
this will never be true, b/c of type hinting
src/Util/Http.php
Outdated
| return $headers; | ||
| } | ||
|
|
||
| private static function setRequest(array $match, array $headers) : array |
Contributor
There was a problem hiding this comment.
consider a better name for both setRequest and setResponse.
src/Util/Http.php
Outdated
| public static function getQueryParamsCollapsed($url, array $expectedArrayParams = []) | ||
| public static function getQueryParamsCollapsed(string $url, array $expectedArrayParams = []) : array | ||
| { | ||
| if (!is_string($url)) { |
Contributor
There was a problem hiding this comment.
This will never be true due to type hinting
chadicus
requested changes
Mar 5, 2018
Contributor
chadicus
left a comment
chadicus
left a comment
There was a problem hiding this comment.
You still need to update the namespace of the library
composer.json
Outdated
| "squizlabs/php_codesniffer": "^3.2" | ||
| }, | ||
| "autoload": { | ||
| "psr-4": { "DominionEnterprises\\": "src" } |
Contributor
There was a problem hiding this comment.
Need to update the namespace
src/HttpException.php
Outdated
| * Defines the \DominionEnterprises\HttpException class. | ||
| */ | ||
|
|
||
| namespace DominionEnterprises; |
src/Util/Http.php
Outdated
| * Defines the \DominionEnterprises\Util\Http class. | ||
| */ | ||
|
|
||
| namespace DominionEnterprises\Util; |
| public static function parseHeaders($rawHeaders) | ||
| public static function parseHeaders(string $rawHeaders) : array | ||
| { | ||
| if (!is_string($rawHeaders) || trim($rawHeaders) === '') { |
Contributor
There was a problem hiding this comment.
you should probably keep the trim($rawheaders) check
hotline-emu
force-pushed
the
imp/de-trader
branch
from
19f5aaf to
7c90640
Compare
chadicus
approved these changes
Mar 5, 2018
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
traderinteractiveTraderInteractive