uc_price() is a new complicated thing and needs explaining

Which issue was that? I remember fixing something like that, but not where the problem was.

Can you use the Devel module to provide me with a backtrace on error? I'd like to know what line of code is calling uc_price().

I believe the problem is that $context['revision'] already exists so you can't use the += operand. Changing it to + solves the problem.

  $context += array(
    'revision' => 'themed'
  );

is replaced by

  $context = array(
    'revision' => 'themed'
  );

No, the 'revision' key is used more often than not, so if += really didn't work, there would be a lot more errors happening.

I don't get an error when editing a product that is part of a kit, and I don't think I've changed anything that would have fixed it without knowing it. Do you have other modules that would do something when nodes are updated? There might be some conflicts in there somehow.

I run into this problem as well using this code.

theme('uc_product_price', $pnode->sell_price, 'sell_price')

I am trying to spruce up uc_webform_productize module. It was simply printing the prices without using existing theme functions of UC.

Update
I tried what #4 put and this also cleared up the error but I don't know if that would break something else.

It most certainly will break something else. The $context is supposed to be an array that contains several pieces of data that allows other modules to know if they should alter the price in some way or not. The change in #4 overwrites any information it might contain with a default value that is actually only rarely used.

I see now that I should have handled this API change better, because the second argument of theme_uc_product_price() is now supposed to be an array.
I think you should be able to use the same data as uc_product_view() when you use theme_uc_product_price() in your own pages:

  $context = array(
    'location' => 'product-view',
    'class' => array(
      'product',
      'display', // or 'list, 'cost', or 'sell'
    ),
    'subject' => array(
      'node' => $node,
      'field' => 'sell_price', // or 'list_price', or 'cost'
    ),
  );

You should change $context['class'][1] and $context['subject']['field'] to match the particular price field that you want to output.

Additionally, you can pass a third array, called $options, to prevent the price's label from being displayed.

  $options = array(
    'label' => FALSE,
  );

The default behavior of uc_price() is to display the label, so you don't need to use this parameter if that's what you want.

I figured that would break other items by making that change.

I changed it to an array and I no longer this error. I had looked at the documentation to get the info on the theme function, but since it was not updated I got this error. My guess is that the original error on this issue stems from the same place.

Of course... now I don't get any price displayed. I am probably not using the right variable in place of $context['subject']['node']. Is that supposed to be the product node or just the product node ID?

Update
I figured it out. Thanks

It's the product node object, for anyone else following along.

I think this is solved?

No, because this is a Documentation issue. We need documentation to help developers understand how to use uc_price().

Oi, take a look see http://www.ubercart.org/docs/developer/11375/price_api

Thanks, Ruben.

I'd like to open this up again, because I think uc_price() still needs a lot more explanation.

Most Ubercart users, whether they're writing theme functions, or templates, or module, need to know just one thing about uc_price(): How should it be used to display a price? The existing documentation is not geared towards end users at all, and doesn't answer this question. I would like to see one good clear example showing how to display a simple price, and showing how to port code that used uc_currency_format() (which was simple enough that everybody used it consistently) to code that uses uc_price(). What is the minimum necessary to display a price? Can you just call uc_price($value) for example? If not, why not? Why can't context default to something reasonable, and only force people to deal with context if they want to do something special? It seems like now the context is used to "push" information into the price alterers, rather than as a way for alterers to "pull" information from the module that calls uc_price(). If that is a correct interpretation, then the architecture is doomed to fail because modules can't possibly know a priori what an alterer will need, and alterers can't anticipate what a module will provide.

I'm still not convinced that uc_price() is the right way to go; This is partially due to my lack of understanding of the intent of all the pieces and partially due to my lack of understanding of the big picture - what problems are intended to be addressed by uc_price() ? It would help me a lot if there were detailed documentation. Most of the information is still just in a thread or two discussing uc_price. As a minimum, the conclusions of those threads should be documented. I also think detailed documentation of the uc_price() architecture will help to expose any flaws in reasoning. For instance, I can't see why context is purely arbitrary - how does that help people who write price alterers or price formatters? Also, is the altered price saved in the order table in the DB, or is the unaltered price saved, or both? If the altered price is saved, does the order also store all the information that went into calculating the altered price? I don't understand how a store owner is supposed to keep proper books this way. I'm also concerned that the current architecture, which allows only one price formatter, prevents VAT and Multicurrency from working together, since they both need to format the output (VAT needs to add things like "including VAT" to the price, Multicurrency needs to add the localized currency symbol, etc.) . Likewise, a formatter that prints a strike-through "list price" won't work with VAT or Multicurrency, nor will a formatter that prints "call for pricing". As I said above, a clear articulation of the intent, architecture, use-cases, etc. will serve to reveal whether these problems are just my lack of understanding or are flaws in the current design.

