The PriceDefaultFormatter class is defining the store context although not using it itself. Instead, only the inherited PriceCalculatedFormatter class is actively using the store context. I see no benefit at all in including it in the base class already, especially as the child class is adding more properties and needing a constructor override anyway.

I do have an use case, where this difference is relevant:

I'm currently working on a customer project, where I'm defining a custom entity type, that is (currently) not used as a Commerce purchasable entity, but does need a price field. I was considering the pros and cons about using the price field from commerce_price anyway as base vs defining my own field (effectively duplicating code from Commerce). As the chance is quite high that in the long run these entities actually will be converted into purchasable entities, or at least Commerce functionality is added on another part of the site, I decided to go for the first approach and use a commerce_price field.

At the current state, I do not want to enable too much sub modules I do not really need just to get the price field working. I've enabled the module, added a field, chose to use the default formatter and immediately ran into an exception: the reason was that I haven't enabled commerce_store, but the default formatter is declaring a Drupal\commerce_store\StoreContextInterface variable. But on closer look I saw that it isn't used directly, but only on the calculated formatter.

So, unless there isn't a good reason to keep it in the base class, I propose to move it to the child class. I'll provide a patch and a PR

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

agoradesign created an issue. See original summary.

agoradesign’s picture

Status: Active » Needs review
FileSize
4.74 KB

bojanz’s picture

Status: Needs review » Fixed

This is a fine cleanup. Thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.