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

Add support for PSR-18 HTTP clients #826

Draft
wants to merge 15 commits into
base: master
Choose a base branch
from
Draft

Add support for PSR-18 HTTP clients #826

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
9400f7d
Revert "Merge pull request #777 from Art4/add-psr18-http-client-support"
jtojnar Nov 6, 2023
802d33a
Create Psr7Response and Psr18Client
Art4 Jan 25, 2023
58d812a
Psr18Client follows redirects
Art4 Jan 26, 2023
1092570
Add tests with local files for FileClient
Art4 Jan 26, 2023
b0427c9
Add tests with local files for Psr18Client
Art4 Jan 26, 2023
44cdd6b
SimplePieTest: materialize provider
Art4 May 30, 2023
7d64ada
Set SimplePie->$file to private
Art4 Jan 30, 2023
eddf1a1
Add SimplePie::set_http_client(), add tests
Art4 Jan 27, 2023
55bb279
Add integration test for loading a feed from cache
Art4 Jan 27, 2023
67528cc
Add integration tests for PSR-16 cache and PSR-18 client
Art4 Jan 27, 2023
089fe8f
Add code examples in README.markdown for PSR-18 HTTP client and PSR-1…
Art4 Jan 27, 2023
1f2881a
Deprecate method SimplePie\SimplePie::set_file()
Art4 Mar 21, 2023
35788eb
SimplePie: Remove redundant phpdoc annotations
Art4 Jan 30, 2023
6a188f8
Add Sanitize::set_http_client()
Art4 Feb 16, 2023
0427439
Add Locator::set_http_client()
Art4 Feb 16, 2023
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
2 changes: 1 addition & 1 deletion CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

### Deprecated

