Non-absolute URLs are allowed these days: https://httpwg.org/specs/rfc9110.html#field.location

Not sure what's the problem. This is part of the UriFactory and Request class.

The problem is that the location header could have a non-absolute url (without host). But this is not a problem.

$this->uriFactory->createUri() creates a new Uri instance from the non-absolute url. This uri is set to $request->withUri($uri). This will take care of the missing host:

This method MUST update the Host header of the returned request by
* default if the URI contains a host component. If the URI does not
* contain a host component, any pre-existing Host header MUST be carried
* over to the returned request.

See https://github.com/php-fig/http-message/blob/2.0/src/RequestInterface.php#L99-L129

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];
}

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.

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.

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.

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.

Not sure if we should match error messages like this. I do not think they are guaranteed to be stable.

I'm checking the error message to be sure that the test failed for the expected reason. As an alternative we could throw a new FileNotFoundException.

file_get_contents should not throw an exception by itself. I presume this is to catch it if app throws in the handler passed to set_error_handler? Perhaps add a comment?

file_get_contents() can generate Errors/Throwables. This is tested here: https://github.com/simplepie/simplepie/pull/777/files#diff-71f5ab2331097a56ec44e0ee126d69e4ef859f22a076b0f0055ebbd3a6303e86

AIUI, E_WARNING will not be converted into throwable unless the application defines a custom error handler with set_error_handler, that throws.

That tests catches the throw below:

if ($raw === false) {
    throw new HttpException('file_get_contents() could not read the file', 1);
}

If the user has a custom error handler we can catch the error and throw it as a HttpException. The tests expect the message from the PHP Error as file_get_contents(/path/to/file): Failed to open stream: No such file or directory.

If the user has no custom error handler, $raw will be false and we throw a HttpException with the message file_get_contents() could not read the file. (Saying this I'm realizing that we have no tests for this second case.)

I've added a is_readable() check in #848.

Either way, the error code will probably not be meaningful for HttpException.

Looks like SimplePie will no longer share input data. Perhaps we should have separate internal set_http_client method that will continue accepting it from SimplePie and a public one set_http_factories for consumers that want to use Sanitize or Locator outside the context of SimplePie?

Sanitize has already set_http_client() for PSR classes and the (deprecated) pass_file_data(). If one has not called set_http_client(), the http client will be created on the deprecated file data.

I have seen some libraries to use Psr18ClientDiscovery. While I dislike this on principle, it is certainly convenient.

I wonder if we should just use {@inheritdoc} here for ApiGen.

Weirdly, Symfony started removing it symfony/symfony#47390. Perhaps because they no longer seem to have generated API docs – quick search only found https://symfony.com/blog/new-symfony-api-documentation which returns 404.

I'm not a fan of {@inheritdoc} either.

In #815, we marked the last two as internal. Do we want to promote them to API? I think that would only make sense if we expect consumers to instantiate these classes directly, rather than only having SimplePie create them internally.

For 2.0, I would like to minimize the API surface marking everything final and private, and only opening methods that have good use cases. So we should probably decide if we want to allow consumers to create Locator and Sanitize instances, and make set_http_client() methods on them internal, so that we do not need to deprecate them again.

The last two was set internal because they accept an also internal SimplePie\HTTP\Client. #777 changes this to PSR-18 client and PSR-17 factories.

I agree with you that we should minimize the API surface but atm it is allowed to create Locator and Sanitize instances and we are referencing Sanitize::set_http_client() as an alternative for the deprecated Sanitize::pass_file_data() and maybe should do the same with Locator::set_http_client().

Alternative we could complete deprecate the instantiation of Locator and Sanitize for users. This could be made and discussed in a separate PR.

Deprecating the the instantiation of Locator and Sanitize for consumers in a separate PR sounds good to me.

See #849.

Even though this is no change for PHP variance checker, extending the type that the argument accepts is still a breaking change since subclasses might not expect it.

And there are apparently some 😿 https://github.com/search?q=%2Fextends%20SimplePie.%2BSniffer%2F&type=code

Edit: I am using File::fromResponse here as well.

Removing the type hint is not a breaking change because we have not released 1.9.0 yet. (refs)

Also the type hint in the docblock was wrong, see https://github.com/simplepie/simplepie/blob/1.8.0/src/Content/Type/Sniffer.php#L71-L79

Might be nice to move the instanceof check into the method. Edit: Done here.

This will need to be changed to __toString. Edit: Done.