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

adding 404 status_code #73

Closed
wants to merge 1 commit into from
Closed

adding 404 status_code #73

wants to merge 1 commit into from

Conversation

mrrfly
Copy link

@mrrfly mrrfly commented Jul 7, 2022

Fix Output For 404 Status Code

From This:
Screen Shot 1443-12-07 at 10 36 47

To This:
Screen Shot 1443-12-07 at 10 39 26

@Andrenzo17
Copy link
Collaborator

Hi @mrrfly Thanks for the Open PR.

Currently, Midtrans Php has not handled custom object errors. So that the error is more descriptive and the developer knows the error comes from the Midtrans API, that’s why we added “Midtrans API is returning API error. HTTP status code: ……….”

Could you more clarify, what is your concern to add this enhancement? does the exception object not useful enough?

Thank you

@mrrfly
Copy link
Author

mrrfly commented Jul 7, 2022

oke, simple.

How do I find transactions?

if i find and i found, status code is 200. and return an object.

Then, what if transaction is not found ?
Now, midtrans using exception object, it doesn't match.

That's why I turn it into an object if the transaction is not found and that will make it easier for the developer.

Maybe, you should check this issue: ##66

Thank's

@Andrenzo17
Copy link
Collaborator

Thanks for clarify.

Our concern if we only throw JSON it will be hard to debug, for example if merchant connect to multiple API and Midtrans library only throw JSON result. It wiil be difficult for merchant to find the error.

Yes, we have already checked the previous issue. Also we will put as our backlog mark as an enhancement to implement exception object can be like other library Midtrans which can expose API response.

Thank you

@mrrfly
Copy link
Author

mrrfly commented Jul 7, 2022

But this commit is only for 404 status code.
Not 5xx or whatever

@Andrenzo17
Copy link
Collaborator

The current implementation work as we expected. 404 transaction doesn't exist case is considered as API error, hence it is intended to throw Exception.

However we understand the concern that there is a need from your side to be able to access the raw API response JSON as object, instead of string of Exception. So the correct approach will be to throw Exception, but the exception should have a properties that expose the JSON as an object. So it will be more useful if you intend to implement try-catch. We will assess internally and add to backlog.

Thank you

@mrrfly
Copy link
Author

mrrfly commented Jul 8, 2022

Thank you for the explanation.
I will close this pull.

Thank's for the work 👍

@mrrfly mrrfly closed this Jul 8, 2022
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.

2 participants