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

Remove mdxi validation #91

Merged
merged 2 commits into from
Jan 17, 2019
Merged

Remove mdxi validation #91

merged 2 commits into from
Jan 17, 2019

Conversation

mdaskalov
Copy link
Collaborator

Simplified parsing of the SelectPayment response: closes #89 and #90

@zirnipa
Copy link

zirnipa commented Jan 16, 2019

Please implement a proper error handling.
Currently a rejected request shows an empty page and no error is written in the log file.

@stefanpolzer
Copy link
Contributor

stefanpolzer commented Jan 16, 2019

@mdaskalov this PR will have backward compatibility issue! If you delete the validation than every project that is using the public method Mpay24Order::validate() will beak! The best way is to mark it first as depricatdet an in the next mayor release e.g. 5.0.0 we delete it as announced.

Second, there are some code elements that are not 100% right from my point of view:
keep the __construct() method as it is, because the hasNoError() method is checking for general errors (e.g. authorization, ...)
so if a geneal error is present wen can't parse the response body anyway, see remark from @zirnipa. To avoid unexpected behavior we should do this check in the constructor, right?

Using the AbstractPaymentResponse Class seams to be a good idea but the SelectPayment do not have a MpayTid,
so using the wrong inheritance will end up with a object what have a method the will return null and leads to confusion.

My point is a object should mirror the attributes and methods that reflects the real response.
This could confuse other developers that are not so deep in this topic and may exact the TID, than open a issue and we have to make a remark in the documentation and so on ... you get the point!

@mdaskalov
Copy link
Collaborator Author

@stefanpolzer: I agree about Mpay24Order::validate(). We decided to release 4.3.2 now and then in 5.0 to support only PHP 7.2+ and drop validate().

Correct me if I didn't understood it right, but in my opinion with having hasNoError() check in the constructor the fields errNo and errText are never set, since they are returned only if the status is ERROR. But in this case hasNoError() returns false and parseResponse() is not called.

I also agree on the argument for using AbstractPaymentResponse class. Maybe we should create another class with no MpayTid and Location but having errNo and errText instead. Then we can add MpayTid, Location, etc. as needed.

@stefanpolzer
Copy link
Contributor

@mdaskalov you have a point, but the hasNoError() do not reflect if the error is a general on e.g. "401 Unauthorized" where the parseResponse() methode may be failing and therefor a Exception will be thrown and the hole PHP process is crashing, or an more specific one where we can parse the XML response and read the errNo and errText.

So from my point of view we should refactor some of the code so that we can distinguish between those two cases.

I could check this but I would write the code fore PHP 7.2+ if this is ok with you.

@mdaskalov
Copy link
Collaborator Author

That's ok for me. Then we can go directly for release 5.0.0. I will clarify with @zirnipa if we need a release without MDXI and support for PHP 5.3+ and will make it in the meantime.

I would remove MpayTid and Location from AbrstactPaymentResponse and move them to SelectPaymentResponse, AcceptPaymentResponse, etc. as necessary

@stefanpolzer
Copy link
Contributor

stefanpolzer commented Jan 16, 2019

@mdaskalov you don't need to remove any attribute/method form the AbrstactPaymentResponse Class. The Abstract Class is perfect for AcceptPaymentResponse and ManualCallbackResponse but sadly not for SelectPaymentResponse therefor this class need its own implementation because it is the only one with this list of response fields. Just reverse this commit 314c332 and you are fine with this class ... PR #90 has now everything includes ... if you need a version for PHP 5.3+ than you could build a different tree from that?

@mdaskalov
Copy link
Collaborator Author

OK, I made it simple: rollback and then just removed the MDXI validation without changing anything else. If it's ok for you @zirnipa I will merge and make a new release. Then we will go for 5.0.

I don't think we will need a separate branch. This will be the last release to support PHP 5.3+ and that's it.

Copy link

@zirnipa zirnipa left a comment

Choose a reason for hiding this comment

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

OK

@mdaskalov mdaskalov merged commit 9fb86b3 into master Jan 17, 2019
@mdaskalov mdaskalov deleted the remove-mdxi-validation branch January 17, 2019 08:57
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