In short, I think the documentation has a long way to go. I'd like to help with it myself, but I just don't understand it enough to write the documentation or even give a reasoned critique of the API.

I'm not sure I should have even written documentation since we still have open issues about how the API is working at the very core.

First off, I think you're right TR. I think we do need to supply a default method for using uc_price, and if people don't supply any context info, then they can't expect any module to be able to alter their price in a meaningful way.

However, I disagree with your observation that if modules have to push data to uc_price then the architecture is 'doomed'. I think of it kinda like HTTP POST and form structure. The client is the one actually pushing the data, but the server (read: the Price API) sets the standard for how the data that the client sends is structured. I think we're getting closer to solving this reasonably... Location hierarchy seems a way that modules could in effect know 'a priori' which type of location they're dealing with. Of course, there might be one 'product' location they wanna change that gets added after that module's installed... how to deal with that? Well, my location matrix was shot down... I'm interested in other's potential implementations...

Anyways, let me address your concerns.

"For instance, I can't see why context is purely arbitrary - how does that help people who write price alterers or price formatters?"

I'm not sure how else to make sure the kind of information gets down to the alter/formatter as it needs to be otherwise. It seems to me we;re better off suggesting, not enforcing... that is to say, "put node in $context['subject']['node']", etc. I don't see any way to ENFORCE this, so in essence, I don't see how to make it not "arbitrary".

"Also, is the altered price saved in the order table in the DB, or is the unaltered price saved, or both? If the altered price is saved, does the order also store all the information that went into calculating the altered price? I don't understand how a store owner is supposed to keep proper books this way."

As far as I know, order totals are generally formatted but not altered. One thing I'm curious about it whether multi-currency has any desire to modifyh these prices based on exchange rates. If so, we have to expose it, but we also have to do it in a way so that most alterers can quickly avoid it, realizing it tends to be a static amount. Please check out the VAT/context issue for more discussion on this. I'm sorry that the information is spread around right now, but it's a living discussion.

"I'm also concerned that the current architecture, which allows only one price formatter, prevents VAT and Multicurrency from working together, since they both need to format the output (VAT needs to add things like "including VAT" to the price, Multicurrency needs to add the localized currency symbol, etc.) . Likewise, a formatter that prints a strike-through "list price" won't work with VAT or Multicurrency, nor will a formatter that prints "call for pricing"."

You're confusing two aspects of the Price API (and yes, this should be documented better, but has not been yet, I believe). Formatting handles everything like decimal point, rounding, currency sign, etc. There's another mechanism(s) for things like 'including VAT', 'call for pricing', however.

Alter functions get a reference to the options array used to tweak various aspects of the price alteration/formatting/display. Among those options are 2 called 'prefixes' and 'suffixes', used by the *theme* layer to add prefix/suffixes to the price after all altering/formatting has been done. You can see that in the default alterer:

/**
 * Default price handler alterer; adds the default prefixes to the various
 *   product prices when viewing a product node.
 */
function uc_store_price_handler_alter(&$price, &$context, &$options) {
  // If a class was specified in the price's context array...
  if (is_array($context['class'])) {
    // Look for a product price type in the class array and adjust the price
    // prefix accordingly.
    if (in_array('list', $context['class'])) {
      $options['prefixes'][] = t('List Price: ');
    }
    elseif (in_array('cost', $context['class'])) {
      $options['prefixes'][] = t('Cost: ');
    }
    elseif (in_array('sell', $context['class'])) {
      $options['prefixes'][] = t('Price: ');
    }
  }
}

which adds the prefixes when you're viewing a node's prices. 'Including VAT' is another example of a prefix.

As for 'call for pricing', there is yet another mechanism I included, this time in the $price_info array passed to uc_price, key 'placeholder'. If you put a value here, that value will verbatim be used as the price. It will skip formatting completely. That's what you'd use for 'call for pricing' in place of a complete price.

