Problem/Motivation
in PromotionSubscriber::getPromotions we first check what offer_ids are applicable, and filter out only the ones that implement ShipmentPromotionOfferInterface.
If this is exactly 0, the function getOfferIds will return an empty array.
It is the passed to loadAvailable from PromotionStorage. If the array is empty, then this check will not apply:
if ($offer_ids) {
$query->condition('offer.target_plugin_id', $offer_ids, 'IN');
}
Leading us to load all available promotions (not only the ones that implements ShipmentPromotionOfferInterface). Later the site will then crash either because of this (depending on the php.ini values for assertions):
assert($offer instanceof ShipmentPromotionOfferInterface);
Or because the offer plugin does not have a method called isDisplayInclusive
Error: Call to undefined method MyOfferPlugin::isDisplayInclusive()
Either way, the site crashes, and it might not be possible to check out.
Steps to reproduce
- Make sure you have no shipping promotion plugins
- Make sure you have some non-shipping promotion plugins
- Make sure you have some promotion created
- Go to the checkout
Proposed resolution
Make sure we do not try to load non-relevant promotions
Remaining tasks
Write tests.
Review. Commit.
User interface changes
None.
API changes
None.
Data model changes
None.
Comment | File | Size | Author |
---|---|---|---|
#4 | 3172728.patch | 5.76 KB | eiriksm |
| |||
#4 | 3172728-test-only.patch | 5.13 KB | eiriksm |
#2 | 3172728.patch | 645 bytes | eiriksm |
|
Comments
Comment #2
eiriksmShould be possible to write a test for this, but here is a fix at least.
Comment #3
eiriksmComment #4
eiriksmA test only patch (which hopefully fail), and the fix again.
Comment #6
jsacksick CreditAttribution: jsacksick at Centarro commentedThis looks good except this :):
Comment #7
jsacksick CreditAttribution: jsacksick at Centarro commentedThe one weird thing though... is why don't you have any shipping promotion offers?
Comment #8
eiriksmFair question :)
Because they are unset so the client will not attempt to use them, since either the erp system or the payment provider (don't remember off the top of my head) does not support it somehow :)
Comment #11
jsacksick CreditAttribution: jsacksick at Centarro commentedFixed comments, violations and a slight change to the alter() method.