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

[3.0] Roadmap (2017) #146

Closed
18 tasks done
barryvdh opened this issue Mar 28, 2017 · 28 comments
Closed
18 tasks done

[3.0] Roadmap (2017) #146

barryvdh opened this issue Mar 28, 2017 · 28 comments

Comments

@barryvdh
Copy link
Member

barryvdh commented Mar 28, 2017

Main goals

omnipay/omnipay

  • Drop all gateways from composer
  • Require Guzzle adapter by default

omnipay/common

Tests

Gateways

@delatbabel
Copy link
Contributor

Can you give me a to-do list and I will try tackling the changes with one of the gateways that I'm familiar with (either Stripe or PIN or Fat Zebra I guess).

@barryvdh
Copy link
Member Author

barryvdh commented Apr 9, 2017

See the changes in thephpleague/omnipay-mollie#34 and comments in #137

@delatbabel
Copy link
Contributor

OK but what I was hoping for was a step-by-step guide that we can hand off to gateway plugin developers so that they can 3-ify their code. Rather than having to say to them "have a look at some code changes that someone else made and hopefully when you do something similar it will work".

Can we wiki that up somewhere?

@barryvdh
Copy link
Member Author

@barryvdh
Copy link
Member Author

Soo, did any get a chance to test the alpha? I used it on a gateway, so as far as I'm concerned, I'm gonna release a beta.

@dpfaffenbauer
Copy link

@barryvdh I tested the alpha and migrated worldpay to it (thephpleague/omnipay-worldpay#29, still waiting to get merged). I am also already migrated Payum OmnipayBridge (Payum/Payum#675), but still waiting for this to get merged (thephpleague/omnipay-dummy#6).

But: everything is working fine on my local dev-environment.

When are you going to release a beta?

Good job 👍

@barryvdh
Copy link
Member Author

@dpfaffenbauer Did you encounter any issues or missed something from how to upgrade?

@dpfaffenbauer
Copy link

In fact, I found your upgrade guide when I was already finished :D

It was kinda clear for me on how to change worldpay to make it work. If you take a look at my PR, it's actually very easy to migrate: thephpleague/omnipay-worldpay@87a2d03

@barryvdh
Copy link
Member Author

Yeah I tried to keep it minimal :)

I want to release it soonish, only thing I'm not certain about is the HttpClient stuff. Want to get this right and there is a PSR coming for Http Clients (currently in draft), so kind of inclined to wait for that to be accepted.
See https://groups.google.com/forum/#!topic/php-fig/iFZF6T9L2zA

@judgej
Copy link
Member

judgej commented Aug 26, 2017

I've done a conversion for Sage Pay in this branch: https://github.com/thephpleague/omnipay-sagepay/tree/30alpha1

A few tests I've run to the test API instance seem to work fine. I started this ages ago, and this thread has given me the kick needed to finish it off. I'm not super clear on the dependencies. I assume those will need to be updated as the core 3.0 goes to beta and then production?

@barryvdh
Copy link
Member Author

You should be able to set the dependencies to 3.0 already, but lower your minimum stability to test it (to dev or alpha etc)

@judgej
Copy link
Member

judgej commented Aug 31, 2017

Thanks @barryvdh I really need to read up on the way composer does its matching of version numbers, which seems to include some implied regex stuff that I've never got my head around.

@judgej
Copy link
Member

judgej commented Oct 16, 2017

@barryvdh Any thoughts on the Price of an Item in the basket ItemBag for v3? At the moment it contains (I assume!) the line total, and is a float, presumably in major currency units and fractions thereof. In order to pass the price to another library that requires a money/money object, I'm having to load up a money parser to parse it, and even then it needs to be cast to a string for that to work. An example of that is here: https://github.com/academe/Omnipay-AuthorizeNetApi/blob/master/src/Message/AuthorizeRequest.php#L155

It would be nicer if the Item is already able to supply a money/money value for the price, sop the drivers don't need to make so many assumptions. It would also help in any calculations needed. For example, the Authorize.Net JSON API asks for the unit price, so in theory the line price needs to bne divided buy the Quantity before passing to the gateway.

I guess there are lots of additional properties and features the Item could do with supporting (tax, gross, net, item cost, line cost, discount, etc.) but just getting money/money into it for current fields would be a start. Is this something that should be in 3.0?

@barryvdh
Copy link
Member Author

FYI, I want to update it to PSR-18 when accepted, and then release soon: https://groups.google.com/forum/#!topic/php-fig/7PFR-KAAcMk

@judgej
Copy link
Member

judgej commented Jan 19, 2018

Should I be able to try out a 3.x adapter using composer to install? I've not managed to get it to work yet, and cannot work out why.

For example, I can clone https://github.com/academe/Omnipay-GiroCheckout master, which is 3.x, install all its dependencies and run some tests (both unit tests in the package, and some external tests to the gateway). That all works fine.

However, if I try a composer require academe/omnipay-girocheckout in any form, in a clean empty directory, I just cannot get it to install at all. Should I not be able to do that? Even a composer require omnipay/common does not work, and the errors make no sense to me. The 2.5 branch works fine composer require omnipay/common:2.5. Same with any 2.x adapters - they simply install. But anything 3.x dos not.

Is there a simple composer command that will install ANY 3.x Omnipay adapter, just so I can see it working without thinking that I can be going mad?

