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

Suppress the warning from simplexml_load_string() because we catch #132

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

robertology
Copy link
Contributor

We're handling the error, so suppress the warning.

We're handling the error, so suppress the warning.
@judgej
Copy link
Member

judgej commented Dec 26, 2019

Could you give an example of what problem this fixes please. It will be worth stating which PHP version you are using too, since there are some long-standing bugs in the XML handling that have been fixed in recent years.

@robertology
Copy link
Contributor Author

We're on PHP 7.2.22 but the same behavior is found through 7.4 as shown here
https://3v4l.org/UYbFH

Basically, simplexml_load_string results in an uncatchable warning when given an invalid string. So:

  1. Your catch statements are probably never hit
  2. But the if (!$xml) { after would catch the failure case

The problem is, PHP spits out warnings which has our system logging it as an error, but it's just noise because the error is actually handled. Suppressing the warning has the same end result (throw new InvalidResponseException()) but without the noise.

@barryvdh
Copy link
Member

barryvdh commented Jan 3, 2020

We throw the same error below when the result is false, so I guess this would be fine. But what @judgej wants to know (I think) is in which case this invalid XML occurs. Because maybe we should be fixing that instead.

@barryvdh
Copy link
Member

barryvdh commented Jan 3, 2020

Ideally we would add the example response to the Test Suite and make sure it works now (and didn't before), eg in https://github.com/thephpleague/omnipay-authorizenet/blob/master/tests/Message/AIMResponseTest.php

@judgej
Copy link
Member

judgej commented Jan 3, 2020

I was mainly wondering how to emulate it, and how much it depends on the environment. For example, warnings can be suppressed at the server level. They can also be caught in a framework (e.g. Laravel) and converted into exceptions.

From everything I have read, use of @ just suppresses the display of the warnings, but does not affect how the warnings are handled. So, assuming I understood that correctly, if the problem more in the framework or the server settings? It was just a case of digging a little deeper into what was happening here.

But as @barryvdh says, what is causing the XML error in the first place? Are we perhaps not catching a non-2xx response early enough? Maybe we aren't confirming the response MIME type indicates it is XML? Just a hunch when I see a @ that something else is going on that needs to be handled. If I can reproduce it, I'm happy to have a play.

@robertology
Copy link
Contributor Author

Here are some error messages we saw. They happened in a bunch, so I imagine there may have been some server maintenance or something returning a poorly formed html page.

Entity: line 8: parser error : Opening and ending tag mismatch: BODY line 3 and HTML

Entity: line 9: parser error : Premature end of data in tag HTML line 1

@judgej
Copy link
Member

judgej commented Jan 14, 2020

Thanks, that helps a lot.

It looks like HTML is being returned by the gateway here, for whatever reason, instead of XML. The content type header should tell us this is happening before we even look at the payload. Rather than trying to squeeze a non-XML payload into an XML parser, we can check the content type and error earlier, with a more appropriate error or exception.

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.

3 participants