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

v4? #675

Open
pkly opened this issue Jun 2, 2022 · 5 comments
Open

v4? #675

pkly opened this issue Jun 2, 2022 · 5 comments

Comments

@pkly
Copy link

pkly commented Jun 2, 2022

Hello.

We've implemented omnipay as v3 about 2 years ago now, and it works fine for the most part.
I was wondering if you were considering a new major version which would require a rework?

Things to consider:

  • The base Omnipay http client implementation uses a custom http client interface It would be entirely possible to simply move to PSR-18 along with http client discovery, and http-message-factory interfaces, thus allowing any framework to simply plug in an implementation, and dropping guzzle as a requirement.

This is a breaking change, as the client requirements would change along with the exceptions thrown.
I would propose also no default client to be added, instead by going along with what composer wants us to do simply require an implementation.
This would allow omnipay to completely decouple from http requests.

  • The gateways are created statically and sit in a "main" namespace, with logic that will create objects in a rather strange way

This should likely be just removed, instead the gateways should implement a main interface and sit in separate namespaces.
A "shared" main namespace could still be considered for official gateways.

The methods for gateways could be split into interfaces instead of an annotation, thus having a proper requirement of implementation on the gateway and allow someone to simply do a $gateway instanceof PurchaseGatewayInterface

  • No more setters/getters into infinity that are a hard requirement

By requiring php 8.1 or simply moving some of the logic elsewhere it would be possible to stop using the set/get pattern along with likely dropping the requirement for ParameterBag and by thus I believe the symfony-framework?
If still needed it would be wise to maybe consider an alternative, or simply store parameters in an array.

  • There's little to any input validation

I'm currently on implementing another gateway for our project and I noticed that barely anything typechecks, as omnipay was designed to work with... php5?

  • Updating language features

At a time like this it seems a good idea to jump straight to php8.1 allowing a greater freedom of choice and improving development experience. The project could also use an upgrade of all libraries in use (omnipay/tests still uses phpunit 6...)

  • Updating quality tools, ensuring templates and so on

As code evolves it would be a good idea to use phpstan, psalm and so on as quality assurance tools.

@devnix
Copy link

devnix commented Aug 27, 2024

I agree with all the points!

I would add a couple of ideas:

@pkly
Copy link
Author

pkly commented Aug 27, 2024

At this point I've decided to create a custom solution which can benefit from proper php code instead of holding on to this legacy library. As omnipay is not exactly easy to integrate with something like symfony (and we ended up writing a ton of custom code and our custom gateways anyway) we're moving away from it. I'm not sure that creating an open source solution to match-all gateway needs makes sense, but if at some point later I decide it could be split into a library we might pull it out into an open source project as we've done with our previous 2 libraries.

@pkly pkly closed this as completed Aug 27, 2024
@devnix
Copy link

devnix commented Aug 30, 2024

I don't know how far I'll reach, but I'm going to start a little effort to propose a new major version: https://github.com/devnix-labs/omnipay-common/tree/add-static-analysis

I will consider your original ideas, but I would love to know more about your ideas and experiences two years later and after building a custom solution 😄

@barryvdh
Copy link
Member

Thanks, I'll look into this

@barryvdh barryvdh reopened this Aug 30, 2024
@pkly
Copy link
Author

pkly commented Aug 31, 2024

@devnix some suggestions then
everything as interface, generic gateway interface, but things like we hook/notification support should be optional interfaces instead of whatever it's right now with method annotations
use PSR for everything library wise, no more custom omnipay http client, assume a logger and PSR cache may be supplied (only setter method)
no more silly property requirements for requests to work
probably also only ever return requests for them to be sent explicitly by the implementation instead of omnipay in a hidden http client in request
require parameters explicitly for the gateway to work preferably in the individual implementation
no more stupid random factory and namespace requirement (it does literally nothing good) and I know it wasn't really a retirement but honestly it shouldn't even be there at all
there should be normalized exceptions for all generic gateway issues (not including http issues), for developers to simply implement
perhaps rework the line items as they were slightly cumbersome to use, make it easier to specify types of amounts (tax, product, total), get rid of the money library and replace it with something more modern, use integers for all numeric operations (only strings allowed should be in the api format itself)
add simple standardized phpunit abstracts you can use to bootstrap your tests (I'm not sure that was that easy last time I wrote a gateway)

all gateway management should be handled by the framework implementing it so the rest depends on how you handle that but you should have a setup simple enough to where it should be possible to simply hook in a setup gateway in a symfony application without maybe any but 1 additional class (something to contain the gateways)
a callback interface for handling certain actions like orders being rejected from notifications in gateways could be nice too

if you need more ideas hit me up

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

No branches or pull requests

3 participants