While I was tracking down coupon display issues, I got a look at the internals of this field handler and the code used to determine a discount's value display. It currently invokes an alter hook on an empty string to generate a value:

  $display = '';
  drupal_alter('commerce_coupon_discount_value_display', $display, $discount, $order);

This is odd, because typically you're altering an initialized value. Additionally, the field handler itself shouldn't be encapsulating all of the logic here. If a value display is a thing we need to be able to generate in the module, there should be an API function that the handler invokes to request the value display of the discount, and it should arguably reside in Commerce Discount itself instead of Commerce Coupon. But we have to deal with the code where it is now first.

I believe it needs improvement in several ways:

  1. Remove all logic from the field handler, invoking a single API function that receives the discount and order objects and returns the rendered output.
  2. Instead of putting all of the logic in a single function like we have now in commerce_coupon_commerce_coupon_discount_value_display_alter(), we should use offer type callbacks. Commerce Coupon can alter those onto the offer types and define the functions, since it's the module defining this system.
  3. Each callback should receive the order and discount object and process it as necessary to return the summary of the offer.
  4. I don't like the way we currently have condition evaluation in the discount / offer type specific code block. That should be abstracted if possible, but I don't really know how to do it off the top of my head without having to return a variety of templates from the offer type callback that we replace conditions into.
  5. Once the callback has been invoked by the main API function, it should then invoke an alter hook. I don't see a need here for two alter functions like we have currently, though, especially since hook_commerce_coupon_value_display_alter() is currently just letting you alter rendered HTML. That doesn't make sense to me.

Ultimately, this isn't a very compelling feature, but we can at least clean up the implementation we have and ensure it accommodates all of the core Commerce Discount offer types and conditions.

Comments

rszrama created an issue.

josephr5000@yahoo.com’s picture

+1 on this report above. I just ran into this.