Deprecated: preg_match() Passing null in getClientIp()

[jamieburchell]

I'm seeing this on PHP 8.1 using v4:

Deprecated: preg_match(): Passing null to parameter #2 ($subject) of type string is deprecated

Coming from here (omnipay/sagepay/src/Message/DirectAuthorizeRequest.php in Omnipay\SagePay\Message\DirectAuthorizeRequest::getClientIp at line 138)

I'm not sure why the IP can be null at that point, but here we are.

[judgej]

Thank you. It looks like parent::getClientIp() is returning null, which is understandable if running tests and the tests don't explicitly set the IP address.

It needs to skip the regex check if the IP address is already null, or is not a string.

[jamieburchell]

Strange thing is, this wasn't the result of running tests. This happened in production.

[judgej]

Just checked the core and I completely forgot that it is the application that supplies the IP address. I'm guessing your application isn't providing the client IP. A quick workaround would be to pass in anything that is not a valid IPv4 and not a null. An empty string would work.

$request->setClientIp('');

Just for some background, what the regex does, is to make sure the IP address is IPv4, and blanks it out if it isn't. Sage Pay would error (last I checked) if the IP was not in the a.b.c.d format. So it is not essential the IP address is sent (though for 3DS v2 maybe it is, but not at this point?)

The error is a PHP 8.1 deprecation warning being converted into an exception in your application (or maybe these are just beig displayed as warnings?). Another workaround would be to catch and log those, but not convert them into an exception (or not display the warnings). Laravel can do this with the deprecation log (very handy) and your framework may do something similar.

[jamieburchell]

Not sure if relevant, but I'm using the server integration and ClientIPAddress isn't a thing so I would have no need to set this. I think my PR still stands.

[judgej]

Yeah, PR looks good. It was PHP 8.1 that has brought this deprecation notice - what you are [not] setting is good too. I was just trying to think of a quick workaround that would fix the problem for you.

[judgej]

Also if clientIp is not even used in the server integration, then I think changes are needed so that it does not try to fetch it in the first place. Sage Pay Direct accepts the client IP address, since it does not directly see the client, The Server integration has a visit from the client browser, so it can see the client IP address for itself.

[jamieburchell]

I don't think I can assist with refactoring that. I believe a lot of the Server code extends the Direct code? There is a lot that's set in DirectAuthorizeRequest::getBaseAuthorizeData that isn't relevant for Server.

[judgej]

np - thanks again - I've merged and made a patch release.