Update Live and Test URLs as per Opayo requirements

jamieburchell commented 10 months ago

From Opayo:

The URLs you use to send transactions to Opayo will be changing to an Elavon domain and you need to make the changes to send your transactions to the updated URLs.

These changes must be made by 31st March 2024, this is when the SagePay URLs will cease to work.

We've already added these updated URL's to our Developer Hub to help you make the changes, you can find the URLs specific to your integration below:

... Direct integration Form integration Server integration ...

judgej commented 10 months ago

Thank you, this will reduce the worry of a lot of people.

That typo in the tests inplies the tests aren't running on PRs, so I guess that needs looking at too.

jamieburchell commented 10 months ago

My bad, I didn't run phpunit. Running it flagged the typo, which I have fixed and merged with my previous commit.

However, for me phpunit is spewing lots of warnings and notices to the console and failing:

Warning - The configuration file did not pass validation!
The following problems have been detected:

Line 12:
- Element 'phpunit', attribute 'syntaxCheck': The attribute 'syntaxCheck' is not allowed.

Line 18:
- Element 'filter': This element is not expected.

PHP 8.1:

FAILURES!
Tests: 290, Assertions: 952, Failures: 2.

These seem to be errors relating to passing NULL to strtoupper/strtolower

PHP 8.2:

FAILURES!
Tests: 290, Assertions: 952, Failures: 5.

As above, plus dynamic class properties.

judgej commented 10 months ago

Those tests should have run automatically on commit, so that's something for me to look at.

