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.

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

Add PSR-7 support #768

Closed
wants to merge 86 commits into from
Closed

Add PSR-7 support #768

wants to merge 86 commits into from

Conversation

Art4
Copy link

@Art4 Art4 commented Sep 8, 2022

This PR will add PSR-7 HTTP message support to Requests. This means that Requests will be able to process any PSR-7 Psr\Http\Message\RequestInterface implementation and response with a PSR-7 Psr\Http\Message\ResponseInterface implementation.

Pull Request Type

  • I have checked there is no other PR open for the same change.

This is a:

  • New feature

Context

@jrfnl stated in #320 (comment) that PSR-7 support would be a useful addition. Therefore I started with the implementation as a draft. Since I don't have any experience in contributing to Requests yet (I've read the .github/CONTRIBUTING.md), I appreciate early feedback to avoid gross errors early on.

Detailed Description

The plan is that the PSR-7 implementation will be built as a wrapper around the existing classes in the WpOrg\Requests\Psr namespace. This way there should be no breaking changes and the usage is optional. This is also important because PSR-7 has some limitations, such as missing multible requests.

Long term goal is as I mentioned here a PSR-18 implementation to be able to use Requests as a PSR-18 HTTP client. However, this will require PHP 7.0+ and is therefore not part of this PR. Nevertheless, this PR already includes a class that is compatible with PSR-18 and PSR-17 RequestFactory, but does not yet rely on the interfaces. This must be implemented in a future PR after bumping the PHP version to 7.0+.

Work progress

  • Implement request factory compatible to Psr\Http\Message\RequestFactoryInterface
  • Implement Psr\Http\Message\UriInterface
  • Implement Psr\Http\Message\RequestInterface
  • Implement Psr\Http\Message\StreamInterface
  • Implement stream factory compatible to Psr\Http\Message\StreamFactoryInterface
  • Implement Psr\Http\Message\ResponseInterface
  • Implement HTTP client compatible to Psr\Http\Client\ClientInterface

Quality assurance

  • This change does NOT contain a breaking change (fix or feature that would cause existing functionality to change).
  • I have commented my code, particularly in hard-to-understand areas.
  • I have added unit tests to accompany this PR.
  • The (new/existing) tests cover this PR 100%.
  • I have (manually) tested this code to the best of my abilities.
  • My code follows the style guidelines of this project.

Documentation

For new features:

  • I have added a code example showing how to use this feature to the examples directory.
  • I have added documentation about this feature to the docs directory.
    If the documentation is in a new markdown file, I have added a link to this new file to the Docs folder README.md file.

Make sure, provided Iri instance is immutable
@BrianHenryIE
Copy link

BrianHenryIE commented Sep 26, 2022

Regarding the PSR-18 functionality.

final class HttpClient/* implements \Psr\Http\Message\RequestFactoryInterface, \Psr\Http\Message\StreamFactoryInterface, \Psr\Http\Client\ClientInterface */ {

If the class wasn't final, in my plugins, with higher minimum PHP, I could just use:

class MyHttpClient extends HttpClient implements \Psr\Http\Message\RequestFactoryInterface, \Psr\Http\Message\StreamFactoryInterface, \Psr\Http\Client\ClientInterface { }

Is there a benefit to using a final class that I'm not realising?

I have a small concern/comment about the use of PSR in WordPress. I noticed with psr/container, between 1.0.0 and 1.1.0 functions' parameter types were added. This caused fatal errors where WooCommerce was using 1.0.0 and my plugin was using 1.1.0, since classes implementing ContainerInterface were no longer adhering to the interface. So from that experience, I suggest fixing the version to "psr/http-message": "1.0.1" rather than "psr/http-message": "^1.0.1".

Good work. I look forward to excluding Guzzle from my plugins.

@Art4
Copy link
Author

Art4 commented Sep 28, 2022

Thank you for your feedback.

If the class wasn't final, in my plugins, with higher minimum PHP, I could just use:

class MyHttpClient extends HttpClient implements \Psr\Http\Message\RequestFactoryInterface, \Psr\Http\Message\StreamFactoryInterface, \Psr\Http\Client\ClientInterface { }

Is there a benefit to using a final class that I'm not realising?

I've set every class as final by default because it gives us the ability to change the code in the future without thinking to much about breaking changes. Using final also encourages composition.

I can also recommend this blog post for more benefits of using final classes: https://ocramius.github.io/blog/when-to-declare-classes-final/

Btw your example will not work because HttpClient can't implement PSR-17 or PSR-18, because of the missing return type declaration functionality in PHP <7.0. If you want to use the HttpClient as PSR-18 client you should create a decorator instead.

<?php declare(strict_types=1);

final class MyHttpClient implements \Psr\Http\Client\ClientInterface
{
    private $client;

    public function __construct(\WpOrg\Requests\Psr\HttpClient $client)
    {
        $this->client = $client;
    }

