[2.x] Update deps #341
[2.x] Update deps #341
Conversation
j0k3r
force-pushed
the
fix/2.x-update-deps
branch
3 times, most recently
from
1188856
to
5e85278
Compare
j0k3r
force-pushed
the
fix/2.x-update-deps
branch
from
5e85278
to
de4e966
Compare
- update GitHub Actions to latest version - run PHP-CS-Fixer - use `Psr\Http\Client\ClientInterface` instead of `Http\Client\HttpClient` - use `Http\Discovery\Psr18ClientDiscovery` instead of `Http\Discovery\HttpClientDiscovery`
j0k3r
force-pushed
the
fix/2.x-update-deps
branch
from
cac6ecb
to
b1aff11
Compare
There was a problem hiding this comment.
On the removal of utf8_encode, did you check that it does not break wallabag/wallabag#2387?
|
@Kdecherf yep I've checked, nothing is broken |
| @@ -21,14 +21,14 @@ class ContentExtractor | |||
| private $xpath; | |||
| private $html; | |||
| private $config; | |||
| private $siteConfig = null; | |||
There was a problem hiding this comment.
I like explicitly initialized value for properties that are not immediately initialized in the constructor. IMO, no_null_property_initialization erases this information so I consider it harmful. See also PHP-CS-Fixer/PHP-CS-Fixer#7447 (comment)
But it is probably not an issue in the legacy branch.
| * @param array $config | ||
| */ | ||
| public function __construct(Client $client, $config = [], LoggerInterface $logger = null) | ||
| public function __construct(ClientInterface $client, $config = [], LoggerInterface $logger = null) |
There was a problem hiding this comment.
Client → ClientInterface should be fine. Client has been a subtype since 2.0 (php-http/httplug@6885b6c).
| @@ -118,7 +114,7 @@ public function __construct($config = [], Client $client = null, ConfigBuilder $ | |||
| ); | |||
|
|
|||
| $this->httpClient = new HttpClient( | |||
| $client ?: new PluginClient(HttpClientDiscovery::find(), [new CookiePlugin(new CookieJar())]), | |||
| $client ?: new PluginClient(Psr18ClientDiscovery::find(), [new CookiePlugin(new CookieJar())]), | |||
There was a problem hiding this comment.
Should be fine as well, just searches for the supertype now:
https://github.com/php-http/discovery/blob/664ded67cc61abcaa5b77d27581777483d1c843d/src/HttpClientDiscovery.php#L27
https://github.com/php-http/discovery/blob/664ded67cc61abcaa5b77d27581777483d1c843d/src/Psr18ClientDiscovery.php#L25
| @@ -150,8 +146,6 @@ public function reloadConfigFiles() | |||
| * Return a config. | |||
| * | |||
| * @param string $key | |||
| * | |||
| * @return mixed | |||
There was a problem hiding this comment.
IIRC explicit return type signals PHPStan that the return value is defined. But again, probably fine on legacy branch.
| @@ -181,7 +181,7 @@ public function dataForBuild(): array | |||
| /** | |||
| * @dataProvider dataForBuild | |||
| */ | |||
| public function testBuildSiteConfig(string $host, bool $expectedRes, ?string $matchedHost = null): void | |||
| public function testBuildSiteConfig(string $host, bool $expectedRes, string $matchedHost = null): void | |||
There was a problem hiding this comment.
Not really a fan of having different type annotation based on a default value. That way, I need to check two places to know what I can pass to an argument.
Even Python recognized it is a bad idea.
| @@ -163,7 +163,6 @@ public function dataWithAccent(): array | |||
| return [ | |||
| // ['http://pérotin.com/post/2015/08/31/Le-cadran-solaire-amoureux'], | |||
| ['https://en.wikipedia.org/wiki/Café'], | |||
| ['http://www.atterres.org/article/budget-2016-les-10-méprises-libérales-du-gouvernement'], | |||
There was a problem hiding this comment.
Maybe we could get it from Internet Archive https://web.archive.org/web/20210121073723/http://www.atterres.org/article/budget-2016-les-10-méprises-libérales-du-gouvernement
Psr\Http\Client\ClientInterfaceinstead ofHttp\Client\HttpClientHttp\Discovery\Psr18ClientDiscoveryinstead ofHttp\Discovery\HttpClientDiscoveryutf8_encodeused (because it's deprecated in PHP 8.2) and I don't think these lines are still used (the associated test was already removed)