Obviously, there may be a better way to do the stuff I got here, but so far I've been told this support is needed for 2.x without any real direction, so I've sort of had to throw everything I'm documenting now together. It has been tweaked since and I didn't really get a say in how Ubercart core implemented the Price API (the buck stops at Lyle for the core implementation), so some things ended up different than I had planned. However, I believe we are making progress, and I welcome dialogue on how to improve the system, and also critique.

I do plan on geting everything documented but it seems that since the API hasn't really solidified yet, it might be a mistake to do that. I don't think the fact that the API isn't solid yet is a sign of problems either. I think that we all have a desire to get it right and not just commit and forget!

Thanks for the clarifications. Don't forget the last sentence of my previous post: " ... I just don't understand it enough to write the documentation or even give a reasoned critique of the API". My comments weren't meant to berate the current structure. Rather, they're a plea for help trying to make sense of the uc_price() implementation, since it doesn't make much sense to me. (See the title of this thread!) I just don't know at this point how to properly re-engineer my Multicurrency module to use uc_price(), and I don't think that any open issues about how uc_price() *should* work can be resolved without trying to implement some real-world use cases.

Let me address a few of your points:

However, I disagree with your observation that if modules have to push data to uc_price then the architecture is 'doomed'. I think of it kinda like HTTP POST and form structure. The client is the one actually pushing the data, but the server (read: the Price API) sets the standard for how the data that the client sends is structured.

I was speaking specifically about the context part. If it were just price being pushed, there would be no problem. To take your analogy further (always a danger ...), the server can only provide a meaningful reply when the protocol elements are agreed on ahead of time. In a POST, the client passes name/value pairs to the server, which the server uses to form a proper response. If these name/value pairs correspond to context location elements in uc_price(), and if the context location is arbitrary (i.e. the client is not restricted in what to enter in the name/value pairs), then the price alterer (server) can't in general know what to return. This problem is reflected in the only actual code I know of that implements a price alterer, namely uc_vat. In uc_vat you have a big switch statement checking all sorts of possible context locations generated by core. But what if a new module comes along and defines a new context location? Then uc_vat doesn't know what to do with it, and the 'default' case may be the wrong thing to do. Either uc_vat has to be prescient or module writers have to be hyper-aware of what locations uc_vat expects to see - impossible because many module writers may not even know or care about uc_vat. This is what I meant by 'doomed'; it seems like a high-maintenance scheme that requires constant hands-on just to keep modules updated and working properly.

If context locations had to be elements of a pre-determined set, then a price alterer would be able to address all possible locations from the set, and contributed modules, the clients, wouldn't have to know anything about the alterers.

I think we're getting closer to solving this reasonably... Location hierarchy seems a way that modules could in effect know 'a priori' which type of location they're dealing with. Of course, there might be one 'product' location they wanna change that gets added after that module's installed... how to deal with that? Well, my location matrix was shot down... I'm interested in other's potential implementations...

My only thought along these lines is that the location part of the context should be taken from the current path. So, a product price shown on the page at the URL /catalog should be the same whether that price is in a block, in a node view, or in the product list - you certainly don't want to have three different prices showing. So it seems to me that the path provides sufficient *default* location. Like with the Drupal menu system, location would be hierarchical, so new modules which define menus underneath existing menus would be processed by their first 'parent' location. Also, except for administration menus, modules are far more likely to modify elements of existing pages than define new pages, meaning contributed modules will automatically use the existing context location for that page. Just a thought anyway - again I don't understand enough about the intent to judge whether this will meet the envisioned needs for price altering/formatting.

As far as I know, order totals are generally formatted but not altered. One thing I'm curious about it whether multi-currency has any desire to modifyh these prices based on exchange rates. If so, we have to expose it, but we also have to do it in a way so that most alterers can quickly avoid it, realizing it tends to be a static amount. Please check out the VAT/context issue for more discussion on this. I'm sorry that the information is spread around right now, but it's a living discussion.

Product prices are also saved in one of the orders tables, not just order totals. That way there's a record of the price paid for a product independent of any future price changes. If the altered price is saved, then it's important to also save information about how and by what means the price was altered. So I'm still confused whether uc_price() is a theme-layer function like uc_currency_format() or whether it affects the numbers stored in the DB in the orders tables.

