Update API doc and deprecations #19
Conversation
theofidry
changed the title
theofidry
changed the title
| @@ -31,36 +41,68 @@ public function __construct() | |||
| $this->options = array_replace_recursive($this->options, $options); | |||
| } | |||
|
|
|||
| /** | |||
| * @param string $filename Name of the file to read. | |||
There was a problem hiding this comment.
should $filename be actually renamed to $url?
There was a problem hiding this comment.
It just reflects the docs: http://php.net/file_get_contents.
| public static function hasNextRequestHeaders() | ||
| { | ||
| return !empty(self::$nextRequestHeaders); | ||
| return null !== self::$nextRequestHeaders; |
There was a problem hiding this comment.
Actually probably not acceptable for a minor version as technically it's a BC break: empty request headers would have returned true before.
Tell me if the change of behaviour is wanted as IMO empty request headers still means there's headers, just no actual value, as opposed to no request headers set yet. If yes, then I'll update the code to add deprecations to reflect on that.
There was a problem hiding this comment.
I think we're fine if the original behaviour was incorrect.
| $headers | ||
| ); | ||
|
|
||
| if (empty($headers)) { |
There was a problem hiding this comment.
the changes are due to that line, i.e. doing an early return to avoid a 2 deep nested statements
| @@ -198,6 +266,8 @@ protected function getTlsStreamContextDefaults($cafile) | |||
| * @throws \RuntimeException If https proxy required and OpenSSL uninstalled | |||
| * | |||
| * @return resource Default context | |||
| * | |||
| * @final since 1.1.0 | |||
| */ | |||
| protected function getMergedStreamContext($url) | |||
There was a problem hiding this comment.
the function has changed since this has been pasted, see https://github.com/composer/composer/blob/master/src/Composer/Util/StreamContextFactory.php, so maybe we should update the code to reflect on that?
There was a problem hiding this comment.
Assuming there's no changes in its behaviour, but sure it's probably no harm to stay close to the original source.
There was a problem hiding this comment.
I really don't feel comfortable enough with that part to really assess if there's any problematic change that occurred right now so I'll leave it there for now. Might check again later
| return CaBundle::getSystemCaRootBundlePath(); | ||
| } | ||
|
|
||
| /** | ||
| * @deprecated | ||
| * @deprecated since 1.1.0 and will be removed in 2.0.0. | ||
| */ | ||
| protected static function validateCaFile($contents) |
There was a problem hiding this comment.
I would like a more helpful deprecation message here like the one just above, but I can't find any. There's actually no use of that method in our codebase so should this be exposed in a function or this shouldn't exist in the first place?
There was a problem hiding this comment.
I probably left it out of caution, but would remove if no longer used.
| @@ -31,36 +41,68 @@ public function __construct() | |||
| $this->options = array_replace_recursive($this->options, $options); | |||
| } | |||
|
|
|||
| /** | |||
| * @param string $filename Name of the file to read. | |||
There was a problem hiding this comment.
It just reflects the docs: http://php.net/file_get_contents.
| public static function hasNextRequestHeaders() | ||
| { | ||
| return !empty(self::$nextRequestHeaders); | ||
| return null !== self::$nextRequestHeaders; |
There was a problem hiding this comment.
I think we're fine if the original behaviour was incorrect.
| @@ -198,6 +266,8 @@ protected function getTlsStreamContextDefaults($cafile) | |||
| * @throws \RuntimeException If https proxy required and OpenSSL uninstalled | |||
| * | |||
| * @return resource Default context | |||
| * | |||
| * @final since 1.1.0 | |||
| */ | |||
| protected function getMergedStreamContext($url) | |||
There was a problem hiding this comment.
Assuming there's no changes in its behaviour, but sure it's probably no harm to stay close to the original source.
| return CaBundle::getSystemCaRootBundlePath(); | ||
| } | ||
|
|
||
| /** | ||
| * @deprecated | ||
| * @deprecated since 1.1.0 and will be removed in 2.0.0. | ||
| */ | ||
| protected static function validateCaFile($contents) |
There was a problem hiding this comment.
I probably left it out of caution, but would remove if no longer used.
|
Ready for a new review |
|
@Nyholm you might want to take a look |
Humbugnamespace