Come together with the global Drupal community in Rotterdam, 28 Sept – 1 Oct 2026. Sessions, contribution, connection, and Early Bird savings until 8 June._uc_attribute_alter_form() abuses the ternary operator when calculating the total or adjustment price to be displayed, this can be rewritten to make it easier to follow.
Spawned from #1301430: Inclusive tax implementation is not flexible (attribute options) but split out here so it can be committed separately.
| Comment | File | Size | Author |
|---|---|---|---|
| #16 | D6-1376724.patch | 4.48 KB | tr |
| #8 | 1376724-attribute-refactor-7.patch | 15.72 KB | wodenx |
| #6 | 1376724-uc_attribute_alter_form-5.patch | 4.48 KB | wodenx |
| #2 | _uc_attribute_alter_form-refactor.patch | 2.94 KB | longwave |
| #1 | _uc_attribute_alter_form-refactor.patch | 2.11 KB | longwave |
Comments
Comment #1
longwaveFurther minor change to make the adjustment +/- prefix easier to see.
Comment #2
longwaveThis patch adds a new theme function so the total/adjustment display can be rethemed to be "Option (+$1.00)" etc. instead of "Option, +$1.00".
Comment #3
tr commentedI haven't tested it yet, but +1 for the concept.
Also, please take a look at #1238610: uc_attribute not displaying the product price in some cases - you should make sure your refactoring addresses that issue at the same time.
Comment #4
longwaveI think that other issue may be considered a feature not a bug, I will think about it a bit more but I am not sure whether it should be changed.
Comment #5
longwaveCommitted #2. Will leave the other issue open for a bit, but I think it works well as-is and should be considered by design, unless someone else can find a good argument for changing it.
Comment #6
wodenx commentedIf you have more than one priced attribute, options with 0 price are displayed (inaccurately) as a total price, while those with an adjustment display an adjustment. The first patch in this sequence fixes this and guarantees that in those cases (where the actual total depends on the combination of options selected), the prices are always displayed as adjustments.
It should be possible to do better than this via an ajax update whenever an attribute is selected - i'll look into that tomorrow morning.
Regarding the other linked issue - I can see that if you have a catalog page full of some product with an attribute that has several options, but one of the products has only one of the options available, you might want the price to display for that as well. the second part of this patch provides a mechanism to determine whether an attribute should be considered 'priced' based not-only on whether the individual product has a nonzero option, but (optionally) whether the product *class* has an option with the default price. Take it or leave it - but this way, the store owner can decide how he wants it displayed.
Comment #7
wodenx commentedAnother, perhaps better, option would be to provide the product price (w/o options) as an add'l argument to theme_uc_attribute_option(). Then this could be resolved on the theme level.
Comment #8
wodenx commentedOK - Here's the ajax functionality. With this, it should be possible to display attribute options as total prices even when there are more than one attribute for a product. Checkboxes are a little trickier. If you display the total price for checked boxes, then you lose information about what would happen if you unchecked the box. So maybe it's better to continue to display checkbox option prices as adjustments in all cases. Anyway, the attached patch (2 parts) offers both solutions.
Comment #9
wodenx commentedComment #10
wodenx commentedComment #11
longwaveCommitted the first part of this patch, thanks for fixing that.
For the second part I can see where you're coming from here but I think this might be even more confusing depending on how a store owner sets up their class attributes, and it doesn't take into account the default attribute option prices. I still think the existing behaviour is the best option, and as only one person considers it an issue, changing it might cause more problems than it solves.
I want to see more Ajax used in the Ubercart front end but found the price updates a bit confusing/disconcerting when in "total" mode. I think we should hold off implementing this until we can also update the sell price and display price fields via Ajax at the same time (and perhaps the weight too), effectively bringing uc_aac into core - and preferably let's break this out into a separate issue, here I was only trying to refactor existing code rather than add new functionality.
Comment #12
wodenx commentedFair enough. What about providing the product sell price as an additional argument to the theme function?
Will do and post this patch there as a starting point, along with further comments.
Comment #13
longwaveI think we have more important bugs and features to work on at the moment than spending time making minor improvements to this feature.
The original issue here is fixed, let's continue the Ajax work in the other issue you opened.
Comment #15
tr commented#2 needs backporting to 6.x-2.x.
See duplicate issue #947148: You need to hack _uc_attribute_alter_form to change how option prices display
Comment #16
tr commentedAttached patch is a backport of #2 + first part of #8, which fixed a bug in #2.
I'd appreciate a test from someone before I commit this.
Comment #17
longwaveWorks for me. Committed.