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 28 forks source link

Throw max usage reached exception #158

Closed dolmit-tanel-paaro closed 5 months ago

dolmit-tanel-paaro commented 5 months ago

Allow developers to catch the VoucherServiceException that takes place when voucher token's max usage limit has reached. This is necessary for displaying correct error message ('cart.error-voucher-code-' . $e->getCode()) to the end user.

github-actions[bot] commented 5 months ago

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

dolmit-tanel-paaro commented 5 months ago

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

dolmit-tanel-paaro commented 5 months ago

recheck

fashxp commented 5 months ago

Hmm, that could potentially break existing code, right? As now, an exception is fired which was not fired before.

dolmit-tanel-paaro commented 5 months ago

Hmm, that could potentially break existing code, right? As now, an exception is fired which was not fired before.

That's the same thing my colleague pointed out. :)

No, it isn't a BC break because the method was able to throw an exception before too. The method calls parent::checkToken which can throw various VoucherServiceExceptions. This all means all the existing implementations must have considered this and have included error handling.

fashxp commented 5 months ago

You are right! Still, I'm a bit worried putting that into a bugfix release. WDYT? Is it really a bugfix or more an improvement?

dolmit-tanel-paaro commented 5 months ago

I would say it's a bugfix.

Because before the fix, trying to apply an invalid (usages exhausted) token to the cart, it would fail silently. But it's not expected to do so. Looking at TokenManager\Pattern::checkToken, it throws an error when the token does not satisfy additional conditions.

Now the Single token manager behaves the same way, throwing an exception when additional condition is not satisfied.

fashxp commented 5 months ago

convinced, thanks for your insights and contribution!