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

Replace HttpClient with PSR-18 HTTP Client #975

Merged
merged 15 commits into from
Apr 3, 2024
Merged

Replace HttpClient with PSR-18 HTTP Client #975

merged 15 commits into from
Apr 3, 2024

Conversation

pierredup
Copy link
Member

@pierredup pierredup commented Nov 18, 2023

Fixes #954

  • The HttpClient has been replaced with a PSR-18 HTTP Client
    • The httplug.message_factory config option is deprecated. Use payum.http_message_factory instead
    • The httplug.stream_factory config option is deprecated. Use payum.http_stream_factory instead
    • The httplug.client is deprecated. Use payum.http_client instead
    • The Payum\Core\Bridge\Httplug\HttplugClient::send method is deprecated and will be removed in 3.0. Use Payum\Core\Bridge\Httplug\HttplugClient::sendRequest instead
    • The Payum\Core\Bridge\Httplug\HttplugClient is deprecated and will be removed in 3.0. Use PSR-18 HTTP Client instead
    • The payum.http_client config option will return an instance of Psr\Http\Client\ClientInterface in a future version.
      • Change all type-hints from Payum\Core\HttpClientInterface or Payum\Core\Bridge\Httplug\HttplugClient to Psr\Http\Client\ClientInterface

@pierredup pierredup added this to the 2.0.0 milestone Nov 18, 2023
@pierredup pierredup self-assigned this Nov 18, 2023
@rimas-kudelis
Copy link

May I suggest that you change the prefix of relevant option names from httplug to something else, like payum? Prefixing them with httplug while expecting not necessarily an instance of one of httplug's interfaces seems wrong.

@pierredup
Copy link
Member Author

Actually I'm considering re-adding the interfaces and classes that wads removed, and keeping the current config names. I'm trying to minimise breaking changes for V2, so that all external plugins and gateways can work fine with both V1 and V2, and a lot of them uses the current httplug... config options. So I'm thinking about keeping it as-is, and rather deprecating them in 2.0

@rimas-kudelis
Copy link

Hmm, not sure I'm a fan of the idea. It's not often that you're releasing a new major version. IMO, cleaning up as much as possible while doing so would make sense, unless Payum were willing to commit to some soft of a predictable major release schedule (like Symfony and PHP itself).

@pierredup
Copy link
Member Author

There are hundreds of custom gateways available that might not all get updated to support V2, so releasing a new major version with no compatible gateways (except the built-in ones) and having slow adoption does not sound like a good strategy. I want people to be able to upgrade to Payum V2 and get access to some of the new features and cleaner code, without having to wait for their gateways to get updated, otherwise most people will just stay on V1

It's not often that you're releasing a new major version. IMO, cleaning up as much as possible while doing so would make sense

Just because a new major version is released, doesn't mean you need to take that chance and break as much as possible for everyone. A lot of clean up can still happen while still keeping existing functionality and gateways working.

It does not make sense for me to release a new major version that would hardly be usable in most applications without a lot of extra effort, or not being able to use any existing payment gateways because the author doesn't update it soon enough

@rimas-kudelis
Copy link

I get your point. I guess, as a fan of semver, I just have that habit of associating major releases with API breakage and not seeing it as a bad thing.

But if you intend to keep the old API intact, I just don't see much of a point to call this next release 2.0, I guess, because I see it more as an incremental change (1.x) than something big.

Of course, I don't know what else is changing and how, but assuming that your intention is to keep the work required to migrate at zero, it still seems like a minor release to me. :)

On the other hand, you yourself said in #954:

I think it makes sense to remove any HTTP client integration in Payum core, since I don't think it's needed or used anywhere inside core, and let each gateway do it's own HTTP client integration.

and I agree with this quote wholeheartedly. I think that not providing its own HTTP client could potentially make Payum a bit easier to work with and a bit cleaner code-wise. It would allow to drop some classes and interfaces entirely, and clean up at least one other place (CoreGatewayFactory), making it more maintainable.

@pierredup
Copy link
Member Author

as a fan of semver, I just have that habit of associating major releases with API breakage and not seeing it as a bad thing

According to Semver, when releasing a new major version, you CAN break BC, not that you MUST break BC.
Releasing a new major version without any breakages still conforms perfectly to semver.

But if you intend to keep the old API intact, I just don't see much of a point to call this next release 2.0, I guess, because I see it more as an incremental change (1.x) than something big.

There are a lot of changes planned for 2.0, so I want to release it as a new major version with a lot of overhaul and new features, which warrants a new major version. Nobody will be excited about a 1.8 release :)

On the other hand, you yourself said in #954:

And I still stand with this 100%, just not for the next release. The migration path to 2.0 should be as smooth as possible, then we can start working on more breaking changes for 3.0.

Anyway, let's move this discussion to https://github.com/orgs/Payum/discussions/977 and keep the discussion on this PR related to the changes.

@pierredup pierredup removed the BC Break label Apr 1, 2024
@pierredup
Copy link
Member Author

I have made the following changes to the PR:

  • Keep the httplug.* config options, but trigger a deprecation notice
  • Added payum.http_* options to replace the httplug options, which uses PSR-17
  • Kept the HttPlug class, but adjusted the deprecation and implemented Psr\Http\Client\ClientInterface.
  • Going forward for compatibility, all gateways should remove the httplug. usage and replace it with the corresponding payum.http_ options

These changes allows a gateway to be compatible with both Payum 1 and 2, but will trigger deprecations on Payum 2.

@pierredup pierredup merged commit c8b1161 into master Apr 3, 2024
14 checks passed
@pierredup pierredup deleted the php-http branch April 3, 2024 14:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

Update Guzzle deprecation to reference PSR-18 instead of Httplug
3 participants