Prior to uc_price(), Multicurrency was implemented by modifying uc_currency_format(), so Multicurrency only affects the display of the price to the end-user. The prices stored in the product or order tables are never changed. However, Multicurrency has its own table which stores all the conversion and formatting factors, indexed by the order number, so that the conversion calculations can be reconstructed at a later time (e.g. in the order history view). Because of this, my first impression is that under the new uc_price() scheme Multicurrency needs to be a price formatter, not alterer. But if only one formatter is allowed, I'm afraid Multicurrency won't be compatible with any other formatter. The other issue is that Multicurrency will need to also be an alterer, so that saved orders can be viewed using the conversion rate in effect on the day the order was placed. Also, Multicurrency has a rounding function so that converted prices can be displayed as nice even figures if desired - this is a job for an alterer as well?

You're confusing two aspects of the Price API (and yes, this should be documented better, but has not been yet, I believe).

I'll take a look at what you wrote, and the default implementation, and try to digest it. So it sounds like a price alterer is part form API array (without the processing via a drupal_render_form() function) and part theme function, with the formatter part just being a substitute for the PHP number_format() function? And uc_price() calls the price alterers, which in turn use the one formatter? I will have to try to figure out how the prefix/suffix works in the case of multiple alterers... One other thought this brings up is how can prices be themed in the conventional sense? Only by a price alterer? Say I wanted to move the label from before to after the price, or say I want to wrap the price in a div so I can change the color. Is this still done by theme_uc_product_price() or something similar?

I think this does call for documentation, even if uc_price() is not finalized and subject to change. A big part of my problem has been trying to extract what was implemented out of a thread discussing different possibilities. The source code is the only real record of this, but code doesn't explain intent or architecture so I can't tell when a perceived problem is due to design, to implementation, or my own misunderstanding.

Well, let's take the analogy even further.. *ducks*

You could say the POST *is* arbitrary, because the client can send whatever NVP they wan't, the issue is that only ones that conform to the structure we agree on will be processed... the rest are silently ignored (usually). It's the same with locations... You can see in the other thread we're talking about location sets like you referenced. If my module set a location 'foobar', I wouldn't expect any modules to have any clue about it. We're talking about making a hierarchy of locations so that we can say 'this is a cart price' or 'this is a line item price', then specializing further if needed, 'this is a shipping line item', 'this is a cart subtotal' Please check this out over here and drop 2 cents :) http://drupal.org/node/475474#comment-1732856

This would potentially be the defined 'sets' of locations that I mentioned could let modules in the future know what they're dealing with. Nothing is solidified yet. The usage of uc_price in core would have to be standardized to make this work...

Unfortunately, I disagree that we should use the Drupal path as the hierarchy... one reason I can think of is blocks, like the cart block... but besides that, I'm just not sure it's going to be descriptive enough, and plus... doesn't that bring us back to the 'a priori' problem? If anew module says 'I have a product price' we can handle it.

I think multicurrency could reside purely in the formatting realm actually... It seems to me that if your store has something for $20, you wouldn't want '20,000' to be store if the customer paid in yen would you? So I think the behavior won't really have to change.

The default formatter just takes the place of uc_currency_format(), allowing more things to be specified like precision and the actual sign. You could realistically probably just wrap the default formatter with your own logic, change $options 'sign', 'dec', and 'thou' ('sign_after', too?) and change the $price with the exchange rate before you pass it in.

I'll get some more documentation up about this stuff, I'll make an example of how a multicurrency functionality could be implemented as I described if you want.

I'll elaborate on the location suggestion over at #475474: Price alterer contexts and VAT support. Hopefully we can also sprint on this in Denver this weekend.

I think multicurrency could reside purely in the formatting realm actually... It seems to me that if your store has something for $20, you wouldn't want '20,000' to be store if the customer paid in yen would you? So I think the behavior won't really have to change.

Well, ~2,000¥ if you want to be pedantic (I do, I do!) :-) Anyway, yes, this is what I was saying - for just product prices Multicurrency can be a formatter. And that's the way it works now. But Multicurrency also has to be an alterer for things like order history, and Multicurrency needs to alter the number used by the payment module if the store owner wants the charge to be in the localized currency rather than the store default currency.

Sorry I hadn't seen this discussion. Really great stuff.

I'd be down for helping out on the sprint this weekend, as long as we can get Thai food beforehand.

We also need user documentation.

Let's make sure we figure out how the API is structured before we write docs!

.

Subscribe.

Stuff on how hook_uc_price_handler alterers and formatters will definitely be needed.

Alright. Updated the developers' guide to the new version of the API and added a users' guide page to explain the settings page.

Nice!

