Actually, thinking about it some more, it seems to me like HttpException is a misnomer and should be named something like ClientException. According to PSR-18:

A Client MUST NOT treat a well-formed HTTP request or HTTP response as an error condition. For example, response status codes in the 400 and 500 range MUST NOT cause an exception and MUST be returned to the Calling Library as normal.

And our Psr18Client does appear to preserve the semantics, only promoting ClientExceptionInterface implementations to HttpException.

Similarly, FileClient throws HttpException when success property is false but that only appears to be the case in errors other than HTTP error responses. In fact, File never seems to set status_code property when setting success = false, so get_status_code() there should always return the default 0 value.

Actually, we do enable CURLOPT_FAILONERROR in File so the success will be false on HTTP error in this case:

So that means Psr18Client is inconsistent with FileClient as the former will not return HttpException on HTTP error.

I'm guessing all the changes needed to improve the flows and make them consistent should be addressed in another PR, right?

My main fix was in the class SimplePie to get 429 error codes.

The change to FileClient was just an addition in case something sets that value.
If nothing sets the value, as you mentioned, it remains at 0, so it won't change the behavior of HttpException.

I have some fundamental issues with this PR. The HttpException is not made to represent a response, but to tell you that there was a problem making the request. If you have a response status code (even 500) the pure http request was successful, so no HttpException will be thrown. (Please note that the File/FileClient will be deprecated in favor of PSR-18.)

See PSR-18 error handling https://www.php-fig.org/psr/psr-18/#error-handling:

A Client MUST NOT treat a well-formed HTTP request or HTTP response as an error condition. For example, response status codes in the 400 and 500 range MUST NOT cause an exception and MUST be returned to the Calling Library as normal.

The request could fail for much more reasons as only for an HTTP request, e.g. reading a file from the local file system. One could not rely on the exception code to get a possible response status code. In opposite if you have a response status code the HTTP request was successful (but the response could be a 500 error).

Instead of moving the status code into the HttpException you can use the Psr18Client and get the status code from the response:

try {
    $response = $psr18Client->request(
        'GET',
        'http://example.com/feed.xml',
        ['Accept' => 'application/atom+xml']
    );
} catch (HttpException $e) {
    // Something went wrong making the HTTP request, reading the local file, etc
}

// $response is instance of \SimplePie\HTTP\Response
$statusCode = $response->get_status_code(); // Check for 500, 429, 200 or what else

Right. But currently, FileClient is not deprecated yet and it does throw HttpException for HTTP error response:

We probably cannot drop the CURLOPT_FAILONERROR due to BC so we will probably want FileClient to check if !$file->success && $file->get_status_code() === 0 and only throwing HttpException then.

We can do this right now. HttpException and FileClient only exists in the master branch so far. We can deprecate it or mark it as internal. (FileClient is marked as internal already for exact this reason. 🙈 ) And we should rename the HttpException to ClientException as you suggested, maybe as part of #826?

FileClient is marked as internal already

Probably the issue is if the users of SimplePie don't call set_http_client() the default behavior is to use FileClient.

The documentation for set_http_client() says

This will become required with SimplePie 2.0.0.

Is there any word on when this version is planned to be released?
Just wondering in case it's worth shipping this minor fix for FileClient

I've never totally understood PSR wording, so please bare with me 😅
The PSR-18 page says

Client - A Client is a library that implements this specification [...]

And

A Client is an object implementing ClientInterface.

But in the repo there's no class which implements this interface.
The name of the class Psr18Client makes me believe this already implements the PSR, but instead accepts a parameter ClientInterface $httpClient which should be the object actually implementing the specification.
Is this correct? Or am I missing something?

So, this will make every user of SimplePie migrating to 2.0.0 (when it comes out) require to implement their own way of making http requests instead of being already included out of the box.
Wouldn't this cause a lot of friction of adoption of the new version?
Or is this the intended flow?

Just wondering in case it's worth shipping this minor fix for FileClient

I think it is still worth it. It is hard to predict volunteer availability.

I've never totally understood PSR wording, so please bare with me 😅
The PSR-18 page says

Client - A Client is a library that implements this specification [...]

And

A Client is an object implementing ClientInterface.

I would call the first a PSR-18 Client library but I guess they decided to keep it short. Such shortening is fine in unambiguous cases, but ambiguity is often easy to miss, which leads to confusion, as you have experienced.

Also, since HTTP is a client-server protocol, even more confusingly, client can also refer to the abstract role in the HTTP protocol, or any implementation the role, such as curl.

The purpose of PSR-18 is to make the HTTP client implementations interchangeable. Without that, each library that wants to use HTTP would need to bring its own HTTP client implementation. As a result, an application that uses multiple such libraries (e.g. RSS reader can use SimplePie and Graby) would transitively depend on multiple, possibly slightly different HTTP libraries.

An example of a client library is https://github.com/guzzle/guzzle. You can see the full list at https://packagist.org/providers/psr/http-client-implementation.

But in the repo there's no class which implements this interface.

SimplePie is not a PSR-18 client. It supports using such library to allow consumers to select their preferred HTTP client implementation.

The name of the class Psr18Client makes me believe this already implements the PSR, but instead accepts a parameter ClientInterface $httpClient which should be the object actually implementing the specification.

Yes, the name is a bit confusing since language is always a bit ambiguous when we do not want overly long class names. The Psr18Client class is a HTTP client that is implemented as a proxy class for PSR-18 HTTP Client. The Psr18Client is parametric and must be provided by the consumer of SimplePie.

We introduced Psr18Client and FileClient as a temporary abstraction so that we can support PSR-18 in addition to the legacy File class. In the future, it is likely going away in favour of using ClientInterface directly.

So, this will make every user of SimplePie migrating to 2.0.0 (when it comes out) require to implement their own way of making http requests instead of being already included out of the box. Wouldn't this cause a lot of friction of adoption of the new version?

Yes, there is some upgrade cost but projects do not need to implement their own HTTP library, there are tons of ready made PSR-18 HTTP clients. And most modern projects will already be using one of them.

I created #909 to rename the misleading HttpException to ClientException and marked it as @internal because it should always catched internally. I also made sure that the response from FileClient and Psr18Client always returns the correct status code. The ClientException is only thrown if there was an error connecting the server or reading a local file. In both cases there is no response status code, so it defaults to 0.

SimplePie stores the status code from the response, so one can use $simplePie->status_code() to get the status code of the latest HTTP request. If $simplePie->status_code() returns 0 there was an error and one can use $simplePie->error() to get the error message why the HTTP request fails.

I don't see any reason for changes in this PR.

Just to be clear: I understand that the real problem of FreshRSS/FreshRSS#7038 is that in FreshRSS SimplePie logs 429 status code but $simplePie->status_code returns 0. I'm not sure why this happens and I cannot reproduce it in #909. So the problem must be somewhere else. Please investigate this problem to make sure you found the real reason so we can work on a solution.

As mentioned above, I believe the issue is that FileClient promotes HTTP errors to ClientError because File enables CURLOPT_FAILONERROR.

@jtojnar Thank you. 👍 I should have read the whole conversion again after such a long time. I will try to reproduce it again.

@pe1uca Thank you for pointing out this issue. Does #909 and using $simplepie->status_code() work for you? Can we close this PR?

Actually, we also need to expose the HTTP headers, for instance to access the Retry-After header after an HTTP 429 Too Many Requests.
Any suggestion for that?

Maybe #930