    public function sendRequest(\Psr\Http\Message\RequestInterface $request): \Psr\Http\Message\ResponseInterface
    {
        try {
            return $this->client->sendRequest($request);
        } catch (\WpOrg\Requests\Exception\Psr\NetworkException $th) {
            throw new class($th) extends \Exception implements \Psr\Http\Client\NetworkExceptionInterface {
                private $request;

                public function __construct(\WpOrg\Requests\Exception\Psr\NetworkException $th)
                {
                    parent::__construct($th->getMessage(), $th->getCode(), $th);
                    $this->request = $th->getRequest();
                }

                public function getRequest(): \Psr\Http\Message\RequestInterface {
                    return $this->request;
                }
            };
        } catch (\WpOrg\Requests\Exception\Psr\RequestException $th) {
            throw new class($th) extends \Exception implements \Psr\Http\Client\RequestExceptionInterface {
                private $request;

                public function __construct(\WpOrg\Requests\Exception\Psr\RequestException $th)
                {
                    parent::__construct($th->getMessage(), $th->getCode(), $th);
                    $this->request = $th->getRequest();
                }

                public function getRequest(): \Psr\Http\Message\RequestInterface {
                    return $this->request;
                }
            };
        }
    }
}

$httpClient = new MyHttpClient(new \WpOrg\Requests\Psr\HttpClient($options));

Once Requests provides a fully PSR-18 implemented client, you can use it directly and delete your MyHttpClient class.

- $httpClient = new MyHttpClient(new \WpOrg\Requests\Psr\HttpClient($options));
+ $httpClient = new \WpOrg\Requests\Psr\HttpClient($options);

I have a small concern/comment about the use of PSR in WordPress. I noticed with psr/container, between 1.0.0 and 1.1.0 functions' parameter types were added. This caused fatal errors where WooCommerce was using 1.0.0 and my plugin was using 1.1.0, since classes implementing ContainerInterface were no longer adhering to the interface. So from that experience, I suggest fixing the version to "psr/http-message": "1.0.1" rather than "psr/http-message": "^1.0.1".

This library is usable independently of WordPress, so it is recommended to allow future compatible PSR-7 versions (>=1.0.1 to <2.0.0). How and which PSR-7 version exactly WordPress will ship with in the future is not subject of this PR.

Good work. I look forward to excluding Guzzle from my plugins.

Thank you very much. I am glad that my work is helpful.

@Art4 Art4 marked this pull request as ready for review October 5, 2022 15:06
@Art4
Copy link
Author

Art4 commented Oct 6, 2022

This PR is ready for review. Please note that PHPCS annotates 11 false positives like:

PHPCS: src/Psr/MessageHeaderTrait.php#L55
Method name "getHeaders" in class MessageHeaderTrait is not in snake case format, try "get_headers"

Because these method names are predefined by the interfaces, I cannot change them.

@Art4
Copy link
Author

Art4 commented Oct 6, 2022

I've also create a library from this PR to use Requests as PSR-18 client today.

If anyone is interested, go check it out: https://github.com/Art4/WP-Requests-PSR18-Adapter

@schlessera
Copy link
Member

Hi @Art4,

First of all, awesome work so far, and it's great to see you've added so many tests! 🤩

My apologies for not getting back to you sooner and leaving you hanging in uncertainty here!

@jrfnl and I have been discussing this PR for a while now to decide how to proceed from here. At this point, we've come to the conclusion that it would be preferable to have the PSR-7 and PSR-18 support in separate Composer packages that one can pull in as optional additions to Requests, for the following reasons:

  • @jrfnl and me don't have the bandwidth right now to either process this PR to make it ready to get merged into Requests, or to maintain the PSR-7 and/or PSR-18 code in the future (as we're not using those).
  • It would avoid adding new direct dependencies to Requests itself and only serve that code for projects that really need it.
  • It would decouple version requirements of PSR-7/PSR-18 support from those of Requests itself.
  • It would avoid version/compatibility issues with PSR versions for Requests, like changed signatures in PSR-7 v2.0, as the PSR-7 layer could just be v7.2 from the start.
  • With the above, this means that the PSR can be proper dependencies and the currently commented-out implements can be made active and enforceable.

To make sure you're not hitting any blocks along the way, @jrfnl and me are happy to talk about any additional extensibility mechanisms or changes that might be needed and will merge anything in that regard that makes sense and doesn't go counter to the Requests project philosophy or technical requirements. It looks as though this might not be needed, though, as you've already built this "wrapper" in a very self-contained way to begin with.

Now, as a home for these new packages there are two separate options:

A. Under your own name
Using your own name/vendor prefix and your own GitHub organization would be the simplest option, of course. This would be a package you're offering for anyone who has any interest in supporting PSR-7 together with Requests. You'd have full control over everything.

B. Under the Requests banner
We'd be able to give you all the access you need to host the package(s) under https://github.com/RequestsPHP/. We can discuss what the best namespace would be. We can make sure that most of the infrastructure (testing framework, GitHub Actions workflows, etc...) is flexible enough to support these new packages as well, and the Requests package would point to these as officially endorsed extensions.

Either way, we'd adapt the official documentation to point to this new package so people can find out how to add PSR-7/PSR-18 support.

Note: Under none of these would @jrfnl or me make any commitments to any specific maintenance work for this package. We're happy to support as we are able to and as our bandwidth allows, but we would be counting on you to own the maintainership of these extension packages.

@Art4
Copy link
Author

Art4 commented Aug 25, 2023

Hi @schlessera thank you for your feedback.

Given the fact that I have already created and released the adapter under my name as vendor prefix I think option A will work best for me. I will keep maintaining the library for whoever might need it.

https://github.com/Art4/WP-Requests-PSR18-Adapter

Either way, we'd adapt the official documentation to point to this new package so people can find out how to add PSR-7/PSR-18 support.

That would be great. Thank you.

@jrfnl
Copy link
Member

jrfnl commented Aug 25, 2023

@Art4 Glad to hear you're okay with this.

Would you like to submit a PR to update the docs/README to reference your repo ?

@Art4
Copy link
Author

Art4 commented Aug 31, 2023

I released v1.1.0 today

Would you like to submit a PR to update the docs/README to reference your repo ?

Not sure where to add this note. Would be great if you could do this. Thank you.

@jrfnl
Copy link
Member

jrfnl commented Sep 2, 2023

Would be great if you could do this.

@Art4 I would love to hear your opinion on #827

@Art4 Art4 deleted the add-psr7-support branch September 11, 2023 08:48
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.

4 participants