- The method `SimplePie\SimplePie::set_file()` is deprecated, use `SimplePie\SimplePie::set_http_client()` or `SimplePie\SimplePie::set_raw_data()` instead
- The method `SimplePie\Sanitize::pass_file_data()` is deprecated, use `SimplePie\Sanitize::set_http_client()` instead
- Passing multiple URLs to `SimplePie\SimplePie::set_feed_url()` is deprecated. You can create separate `SimplePie` instance per feed and then use `SimplePie::merge_items()` to get a single list of items. ([#795](https://github.com/simplepie/simplepie/pull/795))
- The method `SimplePie\SimplePie::set_file()` is deprecated, use `SimplePie\SimplePie::set_http_client()` or `SimplePie\SimplePie::set_raw_data()` instead

## [1.8.0](https://github.com/simplepie/simplepie/compare/1.7.0...1.8.0) - 2023-01-20

Expand Down
2 changes: 1 addition & 1 deletion build/compile.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ function remove_header($contents)
$pos = strpos($contents, ';');
$namespace_name = substr($contents, 0, $pos);

$contents = $namespace_name . " {\n\n" . substr($contents, $pos + 1) . "\n\n}";
$contents = $namespace_name . " {\n\n" . substr($contents, $pos+1) . "\n\n}";
} else {
// use bracketed syntax for global namespace
$contents = "namespace {\n\n" . $contents . "\n\n}";
Expand Down
12 changes: 0 additions & 12 deletions phpstan.neon
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,18 +7,6 @@ parameters:
- tests/

ignoreErrors:
# Ignore that only one const exists atm
-
message: "#^Strict comparison using \\!\\=\\= between 'GET' and 'GET' will always evaluate to false\\.$#"
count: 1
path: src/HTTP/Psr18Client.php

# SimplePie\Content\Type\Sniffer::__construct(): Parameter $file could be mixed due to BC.
-
message: '(Result of \|\| is always false\.)'
count: 1
path: src/Content/Type/Sniffer.php

# Not used since https://github.com/simplepie/simplepie/commit/b2eb0134d53921e75f0fa70b1cf901ed82b988b1 but cannot be removed due to BC.
- '(Constructor of class SimplePie\\Enclosure has an unused parameter \$javascript\.)'

Expand Down
8 changes: 4 additions & 4 deletions src/Cache/DB.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,13 +42,13 @@ protected static function prepare_simplepie_object_for_cache(\SimplePie\SimplePi
}

if (isset($data->data['child'][\SimplePie\SimplePie::NAMESPACE_ATOM_10]['feed'][0])) {
$channel = & $data->data['child'][\SimplePie\SimplePie::NAMESPACE_ATOM_10]['feed'][0];
$channel =& $data->data['child'][\SimplePie\SimplePie::NAMESPACE_ATOM_10]['feed'][0];
} elseif (isset($data->data['child'][\SimplePie\SimplePie::NAMESPACE_ATOM_03]['feed'][0])) {
$channel = & $data->data['child'][\SimplePie\SimplePie::NAMESPACE_ATOM_03]['feed'][0];
$channel =& $data->data['child'][\SimplePie\SimplePie::NAMESPACE_ATOM_03]['feed'][0];
} elseif (isset($data->data['child'][\SimplePie\SimplePie::NAMESPACE_RDF]['RDF'][0])) {
$channel = & $data->data['child'][\SimplePie\SimplePie::NAMESPACE_RDF]['RDF'][0];
$channel =& $data->data['child'][\SimplePie\SimplePie::NAMESPACE_RDF]['RDF'][0];
} elseif (isset($data->data['child'][\SimplePie\SimplePie::NAMESPACE_RSS_20]['rss'][0]['child'][\SimplePie\SimplePie::NAMESPACE_RSS_20]['channel'][0])) {
$channel = & $data->data['child'][\SimplePie\SimplePie::NAMESPACE_RSS_20]['rss'][0]['child'][\SimplePie\SimplePie::NAMESPACE_RSS_20]['channel'][0];
$channel =& $data->data['child'][\SimplePie\SimplePie::NAMESPACE_RSS_20]['rss'][0]['child'][\SimplePie\SimplePie::NAMESPACE_RSS_20]['channel'][0];
} else {
$channel = null;
}
Expand Down
8 changes: 4 additions & 4 deletions src/Cache/MySQL.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -250,13 +250,13 @@ public function load()

if ($items !== 0) {
if (isset($data['child'][\SimplePie\SimplePie::NAMESPACE_ATOM_10]['feed'][0])) {
$feed = & $data['child'][\SimplePie\SimplePie::NAMESPACE_ATOM_10]['feed'][0];
$feed =& $data['child'][\SimplePie\SimplePie::NAMESPACE_ATOM_10]['feed'][0];
} elseif (isset($data['child'][\SimplePie\SimplePie::NAMESPACE_ATOM_03]['feed'][0])) {
$feed = & $data['child'][\SimplePie\SimplePie::NAMESPACE_ATOM_03]['feed'][0];
$feed =& $data['child'][\SimplePie\SimplePie::NAMESPACE_ATOM_03]['feed'][0];
} elseif (isset($data['child'][\SimplePie\SimplePie::NAMESPACE_RDF]['RDF'][0])) {
$feed = & $data['child'][\SimplePie\SimplePie::NAMESPACE_RDF]['RDF'][0];
$feed =& $data['child'][\SimplePie\SimplePie::NAMESPACE_RDF]['RDF'][0];
} elseif (isset($data['child'][\SimplePie\SimplePie::NAMESPACE_RSS_20]['rss'][0])) {
$feed = & $data['child'][\SimplePie\SimplePie::NAMESPACE_RSS_20]['rss'][0];
$feed =& $data['child'][\SimplePie\SimplePie::NAMESPACE_RSS_20]['rss'][0];
} else {
$feed = null;
}
Expand Down
17 changes: 3 additions & 14 deletions src/Content/Type/Sniffer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,7 @@

namespace SimplePie\Content\Type;

use InvalidArgumentException;
use SimplePie\File;
use SimplePie\HTTP\Response;

/**
* Content-type sniffing
Expand All @@ -27,26 +25,17 @@ class Sniffer
/**
* File object
*
* @var File|Response
* @var \SimplePie\File
*/
public $file;

/**
* Create an instance of the class with the input file
*
* @param File|Response $file Input file
* @param File $file Input file
*/
public function __construct(/* File */ $file)
public function __construct(File $file)
{
if (! is_object($file) || ! $file instanceof Response) {
// For BC we're asking for `File`, but internally we accept every `Response` implementation
throw new InvalidArgumentException(sprintf(
'%s(): Argument #1 ($file) must be of type %s',
__METHOD__,
File::class
), 1);
}

$this->file = $file;
}

Expand Down
12 changes: 6 additions & 6 deletions src/Enclosure.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -722,7 +722,7 @@ public function get_size()
{
$length = $this->get_length();
if ($length !== null) {
return round($length / 1048576, 2);
return round($length/1048576, 2);
}

return null;
Expand Down Expand Up @@ -809,7 +809,7 @@ public function get_width()
* @param array<string, mixed>|string $options See first parameter to {@see embed}
* @return string HTML string to output
*/
public function native_embed($options = '')
public function native_embed($options='')
{
return $this->embed($options, true);
}
Expand Down Expand Up @@ -940,9 +940,9 @@ public function embed($options = '', bool $native = false)
if ($height === 'auto') {
$width = 480;
} elseif ($widescreen) {
$width = round((intval($height) / 9) * 16);
$width = round((intval($height)/9)*16);
} else {
$width = round((intval($height) / 3) * 4);
$width = round((intval($height)/3)*4);
}
} else {
$width = '100%';
Expand All @@ -960,9 +960,9 @@ public function embed($options = '', bool $native = false)
$height = 360;
}
} elseif ($widescreen) {
$height = round((intval($width) / 16) * 9);
$height = round((intval($width)/16)*9);
} else {
$height = round((intval($width) / 4) * 3);
$height = round((intval($width)/4)*3);
}
} else {
$height = 376;
Expand Down
12 changes: 5 additions & 7 deletions src/File.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ class File implements Response
public $url;

/**
* @var ?string User agent to use in requests
* @var string User agent to use in requests
* @deprecated Set the user agent in constructor.
*/
public $useragent;
Expand Down Expand Up @@ -265,7 +265,7 @@ public function __construct(string $url, int $timeout = 10, int $redirects = 5,
}
} else {
$this->method = \SimplePie\SimplePie::FILE_SOURCE_LOCAL | \SimplePie\SimplePie::FILE_SOURCE_FILE_GET_CONTENTS;
if (empty($url) || ! is_readable($url) || false === $filebody = file_get_contents($url)) {
if (empty($url) || ! is_readable($url) || false === ($filebody = file_get_contents($url))) {
$this->body = '';
$this->error = sprintf('file "%s" is not readable', $url);
$this->success = false;
Expand Down Expand Up @@ -445,18 +445,16 @@ private function flatten_headers(array $headers): array
*/
final public static function fromResponse(Response $response): self
{
$headers = [];

foreach ($response->get_headers() as $name => $header) {
$headers[$name] = implode(', ', $header);
if ($response instanceof self) {
return $response;
}

/** @var File */
$file = (new \ReflectionClass(File::class))->newInstanceWithoutConstructor();

$file->url = $response->get_final_requested_uri();
$file->useragent = null;
$file->headers = $headers;
$file->set_headers($response->get_headers());
$file->body = $response->get_body_content();
$file->status_code = $response->get_status_code();
$file->permanent_url = $response->get_permanent_uri();
Expand Down
32 changes: 9 additions & 23 deletions src/HTTP/Psr18Client.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
use Psr\Http\Message\RequestFactoryInterface;
use Psr\Http\Message\UriFactoryInterface;
use SimplePie\Exception\HttpException;
use SimplePie\Misc;
use Throwable;

/**
Expand All @@ -22,24 +23,16 @@
*/
final class Psr18Client implements Client
{
/**
* @var ClientInterface
*/
/** @var ClientInterface */
private $httpClient;

/**
* @var RequestFactoryInterface
*/
/** @var RequestFactoryInterface */
private $requestFactory;

/**
* @var UriFactoryInterface
*/
/** @var UriFactoryInterface */
private $uriFactory;

/**
* @var int
*/
/** @var int */
private $allowedRedirects = 5;

public function __construct(ClientInterface $httpClient, RequestFactoryInterface $requestFactory, UriFactoryInterface $uriFactory)
Expand Down Expand Up @@ -69,7 +62,7 @@ public function getUriFactory(): UriFactoryInterface
*
* @param Client::METHOD_* $method
* @param string $url
* @param array<string,string|string[]> $headers
* @param string[] $headers
*
* @throws HttpException if anything goes wrong requesting the data
*/
Expand All @@ -83,16 +76,13 @@ public function request(string $method, string $url, array $headers = []): Respo
), 1);
}

if (preg_match('/^http(s)?:\/\//i', $url)) {
if (Misc::is_remote_uri($url)) {
return $this->requestUrl($method, $url, $headers);
}

return $this->requestLocalFile($url);
}

/**
* @param array<string,string|string[]> $headers
*/
private function requestUrl(string $method, string $url, array $headers): Response
{
$permanentUrl = $url;
Expand Down Expand Up @@ -120,7 +110,7 @@ private function requestUrl(string $method, string $url, array $headers): Respon
$statusCode = $response->getStatusCode();

// If we have a redirect
if (in_array($statusCode, [300, 301, 302, 303, 307]) && $response->hasHeader('Location')) {
if (in_array($statusCode, [300, 301, 302, 303, 307, 308]) && $response->hasHeader('Location')) {
// Prevent infinity redirect loops
if ($remainingRedirects <= 0) {
break;
Expand All @@ -131,7 +121,7 @@ private function requestUrl(string $method, string $url, array $headers): Respon

$requestedUrl = $response->getHeaderLine('Location');

if ($statusCode === 301) {
if ($statusCode === 301 || $statusCode === 308) {
$permanentUrl = $requestedUrl;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wonder if we can do anything about being passed a client that already handles redirects (like selfoss does).

We might not have other choice than adding support for each specific one. For example:

if (class_exists(\GuzzleHttp\RedirectMiddleware::class) && $response instanceof \GuzzleHttp\Psr7\Response && $response->hasHeader(\GuzzleHttp\RedirectMiddleware::HISTORY_HEADER)) {
    $permanentUrl = self::getGuzzleHistoryPermanentUrl($url, $response);
}


/**
 * Get permanent URL of the response.
 * RedirectMiddleware will need to be enabled for this to work.
 *
 * @param string $url requested URL, to use as a fallback
 * @param GuzzleHttp\Psr7\Response $response response to inspect
 *
 * @return string URL we should change the future requests to
 */
public static function getGuzzleHistoryPermanentUrl(string $url, \GuzzleHttp\Psr7\Response $response) {
    // Sequence of fetched URLs
    $urlStack = array_merge([$url], $response->getHeader(\GuzzleHttp\RedirectMiddleware::HISTORY_HEADER));
    // Let’s consider the first URL a permanent redirect since it is our start point.
    $statusStack = array_merge([301], $response->getHeader(\GuzzleHttp\RedirectMiddleware::STATUS_HISTORY_HEADER));

    // Follow the chain util first non-permanent redirect, since non-permanent
    // redirect could invalidate permanent ones later in the chain.
    $index = 0;
    while (($statusStack[$i] === 301 || $statusStack[$i] === 308) && $index < count($urlStack)) {
        $index++;
    }

    return $urlStack[$index - 1];
}

Copy link
Contributor

@Art4 Art4 Mar 20, 2023 •
edited

Choose a reason for hiding this comment

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

If a PSR-18 client handles the redirects they will return a PSR-7 Response with a 200 (or other then 301||308) status code. So we have no chance to get the final requested url. But this is not a problem because File will be deprecated and removed. I would not put any work into 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.

Yes, but we need to get the permanent URL for features like fossar/selfoss#1178.

Unless we want to deprecate SimplePie::$permanent_url and expose the Response in SimplePie so that apps could extract the permanent URL themselves.

Or we could instruct consumers to not pass clients with RedirectMiddleware in the docs but that is error-prone.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, we should deprecate also SimplePie::$permanent_url while deprecating File, and we should expose Psr\Http\Message\ResponseInterface instead. The extraction of the permanent URL would be a detail of the provided PSR-18 client. This has to be discussed in future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, looking at the Guzzle docs, their redirect middleware will apparently not be used when using Guzzle as PSR-18 client:

This option has no effect when making requests using GuzzleHttp\Client::sendRequest(). In order to stay compliant with PSR-18 any redirect response is returned as is.

That means the PSR-18 client will not be aware that the requests are connected when we handle redirects ourselves and there will be no way to extract the permanent URL from it.

}

Expand All @@ -144,10 +134,6 @@ private function requestUrl(string $method, string $url, array $headers): Respon

private function requestLocalFile(string $path): Response
{
if (! is_readable($path)) {
throw new HttpException(sprintf('file "%s" is not readable', $path));
}

try {
$raw = file_get_contents($path);
} catch (Throwable $th) {
Expand Down
32 changes: 15 additions & 17 deletions src/HTTP/Psr7Response.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,31 +19,24 @@
*/
final class Psr7Response implements Response
{
/**
* @var ResponseInterface
*/
/** @var ResponseInterface */
private $response;

/**
* @var string
*/
/** @var string */
private $permanent_url;

/**
* @var string
*/
private $requested_url;
/** @var string */
private $final_requested_url;

public function __construct(ResponseInterface $response, string $permanent_url, string $requested_url)
public function __construct(ResponseInterface $response, string $permanent_url, string $final_requested_url)
{
$this->response = $response;
$this->permanent_url = $permanent_url;
$this->requested_url = $requested_url;
$this->final_requested_url = $final_requested_url;
}

/**
* Return the string representation of the permanent URI of the requested resource
* (the first location after a prefix of (only) permanent redirects).
* Return the string representation as a URI reference.
*
* Depending on which components of the URI are present, the resulting
* string is either a full URI or relative reference according to RFC 3986,
Expand All @@ -63,14 +56,15 @@ public function __construct(ResponseInterface $response, string $permanent_url,
* - If a fragment is present, it MUST be prefixed by "#".
*
* @see http://tools.ietf.org/html/rfc3986#section-4.1
* @return string the original (first requested) URL before following redirects (expect 301)
*/
public function get_permanent_uri(): string
{
return $this->permanent_url;
}

/**
* Return the string representation of the final requested URL after following all redirects.
* Return the string representation as a URI reference.
*
* Depending on which components of the URI are present, the resulting
* string is either a full URI or relative reference according to RFC 3986,
Expand All @@ -90,10 +84,11 @@ public function get_permanent_uri(): string
* - If a fragment is present, it MUST be prefixed by "#".
*
* @see http://tools.ietf.org/html/rfc3986#section-4.1
* @return string the final requested url after following redirects
*/
public function get_final_requested_uri(): string
{
return $this->requested_url;
return $this->final_requested_url;
}

/**
Expand Down Expand Up @@ -127,6 +122,9 @@ public function get_status_code(): int
* }
* }
*
* While header names are not case-sensitive, get_headers() will preserve the
* exact case in which headers were originally specified.
*
* @return string[][] Returns an associative array of the message's headers.
* Each key MUST be a header name, and each value MUST be an array of
* strings for that header.
Expand Down Expand Up @@ -199,6 +197,6 @@ public function get_header_line(string $name): string
*/
public function get_body_content(): string
{
return $this->response->getBody()->__toString();
return (string) $this->response->getBody();
}
}