Quantity Bug Revist

I'm sorry to keep beating a dead horse, but this quantity bug still isn't fixed in Magento. I fixed it in Agent Ohm almost 2 months ago.

http://github.com/markkimsal/agent-ohm/commit/496caa56c7c7796d02f7b922fe4beae3c6fbc132

If you're in Europe and you like to count ten thousand as 10.000, Magento will treat it as just ten.

http://svn.magentocommerce.com/source/branches/1.4-trunk/app/code/core/Mage/Checkout/Model/Cart.php

API Docs

I just uploaded HTML API documentation for Agento Ohm.

http://code.google.com/p/agent-ohm/

It was generated with my own secret c2x documentor. It only took about 4 minutes to scan and generate the entire thing. Incremental documentation only updates files that have changed (SHA1) hash so it's only about 1/10th the time to generate second time around. And it uses about 1/10th the RAM that phpdoc needs. If anybody is interested in beta testing c2x let me know via github or comment here.

Quantity Bug

I found another Magento bug today, going to fix it in Agent Ohm first.

When ordering quantities 1000 or greater, Magento cannot parse non-US number formats. In Spain, if you were to order "10.000" of an item, you would expect to get ten thousand of that item. But Magento gives you 10 only. The fix isn't as easy as using Zend_Locale_Format::getNumber (although this function does properly parse the number). Something even deeper is wrong with Magento.

This is just one more issue that points to the fact that Magento undergoes no automated testing before it is released.

The function "getNumber" in Mage_Core_Model_Locale says that it uses the current locale setting, but it does not. The locale is not referenced in the code at all.

Playing with the Zend Framework

http://libertini.net/libertus/2009/03/11/playing-with-zend-framework/

This blog entry is parallel to this entire blog to a certain degree. If you're interested in bogging about code analysis, you might like it.

How many times can I fall victim to the same bug?

I just lost about an hour tracking down a problem resulting from a naive transfer of a running Magento site to another server. If you simply use an old mysqldump client, or phpmyadmin your dump file will be corrupted, don't use it. Use mysqldump from 4.1 or later.

So, the problem revolves around magento using a frown-upon practice of staring primary keys at 0. Normally, in mysql, if you insert a value of NULL or 0 into a primary key field, it will run its sequence generator and give you the next sequence. Combine this fact with the nicety of your dump files specifying which auto increment to start at in the table definition, and you can say goodbye to your admin store (store_id 0) after you import.

The mysqldump tool will spot this and insert a call to change the SQL mode to "NO_AUTO_VALUE_ON_ZERO" for you, but phpmyadmin will not! Also note that Magento's own DB dump feature will put in "NO_AUTO_VALUE_ON_ZERO" for you as well.

Remind me to get rid of this bad, highly error prone, and frustrating "feature" of Magento - primary keys starting at 0 have to go.

checkout_cart_product_add_before

There is no event in Magento called "checkout_cart_product_add_before", but it is sorely missed.

Dealing only with "checkout_cart_product_add_after" means that you cannot easily massage the input data for a product. Basically, what this means is, if you are trying to manipulate product attributes which aren't submitted directly in the POST data, then you're in for a rough ride.

If your attributes aren't within certain bounds, or are completely non-existent, then you can't ever get your product into the cart. One solution is to make the attributes not "required" and massage the POST data into your attribute data after adding the product to your cart. But that just means that you have to duplicate all the bounds checking yourself, and go through the extra step of removing the item if it fails attribute validity checking (see: core/Mage/Catalog/Model/Product/Type/Abstract.php: public function prepareForCart)

Going to add the event checkout_cart_product_add_before to Agento-Ohm.

Security Issue

I've found a security issue which may or may not be related to other bug posts on the magento bug tracker. I would not recommend anybody use the back-end order creation tool in Magento or Agent-Ohm until further notice.

Update: It is not recommended to run any multi-store setup under Magento or Agento-Ohm.

Update: Fixed in the latest Agento-Ohm

Admin pages fixed

So, my URL flyweight change had some unexpected problems. Basically, I switched a normal object method into a static one. Any problems with object access like $url->getUrl() should throw an error, right? I should be able to find and fix any old method access. Not so in PHP.

class Foo {
  public static function bar() {
      echo "foo\n";
  }
}
class Zap extends Foo {
  public static function bar() {
      parent::bar();
  }
}
$z = new Zap();
$z->bar();

