search

pimcore / ecommerce-framework-bundle

Ecommerce Framework community bundle provides e-commerce functionality such as product listing and filtering, pricing, carts and checkouts for Pimcore.
https://pimcore.com/docs/platform/Ecommerce_Framework/
Other
8 stars 30 forks source link

Improve exception for orders with aborted payment state #64

Closed mysliwietzflorian closed 1 year ago

mysliwietzflorian commented 1 year ago

With this change, a developer can react to different state updates from the API.

In our case, we see it as a critical error, when the new state is considered successful but was already aborted. Otherwise, we just log it normally.

What do you think?

github-actions[bot] commented 1 year ago

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

mysliwietzflorian commented 1 year ago

I have read the CLA Document and I hereby sign the CLA

mattamon commented 1 year ago

With this change, a developer can react to different state updates from the API.

In our case, we see it as a critical error, when the new state is considered successful but was already aborted. Otherwise, we just log it normally.

What do you think?

For me this is a valid improvement, the only problem I do see, is like if someone already caught the UnsupportedException then he has to change to the new Exception now, so it would be a BC right, unless you could still catch this exception with an UnsupportedException check?

mysliwietzflorian commented 1 year ago

[...] if someone already caught the UnsupportedException then he has to change to the new Exception now, so it would be a BC right

We could extend UnsupportedException instead of AbstractEcommerceException. Would that solve your concern? @mattamon

mattamon commented 1 year ago

[...] if someone already caught the UnsupportedException then he has to change to the new Exception now, so it would be a BC right

We could extend UnsupportedException instead of AbstractEcommerceException. Would that solve your concern? @mattamon

I guess so, I would still need to test it :) But that would be better, for a smooth upgrade, since 11.0.0 will be today and we cannot do any BCs anymore :)

mysliwietzflorian commented 1 year ago

@mattamon I've pushed the changes.

Good luck with the update! 😉