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

Update Live and Test URLs as per Opayo requirements #193

Merged
merged 1 commit into from
Nov 4, 2023
Merged

Update Live and Test URLs as per Opayo requirements #193

merged 1 commit into from
Nov 4, 2023

Conversation

jamieburchell
Copy link
Contributor

@jamieburchell jamieburchell commented Nov 3, 2023

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 judgej mentioned this pull request Nov 3, 2023
@@ -76,7 +76,7 @@ public function testAuthorizeSuccess()
$this->assertTrue($response->isRedirect());
$this->assertSame('{"SecurityKey":"IK776BWNHN","VPSTxId":"{1E7D9C70-DBE2-4726-88EA-D369810D801D}","VendorTxCode":"123"}', $response->getTransactionReference());
$this->assertSame('Server transaction registered successfully.', $response->getMessage());
$this->assertSame('https://test.sagepay.com/Simulator/VSPServerPaymentPage.asp?TransactionID={1E7D9C70-DBE2-4726-88EA-D369810D801D}', $response->getRedirectUrl());
$this->assertSame('hhttps://sandbox.opayo.eu.elavon.com/Simulator/VSPServerPaymentPage.asp?TransactionID={1E7D9C70-DBE2-4726-88EA-D369810D801D}', $response->getRedirectUrl());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This hhttps: typo - does it fail the tests, or have the tests not been run?

@judgej
Copy link
Member

judgej commented Nov 3, 2023

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
Copy link
Contributor Author

jamieburchell commented Nov 3, 2023

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
Copy link
Member

judgej commented Nov 3, 2023

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
Copy link
Contributor Author

jamieburchell commented Nov 3, 2023

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
Copy link
Member

judgej commented Nov 3, 2023

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
Copy link
Contributor Author

jamieburchell commented Nov 3, 2023

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

@jamieburchell
Copy link
Contributor Author

jamieburchell commented Nov 3, 2023

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 (#195) and here (#196)

@judgej
Copy link
Member

judgej commented Nov 3, 2023

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 judgej merged commit aaf8a90 into thephpleague:master Nov 4, 2023
@judgej
Copy link
Member

judgej commented Nov 4, 2023

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
Copy link
Contributor Author

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
Copy link
Member

judgej commented Nov 4, 2023

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 jamieburchell deleted the opayo-urls branch November 5, 2023 16:34
@jamieburchell
Copy link
Contributor Author

jamieburchell commented Nov 6, 2023

@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
Copy link
Member

judgej commented Nov 9, 2023

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
Copy link
Contributor Author

jamieburchell commented Nov 9, 2023

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
Copy link
Member

judgej commented Nov 17, 2023

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
Copy link
Contributor Author

jamieburchell commented Nov 17, 2023

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
Copy link
Member

judgej commented Nov 17, 2023

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
Copy link
Contributor Author

jamieburchell commented Nov 17, 2023

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.

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

Successfully merging this pull request may close these issues.

None yet

2 participants