In PHP, the parent:: token is ambiguous. It works equally well in static and object calling contexts. Basically, it passes along the calling context to the next function up. If you had declared bar() previously as non-static, you wouldn't notice any errors in this setting. But the behavior of calling Zap::bar() would change drastically.

The point is, if you're changing a method from non-static to static, you need to be aware of the internals of all the functions which subclass it. If they call parent::bar(), the result could be unexpected. In the above example, $z would not be modified at all, and no errors, warnings, nor E_STRICTS would be thrown.

Email Templating

I'm trying to work through some transactional e-mail templating issues. I'm trying to figure out where the "items_html" gets created. In the database I can see

{{var items_html}}

but, I can't find any reference to get/set ItemsHtml() in the code. What I find in the layout XML isn't very promising.

So, I setup a small bootstrap script to get ready to send these emails to myself and then I can get into the code and dissect what's going on.

chdir('../../../../../');
require_once 'app/Mage.php';
umask(0);
Mage::app('default');


$mailTemplate = Mage::getModel('core/email_template');
$recipient = array();
$recipient['email'] = 'me@example.com';
$recipient['name'] = 'me';

$order = Mage::getModel('sales/order')->load(4);

$mailTemplate->setDesignConfig(array('area'=>'frontend', 'store'=>0))
->sendTransactional(
    'sales_email_order_template',
    array('name'=>'me', 'email'=>'me@example.com'),
    $recipient['email'],
    $recipient['name'],
    array(
        'order'         => $order,
        'billing'       => $order->getBillingAddress(),
        'payment_html'  => '
payment html
',
    )
);

It looks okay, I should be able to at least trigger the transactional emails and trace through the code with var_dump() until I can find where items_html is set or used.

There's only one problem. The $order relies on using blocks to format its own "createdat" property, and the blocks don't always work unless you have a front controller and an action. Why? ... because the output templates are tied into the request action.. ?

// from Sales/Model/Order.php
public function getCreatedAtFormated($format)
{
    return Mage::getBlockSingleton('core/text')->formatDate($this->getCreatedAt(), $format);
}


//from Mage.php
public static function getBlockSingleton($type)
{
    $action = Mage::app()->getFrontController()->getAction();
    return $action ? $action->getLayout()->getBlockSingleton($type) : false;
}

Why, oh why, can't I just send a transactional email without having to simulate an entire request?

Item Options

Recently I was trying to debug some MadetoOrder (MTO Homepage) code and found a really odd behavior of the order item (Mage_Sales_Model_Order_Item). I was in the middle of working on a function which prints out special order options and found that

the functions you need to call to get the options ordered are dependent on whether or not the order is placed in the database or still in the customer's session.

//sometimes you call:
$item->getOptionsById()
//sometimes you call:
$item->getProductOptionsById()

//so you always have to do:
$buyRequest = $item->getProductOptionsById('info_buyRequest');
if ($buyRequest === NULL) {
$buyRequest = $item->getOptionsById('info_buyRequest');
}
if ($buyRequest === NULL) {
//do real error handling here, order item
//really doesn't have a buy request.
}

Which do you call when? It doesn't really matter, you have to do both all the time in one function and check for null, then call the other one. I was so horrified by my finding that I didn't bother checking which one was for the placed order and which ones was for the session.

So, if you want to change the way a product looks or deals with options, and you want your change to appear the same way in the "Your Cart" page as it does in the "Your Order History" page... you gotta use both for no apparent reason.

Don't believe me? Try it. How does Magento deal with this? Well, they don't really re-use code, so it doesn't affect them. The block for the shopping cart contains functions which are different than the block for the my account page.

How not to use func_get_args()

  public function __construct()
  {
      $args = func_get_args();
      if (empty($args[0])) {
          $args[0] = array();
      }
      $this->_data = $args[0];
      $this->_construct();
  }

instead, do...

  public function __construct($data = array())
  {
      $this->_data = $data;
      $this->_construct();
  }

Flyweight Pattern

If anyone was wondering what I meant when I said I wanted to do ideological change to Magento - this is one of them:

http://github.com/markkimsal/agent-ohm/commit/0c2e5d0885d1c45e1ba847950c8636aca75f9d95

I simply changed the URL to use the flyweight pattern, thus eliminating the repeated conversion of the code "core/url" to Mage_Core_Model_Url.

