search

thephpleague / omnipay-sagepay

Sage Pay driver for the Omnipay PHP payment processing library
MIT License
55 stars 78 forks source link

Add PHP 8.0 support #169

Closed lumnn closed 2 years ago

lumnn commented 3 years ago

This is absolute minimum for PHP 8.0 support.

My work here is only to get composer run test work without any warnings and deprecation messages.

I did not run the code in any other way than through tests, so I'm not 100% confident about everything working correctly, but I have to leave now, so I may give it a bit more attention tomorrow ;)

This relates to issue #167

thtg88 commented 2 years ago

Hey all 👋 is there anything I can do to help this PR being merged? Would be great to be able to use this package with PHP 8 :)

barryvdh commented 2 years ago

Hmm was hoping to run the tests but not much happens.

thtg88 commented 2 years ago

Hmm was hoping to run the tests but not much happens.

I think it might have been because the branch wasn't on the main repo and the owner had not enabled GitHub Actions on it 🤔

Thanks for merging!

barryvdh commented 2 years ago

Hmm I can't run it on 7.2 because of prophecy supports 7.3 and up. Removed it in https://github.com/thephpleague/omnipay-sagepay/pull/171

Any ideas for that? Otherwise it isn't the biggest problem in the world, but still.. We could just bump the PHP version to 7.3 I guess.

thtg88 commented 2 years ago

Hmm I can't run it on 7.2 because of prophecy supports 7.3 and up. Removed it in #171

Any ideas for that? Otherwise it isn't the biggest problem in the world, but still.. We could just bump the PHP version to 7.3 I guess.

To solve the composer install requirements issue we could change (in composer.json) the Prophecy requirement to:

- "phpspec/prophecy-phpunit": "^2.0"
+ "phpspec/prophecy-phpunit": "^1.1|^2.0"

The problem is that the Prophecy API changed with 2.0 (as you'd expect with major versions), so the ProphecyTrait being used in tests is not available in 1.1. I think dropping PHP 7.2 might be the best option here 🤔

lumnn commented 2 years ago

Just to clarify on PHP version, I believe I put 7.2 because this is what was in omnipay/tests. If it comes to me, I don't much care about 7.2, because it's unsupported for a year, therefore I believe it's quite reasonable to drop 7.2 version, which anyway is supported by currently latest release of this package.

barryvdh commented 2 years ago

Agreed. Bumped to 7.3 in https://github.com/thephpleague/omnipay-sagepay/commit/9ddf4fa618a8d29f378528aaa3ab5a237b9fa5a1. Bumped the branch alias to 3.3 for testing so I suggest we release 3.3.0 sometimes with support for 7.3 and up. We can always still release a new 3.2.x version in case of issues.

barryvdh commented 2 years ago

@judgej do you think we should release the commits before this PR as a new 3.2 version? And then tag this as 3.3?

judgej commented 2 years ago

Yes, that seems the right thing to do. I have personally not had time to test these changes, but if people are using them in production, even if just their own branches, then go for it. I have some sites I need to uograde anyway, that will become very urgent soon, so will be available then for any tweaks or addiutional documentation.

So I understand, v3.2 would be the last PHP 7.2 version, then ~v7.3~ v3.3 would be PHP 7.3+?

thtg88 commented 2 years ago

So I understand, v3.2 would be the last PHP 7.2 version, then v7.3 would be PHP 7.3+?

(As i understand it) v3.3 would be PHP 7.3+

barryvdh commented 2 years ago

Yeah

thtg88 commented 2 years ago

I think I'll be able to start testing this (off of master) tomorrow or first thing next week. I'll let you know if I hit any issue with PHP 8.0 👍

thtg88 commented 2 years ago

I can confirm that at least the form methods work as expected with PHP 8.0 - is there anything I can do to help tag an official release? Or would you prefer to have some more testers e.g. for the other integration methods (server, direct)? :)

jamieburchell commented 2 years ago

Also volunteering if required to test against the Server integration

jamieburchell commented 2 years ago

@barryvdh @judgej Is there anything that we can do to get a new tagged release with PHP 8 support?

benjam-es commented 2 years ago

@jamieburchell if you're happy with the update, could you use the specific branch / commit reference within your composer.json to enable use now?

Obviously not ideal, but could be a temporary solution until they manage a release.

benjam-es commented 2 years ago

@jamieburchell also see this: https://github.com/thephpleague/omnipay-sagepay/pull/173 could be worth a test?

jamieburchell commented 2 years ago

@jamieburchell if you're happy with the update, could you use the specific branch / commit reference within your composer.json to enable use now?

Obviously not ideal, but could be a temporary solution until they manage a release.

@benjam-es Sure, but I have no idea how stable that is or how much compatibility there is with PHP 8.x and I'd rather not be pointing production applications at random commits.

I appreciate that maintainers have jobs and lives, but this repo hasn't had a release in nearly 3 years and I'm at the stage where I need to decide if I am going to fork this repo and take it on, or if there is something I can do to help this repo get a stable release.

I will be doing some testing against the master branch, but I'm not familiar enough with the project or its development to be confident that it's doing what it should be.

benjam-es commented 2 years ago

For now, to help out, I think if you and other users pointed a testing site to the master branch, and attempted to do some testing to ensure it works as you want it to, and hopefully do some testing to see if it works for protocol 4 given the latest merged pull request.

I believe the tests which are also run on CI, give the proof that it they work for php 8.0, so you would likely have to review all code change to ensure you are happy before pointing to a commit/branch.

In my case, I forked it, and I have my fork on a private satis so I use that instead until there are official releases.

jamieburchell commented 2 years ago

For now, to help out, I think if you and other users pointed a testing site to the master branch, and attempted to do some testing to ensure it works as you want it to, and hopefully do some testing to see if it works for protocol 4 given the latest merged pull request.

I pointed a PHP 7.4 testing site at master (protocol v4) and basic purchase transactions worked without issue. I also tested the abort from a client and from a timeout.