The codebase is still largely from the PHP 7.4 days, so needs a good upgrade (I'd love to rewrite it all from scratch with a PHP 8.1 minimum TBH). The latest OmniPay core requires ^7.2|^8.0, which is positively ancient. There will be a few places where we can add some attributes to to suppress the PHP deprecation warnings and PHP 8.2 deprecation errors. That will buy some time, but I think a major version update is needed to move to a PHP8.1 minimum, but suspect there will still be some older sites that may need the older branch maintained. Not sure how many though.

jamieburchell commented 10 months ago

If you let me know how you'd prefer to handle the potential for NULLs I may be able to submit a PR

E.g.

strtolower((string) $this->httpRequest->request->get('VPSSignature')); or strtolower($this->httpRequest->request->get('VPSSignature') ?? ''); etc.

Regarding dynamic properties, we can opt-in to this (even at PHP 9) by adding #[\AllowDynamicProperties] to the affected class. Again, I can open a PR:

#[\AllowDynamicProperties]
abstract class TestCase extends PHPUnitTestCase
...

There is one further deprecation, usage of utf8_decode. I don't think I can fix that one without understanding why it's necessary.

judgej commented 10 months ago

I think any dynamic prpperties are an oversight, and probably need to be added explicitly. Might as well move the right direction.

I think the null coalescing operator approach feels right. The reason is that it is accepting either a string or a null, but no other data types to spring surprises. But that could be an argument for always casting to a string. I'm leaning towards ?? but it's your call.

jamieburchell commented 10 months ago

Dynamic properties are only affecting the TestCase class and extended classes it seems and this is from the omnipay repo itself

jamieburchell commented 10 months ago

I think the null coalescing operator approach feels right. The reason is that it is accepting either a string or a null, but no other data types to spring surprises. But that could be an argument for always casting to a string. I'm leaning towards ?? but it's your call.

PR submitted here (https://github.com/thephpleague/omnipay-sagepay/pull/195) and here (https://github.com/thephpleague/omnipay-sagepay/pull/196)

judgej commented 10 months ago

git push/pull seems to be down for me at the moment. I'll come back to this later, but looks good - thank you.

judgej commented 10 months ago

Hi Jamie, I've merged everything into master, with some additional fixes to remove a few other deprecation errors.

The tests run for me on PHP 8.0 and 8.2. I'm guessing if it works on those, then PHP 8.1 will work - but correct me if I'm wrong there.
Not tried PHP 7.4, but that should work. If anything, the utf8_decode replacement may be the thing that fails there? But not sure.
I've not tried it against test Sage Pay (Opayo now, I guess) yet.

Would you like to give dev-master a go, testing, Opayo test account, whatever you have?

jamieburchell commented 10 months ago

Hi Jamie, I've merged everything into master, with some additional fixes to remove a few other deprecation errors.

The tests run for me on PHP 8.0 and 8.2. I'm guessing if it works on those, then PHP 8.1 will work - but correct me if I'm wrong there.

Not tried PHP 7.4, but that should work. If anything, the utf8_decode replacement may be the thing that fails there? But not sure.

I've not tried it against test Sage Pay (Opayo now, I guess) yet.

Would you like to give dev-master a go, testing, Opayo test account, whatever you have?

Excellent. mb_convert_encoding has been around since PHP4 (who knew?!) so that should be a compatible replacement.

I can test against a sandbox/test account next week. If that works, I'll happily push it to a project using live credentials.

judgej commented 10 months ago

Excellent, if I get a chance to try it out on yhe test account soon I will. Really busy until Wednesday though, so maybe later in the week.

jamieburchell commented 10 months ago

@judgej I successfully put through a test transaction using the master branch code against a sandbox account. This was using the SagePay\Server gateway. Also tested an ABORT which worked.

judgej commented 10 months ago

Excellent, great work Jamie. I'll get to tagging this soon - will run it through some older apps I have running first, for beld and braces. I'm trying to keep legacy sites working on the current 4.x branch, then maybe got for a higher PHP version for 5.x while providing some support still for 4.x

Or maybe start again with an Opayo driver.

jamieburchell commented 10 months ago

I'm trying to keep legacy sites working on the current 4.x branch, then maybe got for a higher PHP version for 5.x while providing some support still for 4.x

Or maybe start again with an Opayo driver.

Apart from encouraging people to use newer versions of PHP, I don't think the current codebase requires PHP 8+?

judgej commented 10 months ago

Hey @jamieburchell do you know where I can find the docuemntation for the new URLs? Elavon seem to have broken all the old links and dumped a bunch of documentation (or hidden it really well). All the above links - for me at least - just redirect to the Elaovon developert home page. The API spec for Pi can be found there, but the OpenAPI spec still points to sagepay URLs.

jamieburchell commented 10 months ago

Hey @jamieburchell do you know where I can find the docuemntation for the new URLs? Elavon seem to have broken all the old links and dumped a bunch of documentation (or hidden it really well). All the above links - for me at least - just redirect to the Elaovon developert home page. The API spec for Pi can be found there, but the OpenAPI spec still points to sagepay URLs.

As you say, all of the links in the email we received informing us of the update (copied above) are redirecting to https://developer.elavon.com/. I can only assume that's a (embarrassing) mistake on their part, or they are intentionally pushing people to a new API.

Edit: Think I found them under "Legacy APIs" in the top menu under "API Docs"

https://developer.elavon.com/products/opayo-forms/v1/form-documentation https://developer.elavon.com/products/opayo-server/v1 https://developer.elavon.com/products/opayo-shared-api/v1 https://developer.elavon.com/products/opayo-direct/v1

So, they are "legacy APIs" now?!

judgej commented 10 months ago

Yeah, just spoke to the helpdesk and they pionted me there. They confirmed just change "sagepay" to "opayo" in the URLs and it should work. Also confirmed that the old pages that listed the URLs (and that they sent oiut thousands of emails for) has gone for now, but should be back in the near future. Thanks.

Just found test and live URLs for form: https://developer.elavon.com/products/opayo-forms/v1/test-and-live-urls-4633 and common https://developer.elavon.com/products/opayo-shared-api/v1/urls-4714 Those numbers on the end seem a little "temporary" so expect those URLs to die at some point.

The Pi API is under the new pages and not under legacy, and no URLs are mentions there at all.

But honestly, not putting redirects in when the documentation is being updated is the height of laziness.

jamieburchell commented 10 months ago

just change "sagepay" to "opayo" in the URLs and it should work

That's not quite true - "test" has become "sandbox" and the domain is now opayo.eu.elavon.com, but yeah.

the old pages that listed the URLs (and that they sent oiut thousands of emails for) has gone for now, but should be back in the near future.

Wow. That's pretty terrible isn't it.

But honestly, not putting redirects in when the documentation is being updated is the height of laziness.

Absolutely.

thephpleague / omnipay-sagepay

Update Live and Test URLs as per Opayo requirements #193