@judgej
Copy link
Member

judgej commented Jan 19, 2018

composer require "omnipay/common:v3.0-alpha.1"
...
Your requirements could not be resolved to an installable set of packages.

  Problem 1
    - Installation request for omnipay/common v3.0-alpha.1 -> satisfiable by omnipay/common[v3.0-alpha.1].
    - omnipay/common v3.0-alpha.1 requires php-http/client-implementation ^1 -> no matching package found.

What is this php-http/client-implementation? Does a HTTP client need to be installed before Omnipay 3.x? Installing the dependencies from a clone of the adapter works, but installing the dev-master through composer without cloning first does not. So far as I can see, the dependency tree should be equivalent.

-bash-4.1$ composer require php-http/client-implementation


  [InvalidArgumentException]
  Could not find package php-http/client-implementation.

  Did you mean this?
      php-http/client-implementation

Has a dependency perhaps simply gone from packagist?

@judgej
Copy link
Member

judgej commented Jan 19, 2018

Okay, it looks like the HTTP client does need to be installed first. This works:

composer require php-http/guzzle6-adapter
composer require "academe/omnipay-girocheckout:~3.0"

Without installing the guzzle6 adapter (or an equivalent) first, the girocheckout fails to install. Some magical group of necessary packages masquerade as a "virtual" package called php-http/client-implementation. Being a virtual package, it is not a real package. You need to install an equivalent package.

Sorry if you already know this stuff. I didn't, so I'm sure others won't either. I think installing from the clone just happened to work because the clone was installing omnipay tests, and the tests install php-http/guzzle6-adapter.

@judgej
Copy link
Member

judgej commented Jan 19, 2018

Sorry for spamming - I'm going through a learning curve too loudly. Some points and questions though, after sleeping on it:

  • Omnipay 3.x does not install any particular PSR-7 client by default. That's fine IMO, given that it is documented.
  • The requirement for a PSR-7 client (the implementation) must be very clearly documented. How do we find them? Is there a composer command that will list all the implementations that can be used?
  • The checklist says "Require Guzzle adapter by default", which is implying a preferred client will be installed by default. Would it be better not to do this?
  • The dev stability does require the guzzle implementation. That is probably okay if it is needed for testing (travis needs some implementation installed, and this supplies it).
  • I have seen stated somewhere else that HttPlug will be used as the default. Again, should we really do this, assuming I've read and understood correctly?

On the whole though, it's looking good - great work :-)

@barryvdh
Copy link
Member Author

Yeah that part is a bit tricky and not sure what the best tradeoff between flexibility and ease-of-use is. The dev version is required for testing indeed, because guzzle adds some useful features there.

omnipay/omnipay DOES add a default client (see https://github.com/thephpleague/omnipay/blob/master/composer.json), just not omnipay/common. So libraries etc can just require common, but end-users will get a default client installed (Guzzle), but no gateway should depend on Guzzle ever.

I want to use the PSR version instead of Httplug for the interface: https://github.com/php-http/fig-standards/blob/master/proposed/http-client/http-client.md but that's not accepted still..

@judgej
Copy link
Member

judgej commented Jan 19, 2018

omnipay/omnipay DOES add a default client

Ah, that's what I read, and probably what I should have installed when testing standalone locally. Makes sense now, thanks. I'll think about what I can add to the docs to explain it.

@digilist
Copy link

Is there any plan to release 3.0 until a specific date? I'd like to use omnipay in a current Symfony version, which is currently unfortunately not possible because of Guzzle.

@barryvdh
Copy link
Member Author

I'd like to wait for PSR-18 (HttpClient), but that isn't moving along so fast as I hoped..
https://groups.google.com/forum/#!topic/php-fig/7PFR-KAAcMk

@barryvdh
Copy link
Member Author

I've bumped PHP to 7.1 because that is the last supported version (see http://php.net/supported-versions.php )
I've tagged https://github.com/thephpleague/omnipay-common/releases/tag/v3.0-alpha.2
Gateways testing should probably use a fixed version of omnipay-common.

@barryvdh
Copy link
Member Author

FYI, I've updated the HttpClient, so any implementations using the dev versions should take note: 46daacf

Best to require a fixed alpha tag.

@barryvdh
Copy link
Member Author

So the first alpha release of the PSR HttpClient is released, so I've included that:
https://github.com/php-fig/http-client/releases

As far as I'm concerned, the omnipay-common API shouldn't have to change, we only need to change the underlying HttpClient when it's updated. But the exceptions and usage is the same (unless the spec drastically changes).

So if there are no objections, we can tag a beta release and start testing some upgrades.

@delatbabel
Copy link
Contributor

Still unwell, still only checking into github periodically but l;et me know if there's anything you need me to do especially on the PayPal or other gateways side.

@barryvdh
Copy link
Member Author

So quite a few gateways have been upgraded, according to https://github.com/thephpleague/omnipay#payment-gateways

I've removed the dependency on the PSR http-client package, but mimic it's behavior. I think we can just use that. Did any of you encounter any problems? If not, I think we're good to go for stable.

@barryvdh
Copy link
Member Author

It's tagged. Not all gateways yet, but upgrade guide is here: https://omnipay.thephpleague.com/changelog/

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

5 participants