But could you also explain what is in the $context and $options that come through to a price alteration function?
In particular, what do 'revision' and 'type' mean?
And what's the flow through these at various stages in a product's life, eg as a node, in a cast, at checkout, etc.
I am right now looking at my cart and doing a dsm of it and totally confused -- seems to be called about 4-5 times with different values, only some of them have a $subject, and I don't understand what these all do or which is the right phase to change things on.

For that matter, the formatter too. That has many passes, most of them mysterious.
Eg, I force price to be 0 in my alterer.
Then I output what my alterer gets, and I see:
0, 0, 0 ... original price, 0, 0, 0 ... original price ... 0.
What is going on here?

Can you review the developer docs? I see these things explained in there... perhaps start a forum thread in the docs forum if there's something insufficient in there.

For the most part, and I can't remember if there are any exceptions currently in the code, uc_price() should only affect the way a price is displayed, not how it is stored in the database. So, if a price alterer wants to change a product's price in all of the stages of the workflow, it has to work with the product, cart_item, and order_product types.

The revision just says how much the price has been changed when it is returned by uc_price(). 'original' means nothing has changed, 'altered' means the value, 'formatted' adds currency markers and thousands separators, and 'themed' adds HTML on top of that. 'formatted' and 'themed' can also return the original value with 'formatted-original' and 'themed-original'.

When you're tracing through uc_price(), it's best to look at the entire $context as a whole. Each piece of it is important in determining how it will affect $price_info. As you do that, compare $price_info to different parts of the subject to find the object it came from. Hopefully this will help you figure out what the alterers are looking at.

Ah right, I see the list of possible values of 'Revision'.
Could you write that list as a life-cycle? Things like themed-original I'm confused about: when it happens, and why you might use it.
And I can't see anyting about multiple calls to the formatter.

(Have you considered opening up your docs like on d.org?

I can't really, since revisions aren't part of a life-cycle. Each of them is available to be used if it is needed whenever you need it. It's probably better to think of them in a table:

          | original value     | altered value
----------------------------------------------
numeric   | original           | altered
formatted | formatted-original | formatted
themed    | themed-original    | themed

(Why, yes. We've already done that.)

Never realized that rszrama had put it back to fixed in his post. I would say that the issue is fixed because documentation does exist for the price API. If it's insufficient, then we can change it as needed, and anyone can do that.

> If it's insufficient

Yes.

For instance, what do the possible values of 'type' mean? When are they used?
Eg, how many places in the Ubercart UI use 'line_item' as a type? Without this, one thing unravels as I fix another.

> then we can change it as needed, and anyone can do that.

The page http://www.ubercart.org/docs/developer/11375/price_api does not seem to be editable by ordinary users.

What I mean by lifecycle above is illustrated by this:

uc_discount_price_handler_alter() on the node display page gets $context['revision'] = 'formatted'.
But the node/XX/edit/attributes page gets $context['revision'] = 'themed'.

The handler alteration seems to be happening earlier / later.
Why, and what is the difference?

While working on #1097668: Multicurrency support I've seen several places where uc_price() calls are called inconsistently, and I think this can be cleaned up although possibly at the expense of needing changes in existing contrib that calls uc_price().

If anyone from this old issue is still interested in getting this fixed, speak up now and let's try to get this sorted out once and for all, and at least document everything in the uc_price() function header if nothing else.

Making Ubercart core's calls to uc_price() consistent would be a terrific thing. uc_price() is complex enough without the inconsistencies.

longwave has a patch at #1097668-6: Multicurrency support which is part of his work on adding multicurrency support, but that patch appears to include a number of fixes for calls to uc_price(). Would it be worth splitting that into it's own patch which just tries to make calls to uc_price() consistent?

As noted in that issue and #854638: order products price should locked in altered price the main problem here as I see it is backward compatibility - uc_price() has been around long enough that many contributed and unknown custom modules use it and may implement price handlers, so how do we make changes mid release cycle without breaking things?

uc_price is giving a terrific headache also to me. I need to calculate the price of an item after attribute and custom price module have altered its price. I need to do this inside add_to_cart hook, so I only have nid, quantity and data available to make the calculation. I'm trying to make sure the price of an item is greatear than a minimum required by store owner. Anyone could help me?

uc_price() is still complicated, but it is no longer new, and it is very unlikely that the documentation for 6.x-2.x will be improved now.

#1304884: uc_cart no longer supports uc_multiprice module