Skip to content

Conversation

@GrahamCampbell
Copy link
Contributor

@GrahamCampbell GrahamCampbell commented Jul 19, 2020

Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
License MIT

PHPStan has detected 3 more bugs for us, and some unreachable code in the plugin client.

  1. Fixed bug in the cookie plugin
  2. Fixed null bug in when parsing date header
  3. Fixed typo in methods client phpdoc
  4. Removed unreachable plugin client code
  5. Don't temprarily violate property types in the flexible http client
  6. Added phpstan and some extra type information to help it, in some places

* @param callable(ClientInterface|HttpAsyncClient, Plugin[], array): PluginClient $factory
*/
public static function setFactory(callable $factory)
public static function setFactory(callable $factory): void
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not a BC break: the class is final.

$parts = explode('=', $part, 2);
$key = trim($parts[0]);
$value = isset($parts[1]) ? trim($parts[1]) : true;
$value = isset($parts[1]) ? trim($parts[1]) : null;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not a BC break: the Cookie class only accepts string or null as a value. This is a bug fix.

/**
* Add a client to the pool.
*
* @param ClientInterface|HttpAsyncClient|HttpClientPoolItem $client
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not a BC break. A HttpClientPoolItem is a ClientInterface. It's incorrect to specify it again.

Copy link
Contributor Author

@GrahamCampbell GrahamCampbell left a comment

Choose a reason for hiding this comment

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

I've left some comments to help reviewers. :)

@GrahamCampbell
Copy link
Contributor Author

It looks like that dead was code intentionally in there, even though it was not needed...

- src
ignoreErrors:
-
message: "#^Strict comparison using \\!\\=\\= between null and null will always evaluate to false\\.$#"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a false positive, and should be reported to phpstan as a bug.

path: src/HttpMethodsClient.php

-
message: "#^Method Http\\\\Client\\\\Common\\\\Plugin\\\\Journal\\:\\:addSuccess\\(\\) has no return typehint specified\\.$#"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a no-fix. Changing it would break BC. Maybe we should consider adding void return to the interface in the next major version.

path: src/Plugin/Journal.php

-
message: "#^Method Http\\\\Client\\\\Common\\\\Plugin\\\\Journal\\:\\:addFailure\\(\\) has no return typehint specified\\.$#"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a no-fix. Changing it would break BC. Maybe we should consider adding void return to the interface in the next major version.

Copy link
Contributor Author

@GrahamCampbell GrahamCampbell left a comment

Choose a reason for hiding this comment

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

Ready for review now @dbu. :)

Copy link
Contributor

@dbu dbu left a comment

Choose a reason for hiding this comment

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

awesome, thanks!

@dbu
Copy link
Contributor

dbu commented Jul 20, 2020

Removed unreachable plugin client code

are we missing some tests with that code?

@dbu dbu merged commit b102ff5 into php-http:master Jul 20, 2020
@GrahamCampbell
Copy link
Contributor Author

are we missing some tests with that code?

No, there are tests, and that's what made me put the code back.

@GrahamCampbell GrahamCampbell deleted the static-fixes branch July 20, 2020 07:50
@GrahamCampbell
Copy link
Contributor Author

Maybe unreachable was the wrong word. It's more that the code was duplicated, however that seems to have been intentional.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants