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

This module relies on class ShippingFrameworkModifier existing #34

Closed
jameshoward opened this issue Feb 9, 2015 · 9 comments
Closed

Comments

@jameshoward
Copy link

If I install the discount module on a base shop install I get the following notice:

Modifier class "ShippingFrameworkModifier" does not exist.

I've tracked this down to Shop\Discount\Calculator line 92:

if($shipping = $this->order->getModifier("ShippingFrameworkModifier")){

In Order::getModifier there's a ClassInfo::exists check that's throwing the user_error because that class doesn't exist.

Either there's a dependency on silverstripe-shop-shipping or a better way of determining if the order has that modifier without needing the class to exist.

@markguinn
Copy link
Contributor

Good catch, @jameshoward. I've run into that before as well. Just created pull request #35. Assuming it passes CI I'll merge it today. @jedateach I don't think we want to require the shipping framework do we?

@jameshoward
Copy link
Author

Two potential solutions I can think of:

  1. Change getModifier so it returns NULL rather than throwing an error. Without knowing where else this method is called from, I can't tell if this actually breaks more than it solves.
  2. Implement a hasModifier method. This could do a couple of lazy checks first:
// Lazy checks for class existing and order having modifiers.
if (!ClassInfo::exists($className) || ($this->order->Modifiers()->count() == 0)) {
    return false;
}

// Check whether the order has the modifier...

@jameshoward
Copy link
Author

Or your 3rd option in #35 that looks much less risky! :)

@markguinn
Copy link
Contributor

@jedateach it's worth thinking about #1 above as a fix in the shop module itself.

@markguinn
Copy link
Contributor

35 is merged

@wilr wilr closed this as completed Feb 2, 2016
@wilr
Copy link
Contributor

wilr commented Feb 2, 2016

This has been resolved.

@isaac-at-room9
Copy link

How long until this is released? We're using this module and it has been over a year since anything was released. Ran into this issue today with the latest release, had to fix the package at a commit (not preferable)

@wilr
Copy link
Contributor

wilr commented Sep 6, 2016

@isaac-at-room9 Tagged a 1.2.0 release.

@isaac-at-room9
Copy link

@wilr oh, i must've missed that one when I first installed the package... I can't see it on https://packagist.org/packages/silvershop/discounts

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

No branches or pull requests

4 participants