The name "Mage_Core_Model_Url" is just as flexible and just as inflexible as using the same token everywhere, namely "core/url". Some of you might think that just because the class's name is "hard-coded" into the source that it must only deal with an instance of itself, but that just isn't true. In the same way that Mage::getModel() dishes out various instances of different classes, any class in Magento can operate as a "master of its domain", doling out various configurable instances.

What good is it to spread out the workload of converting tokens to class names, you ask? Well, it tends to avoid recursive loops, it's not guaranteed, but it can make needless recursion easier to spot. It's also easier to benchmark parts of the code if the responsibility for creating every object isn't centralized.

But, this change isn't just about spreading out responsibility or patterns. It's a good, efficient way to avoid repetitive class name look-ups and costly object instantiation for an often used object. The use of "clone" copies an existing object, and if that object's constructor is complicated that can save on total execution of the page.

Change to the view layer

One thing that really frustrates me about all the PHP frameworks out there is that they force you to run the view functions yourself. It's not so much MVC as it is "DIYC".

100% of the time you will want to send output to the browser. Hyperbolically speaking, 99% of the time you will want to send the browser a Web page. Now, if you take the approach that your "View" layer (in MVC) is just a templating library, then I can see how you would want to separate out sending random HTTP headers to the browser from your templating system. But, if your concept of a "View" layer handles all the output to the browser (or whatever client happens to be requesting your service) then it is silly to force each controller action to specify and call the output function.

In Magento you can see this code repeated all over the place:

 $this->loadLayout();
 $this->renderLayout();

A quick grep reveals this bit of code repeated 230 times in the Magento source. But this sort of thing also happens in Code Igniter with the code $this->template().

If the actions or events get called for you automatically by the front controller, why not the template too? Having your output functions called automatically saves repetitive coding and forces a clearer separation between business logic and display logic. In addition to that, it reduces the function stack size by one.

So, I have moved the loadLayout and renderLayout responsibilities up, out of the controller actions to a higher level. For the rare occasion when you need to redirect or show a different output, you can specify a custom output handler with $this->outputHandler = 'myfunction'. If this flag in the controller actions is left alone, then the standard loadLayout() renderLayout() pair is called for you.

Easy Cache Prizes

I made a simple change to the caching system for an easy 1% win. It turns out that all the Zend currency and locale libraries need to cache a lot of data. As I wrote before, Magento lumps in all cached data into one cache repository, making it more difficult to use the tag/attribute system, and generally find what you're looking for in the cache. By simply creating a new _localecache attribute of the App model, and passing it only to Zend Locale/Currency objects, we can get a quick speed up.

Separating out each major cache component into its own cache repository (directory) is the way to go. I just didn't expect it to be this easy.

Cache Fixed, Results Mixed

I finished my "newrouter" branch today. The idea was to clean up the way that stores, websites, and routers interact with the cache. Basically, everything in the XML tag gets copied around under and tags as a way to allow someone to change default settings, but still keep custom changes per store. Well, the result is a horribly bloated XML file, about 4 times larger than it needs to be, because so many modules put everything into "default". Things not related to stores at all.

So, I ended up cutting out that copying of XML and replacing it with a block of code that simply checks the default branch if a specific tag doesn't sit under a store or website. Pretty simple. It cuts the size of the cached config file way down, but it doesn't reduce the peak memory usage much. That must be in the database where peak memory is used.

The trade off comes from having to constantly look up config settings in two places. This should have given a 5-7% boost, but in the end, only about 2-3% boost because of the increased processing. Still though, a boost is a boost. It will probably speed up hits which rebuild the cache too. I haven't stress tested anything thoroughly. All the tests are done on the front page of a store with sample data. Mind you, this is 2-3% on top of the already 20-25% over Mage 1.3.0.

Blocks everywhere but not a drop to build with.

I really don't like the blocks system in Magento. That should have been obvious from the book. The .phtml files are tightly coupled to the PHP class file which is a Block, yet the two are considerably "far apart" in the file system layout. I'm always a fan of things which are tightly coupled and work in concert being "near" each other. And, by "near" I mean that you shouldn't have 5 levels of directory separation between the two. It serves no purpose except to make people think they are very different, independent items when they are not.

