-
Notifications
You must be signed in to change notification settings - Fork 93
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
Add details to error messages to make them more helpful #372
Conversation
d42bda2
to
03322e8
Compare
lib/recurly/errors.php
Outdated
|
||
public function __toString() { | ||
if (!empty($this->field) && ($this->__readableField() != 'base')) { | ||
return $this->__readableField() . ' ' . $this->description; | ||
$details = $this->details ? ' Details: ' .$this->details : ''; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit but space after the period?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, that was my bad. Fixed
lib/recurly/response.php
Outdated
@@ -57,7 +57,8 @@ public function assertValidResponse() | |||
case 0: | |||
throw new Recurly_ConnectionError('An error occurred while connecting to Recurly.'); | |||
case 400: | |||
$message = (is_null($error) ? 'Bad API Request' : $error->description); | |||
$details = $this->details ? ' Details: ' .$this->details : ''; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems odd to be referencing $this
rather than $error
? Though in either case if it runs through parseErrorXml
I'd expect an error object to come back and that we could use the interpolated value in $error->message
. How were you testing this out?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you were right, that was supposed to be $error
rather than $this
in that context
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, there is no unit test for this change. I'll add a test in the Tests
directory.
03322e8
to
291c36a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
291c36a
to
b38b012
Compare
@drewish I added a test to show that the |
b38b012
to
08c59f8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me
When an error comes back from the server, it has a details tag in the XML. For example:
Just stating that the provided XML was invalid is rather useless to a developer trying to fix their issues. Adding the detail of the unacceptable tag is much better.
This issue was originally highlighted by #344, which shows an example of where a developer would have been able to fix their issue more easily if they could see the details of the error.