So, mentally, Blocks are associated with a "module", when in reality they are simply wrappers around random .phtml scripts and should be more considered to be pure template helpers (not Mage Helpers with a capital H, but I'll come back around to this). I say that blocks are mentally associated with a module because they "live" in a module directory, so people think of them as belonging to that module. In reality, most blocks are simply facades that create a thin (but expensive) layer between the PHP scripting code in the templates and the rest of the Models.

The majority of the time, a Block is instantiated when there is no reason to have an actual instance holding the script execution. Very few times will you actually see "$this" referenced inside a Block definition. When you do, it's usually just another utility function like $this->getProductId(), which could easily be handled by a simple template array or registry pattern. So, most blocks can be transliterated into static code with very few changes, and what do we know in Magento which is usually a static class...? Helpers.

I think that 99% of all blocks could be rewritten as helpers, thus reducing the brittle nature of the relationship between .phtml files and their parent blocks. I've seen some templates be destroyed by a simple function name in a Block changing from getItem to getItems. Granted, making functions static doesn't make them impervious to name changes, but it does allow the end-developer to easily make a quick fix. It is much easier to override a static utility class which has virtually no hierarchy than an instance class which is at least 3 levels sub-classed.

So, the whole point of this post is that I started a git branch with NO BLOCKS. (Well, 3 blocks actually) And, I've seen no less than a 50% speed-up in the code (which was already 25% sped up) and a reduction in memory usage around 50%.

Take this with a grain of salt because a lot of the functionality is missing, I only kept the following blocks CMS/Page Core/Template Core/Text_List and some of the HTML page ones because I couldn't untangle them. The front page shows up a little messed up, but all the functionality is there. All the login/logout links, footer, header, products, call-outs. And that page shows 50% speed improvements. Before I had sample data installed, my front page was around 85-90 milliseconds, after installing sample data it dropped to around 120 mills, with the no blocks branch it's around 60-70 mills. Magento 1.3 clocks in around 150 mills.

She blinded me with design-ence

I'm absolutely floored at why people continue to use Magento. It has to be the nice looks. For almost a year there are catalog pricing rule bugs. If you set a special price for some products, it will revert to the normal catalog price in 2 days. There was a bug in which a customer could wipe out your entire product database simply by clearing their wishlist. I've heard stories about tax rounding being incorrect... Magento actually does nothing required by a shopping cart correctly, yet people still flock to it.

Let this be a lesson to all open source projects. Spend $500-$1000 on a designer and nobody will pay attention to your bugs.

A different approach

After looking at some of the template code (block system in general and template parameter filtering (it's an entire parsing engine written in PHP)), I'm starting to question the possibility of just making Magento faster. To me, it is clear that the entire thing needs a rewrite. Maybe a rewrite can leverage some of the lessons learned from the original Magento, maybe not.

So, I had an idea to slowly replace the front end of magento by wrapping it in a framework, and replacing each page with better, more optimized code while allowing non-optimized pages to fall back into original Magento. This would instantly eliminate the PHP5 cruft which saddles each magento request with 50% more work than required to render a page of that complexity.

Once each piece is replaced, the original code base is no longer needed. The problem with this is, it's not optimizing Magento... it's just replacing Magento. But, keeping the DB tables - which some might argue is the worst part of Magento with it's heavy EAV usage throughout.

Something for everyone

We all like fast ... stuff, right?

I found some old "secret" optimizations for Magento recently. I had this crazy idea that people would pay money for Magento "tune-ups" on their servers... but that's another story. I found four SQL index additions. After analyzing the Magento DB from way back when, I found 4 semi-crucial indexes missing, two of them have made it into M 1.3.0 already. Here are the other two:


ALTER TABLE `poll` ADD INDEX ( `closed` )

ALTER TABLE `catalog_category_entity` ADD INDEX ( `path` )

I don't know if these are even relevant anymore.

Sample Data

So, I've decided to install the sample data to see what kind of speed improvements can be made to a more "live" system. Ahh.. the Magento sample data.

C'mon Varien. It's been a year, can we seriously not have sample data that's not in a format which requires a complete re-install?!

If you have configured your system with table prefixes, you cannot install the sample data. Since it's just a database dump of their system it will reset your permissions store names, etc. etc. It's not really sample products... it's really a starter system.

So, if you've configured with DB table prefixes:
edit app/etc/local.xml and delete the prefix you used
mysql drop database `your_db`
mysql create dtabase `your_db`
mysql < magento-sample-data.sql
rm -Rf var/cache/* (since it cached the table prefix)
hit any URL

Not having a packet of sample data makes me worried that I won't be able to work with my product data once I spend a lot of time getting it in. Is a SQL dump the best way to make a backup of your product data? Is it the only way that doesn't lose features? I'm unsure at this point, but I can never get over how people call Magento "enterprise" and "full featured", but yet you can't get some sample products without wiping any installation you've already done.

SimpleXMLElements

One of the things that was stopping Magento from running on Quercus and other non-offical PHP runtimes was the fact that it used custom SimpleXMLElement sub-classes. A not often used feature of the simplexml library. While it's quite handy to add your own behaviors onto each element, it's not necesarry either. I've moved all the element methods on to the base config classes, thus reducing overall memory and hopefully allowing agent-ohm to run in places that Magento won't.

Working on the cache

The config and cache are probably the two worst offenders in magento.

The cache can actually slow down your site because ZF doesn't keep an index of tags on your cached data. If you specify that you want to clean up all "shopping cart" tagged data, ZF cache has to search every single metadata file and look for the tag... resulting in hundreds or thousands of fopen, fread, fclose calls on your system.

If you try to use one of the memory cache backends, like memcache or APC you'll find that they perform *worse* than the file cache. Magento's concept of caching is throwing thousands of tiny little objects into individual files creates huge fragmentation in the memory and actually slows the site down.

The entire way the cache works needs to be thrown out and started over. It should be federated into different directories for config data, template data and all the other top level cache controls currently available. I'm kind of scared to load two or more instances of Zend_Cache_Core in one request. Savvy ZF people will say, "Oh, but it has a nesting depth setting.", not what I'm talking about. The next thing that needs to change: only large chunks of data need to be stored in a cache. Tiny pieces of template data which take little time to compute should not be kept in the cache. Entire pages, on the other hand, could be saved in the cache, minus the customer cart side bar and login/logout links.

Fixing getDesign Bugs

I change the way Mage::getDesign functions. It simply wraps up a call to Mage::getSingleton('core/design_package'). But, I found that not all of the code was updated, some spots still called the getSingleton() method. Why would you wrap up a function like that but not search and replace for all occurances?

After I found the problem it was pretty easy to find the 3 other spots and fix them. But it raises the question about how people are doing refactoring in the Magento core. It's not with search and replace, it's not with any IDE refactor plugin, because both of those methods would have caught the other spots. It must be laissez-faire refactoring.

getModel update

Update to the getModel conundrum. I simply changed the Db collection class to use "createNewItem()" instead of "createNewEmptyItem()" and I'm passing in the database table row as one parameter. createNewItem is part of just the Db collection class, not the abstract, anything-goes collection class.

This addition, plus changing the way stores are initialized in the App object have allowed me to define the store class a little bit better. It's moving away from a flabby cloud of getters and setters into a concrete, testable powerhouse!

Models Models, who's got the Models

This all started with the Store model... Mage_Core_Model_Store

Magento has this nifty feature of handling multiple stores on one installation, all with their own configuration. So, naturally you'd think that the store object has access to its own config object. Well, no. It's peppered with calls to $this->getConfig($key) which in turn, calls Mage::getConfig()... the global config object. So, each time it calls it has to descend down the XML node structure 3 levels just to find its own store config, the nodes go like this
global / stores / $storeId

It would be nice to just have a subset of that node plugged onto the object of quick config lookups.

Normally, this would be simple, like pass the global Config object to the constructor of Store and find it's sub path once, then store a reference to it. But, Magento doesn't like constructors, they like the lazy getters instead.

But, Mage does have a 'getModel' method which will lookup the exact class name based on a configruable code and give you and instance... you can even pass an array of parameters to the constructor... cool.

The real problem - which is the whole point of this post - begins here, with getModel()

The App class initializes all the configured stores each request with _initStores(). It loads all stores with a database "Collection". The collection is a special type of itterable object which deals with mutliple objects mapped to database records at a time. Here's the code


$storeCollection = Mage::getModel('core/store')->getCollection()
->initCache($this->getCache(), 'app', array(Mage_Core_Model_Store::CACHE_TAG))
->setLoadDefault(true);

The problem here is two fold. Collections don't pass any arguments to your class's constructor and you need to create an empty object just to get access to the collection object. Mage::getModel('core/store') creates a blank store object, then you call getCollection() which returns Mysql4_Store_Collection object - a subclass of the special Collection object.

Collections are part of the lib/Varien/* family of libraries. They seem to be divorced from knowing anything about the Magento code. But Mage::getModel() is definitely a Magento native function, and it behaves completely differently than the object instantiation of the lib/Varien/Collection class. But they both make the same objects.

There are two ways around this, one is to connect the data collections back into the getModel() smartness of passing arguments. This seems to me like OOP spaghetti code though, as the data loading function is so far down in the collection library that it doesn't know anything about the required parameters. getModel() also doesn't know anything about the parameters, but it will pass any along that you give it.

What's really needed is one (1) consistent way of creating model objects which understands - globally - required constructor parameters. Without this sort of solution we're stuck in a constructor-less mess of getters and setters with no defined entry points to code in the Models.

Thinking about phemto... more later

Lazy getters

I've never really seen this style of programming used much outside of Magento and ZendFramework, so I don't know what to call it. I decided on "lazy getters" as a good enough term.

Lazy getters consist of a getter function that checks the item which it's going to get and initializes it if it's not set. Wait... that sounds normal you say? ... almost clever? Well, sounding clever is a lot different than actually being clever.

   /**
    * Initialize and retrieve application
    *
    * @param   string $code
    * @param   string $type
    * @param   string|array $options
    * @return  Mage_Core_Model_App
    */
   public static function app($code = '', $type = 'store', $options=array())
   {
       if (null === self::$_app) {
           self::$_app = new Mage_Core_Model_App();

           Mage::setRoot();
           Mage::register('events', new Varien_Event_Collection());
           Mage::register('config', new Mage_Core_Model_Config());

           self::$_app->init($code, $type, $options);
           self::$_app->loadAreaPart(..., ...);
       }
       return self::$_app;
   }

As you can see, this function "app" simply returns a reference to self::$_app, unless $_app is not initialized. Then it uses the parameters passed into it to construct an app and do some initialization. Even the code comments say "initializes and retrieve an application".

When this style of get is used throughout your entire code base you end up with 2 problems:

1. You never know where/when something gets initialized, since everything is lazily loaded. It also makes it hard to search for the initializing portion of code since both inits and gets use the same function name.

2. It's slow. You might not think so, but using lazy getters all over your code guarantees that you cannot access any property other than with a getter, plus the checks for validity are always done each call to "get". The engine has to accommodate for all those optional parameters and create memory for them just to return self::$_app.

Number 2 probably doesn't seem like a big, but when that style infests your entire application, it's probably adding 10% overhead without any benefit. In fact, it's taking away clarity from the code a la problem number 2.

The forking of Magento

In the past year, Magento has gotten faster and faster with each release. But, I still feel there are fundamental problems with the code which stops it from running as fast as a PHP shopping cart can run. Varien's closed development model has led me to believe that forking Magento is the only way to really change Magento for the better. I have tried patching Magento, but there is not a formalized process for discussion the code, nor submitting patches.

I have started with a base of Magento 1.3.0, I have done some quick file name changes before importing into git. Last night I started some simple speed improvements and got to around 4-5% increase before deciding that I should stop and blog about every change.


==Update==
Links are
Code: http://github.com/markkimsal/agent-ohm/tree/master
News, Issues, Discussion: http://code.google.com/p/agent-ohm/

Dependency Injection

I was thinking about adding DI support to agent-ohm instead of the getModel('somecode') registry style pattern. The problem is, it's a lot of call_user_func_array() calls for every new instance.

It's made me realize that the real problem is allowing any class to be overridden at any time. Allowing this level of functionality complicates every decision and it has to go. There is a balance between overriding objects and forcing the developer to re-implement logic. Neither of them are particularly good solutions to the problem of "I want to change this behavior."

Overriding any class file in the system seems nice at first, but what happens when you have two 3rd party modules both wanting to sub-class and override the core "Product" class? You get a conflict. It's the same problem whether you use DI or config files to determine the actual class name to instantiate at run time. At it's heart, dependency injection is a 1-to-1 mapping, when really you need a 1-to-many plugin system to allow mutliple customizations to the same logic.

DI might be good in some instances, for allowing new product types to be loaded, or selecting custom shipping routines. Perhaps DI could/should be used as a part of a chain of command pattern to allow dynamic plugins to be added to any chain.

Either style of DI should be used judiciously - too much flexibility and the code base becomes flabby and undefined.

"If everything is permissible [and] nothing offers me any resistance, then any effort is inconceivable [and] every undertaking is futile." ~Igor Stravinsky