1

I have the following collection that helps me implement a custom report:

class VMR_Customreports_Model_Resource_Productsordered_Collection extends Mage_Reports_Model_Resource_Order_Collection
{
public function __construct()
{
    parent::__construct();
    $this->_init('sales/order_item');
}

public function setDateRange($from, $to)
{
    $this->_reset();
    $this->getSelect()
        ->joinInner(array('OI' => $this->getTable('sales/order_item')), 'OI.order_id = main_table.entity_id', NULL) //AND i.store_id = main_table.store_id
        ->where("main_table.created_at BETWEEN '".$from."' AND '".$to."'")
        ->where('main_table.status != \'canceled\'')
        ->where('main_table.customer_email NOT LIKE \'%@v2cigs.%\'')
        ->where('OI.parent_item_id IS NULL')
        ->columns(array(
            'myname' => 'OI.name', 
            'ordered_qty' => 'sum(OI.qty_ordered)', 
            'ordered_money' => 'sum(OI.price*OI.qty_ordered)'))
        ->group('OI.name');

    return $this;
}

public function setStoreIds($storeIds)
{
    if ($storeIds)
        $this->addAttributeToFilter('main_table.store_id', array('in' => (array)$storeIds));
    return $this;
}

}

The above actually appears to work AFAICT yet I dont understand why I have to include the constructor; In fact I would have expected that contstructor's _init() call to break my collection because I'm assuming that main_table here is sales/order not sales/order_item and AFAIK that _init() call mainly sets $this->_model to what I tell it to. When I don't include it I get, 'Item (Nostress_Pdfprintouts_Model_Order) with the same id "855" already exist' which doesn't make much sense to me. Can someone tell me why I would get the above error and why my _init() call would fix it?

Edit:

One Clue: When I don't include the constructor and I log my sql queries it gags on:

SELECT `main_table`.*, `OI`.`name` AS `myname`, sum(OI.qty_ordered) AS `ordered_qty`, sum(OI.price*OI.qty_ordered) AS `ordered_money` FROM `sales_flat_order` AS `main_table`
 INNER JOIN `sales_flat_order_item` AS `OI` ON OI.order_id = main_table.entity_id WHERE (main_table.created_at BETWEEN '2013-06-03 08:00:00' AND '2013-06-04 07:59:59') AND (main_table.status != 'canceled') AND (main_table.customer_email NOT LIKE '%@v2cigs.%') AND (OI.parent_item_id IS NULL) AND (main_table.store_id IN(12, 4, 3)) AND (main_table.store_id IN('12', '11', '13', '14', '4', '3')) GROUP BY `OI`.`name`

where the 3 results are all from that 2013/6/3 period grouped by name and 2 of the results share the same main_table.entity_id namely, 855. So order 855 has 2 invoices, and if I don't include that _init() it complains about that entity_id not being unique.

FYI: The models section of my config.xml

My config.xml's models section FYI:

....
    <models>
        <enterprise_admingws>
            <rewrite>
                <collections>VMR_Customreports_Model_Admingwscollections</collections>
            </rewrite>
        </enterprise_admingws>
        <customreports>
            <class>VMR_Customreports_Model</class>
            <resourceModel>orders_collection</resourceModel>
            <resourceModel>productsordered_collection</resourceModel>
            <resourceModel>ordersbypaymenttype_collection</resourceModel>
            <resourceModel>ordersbyshippingmethod_collection</resourceModel>
            <resourceModel>accounting_collection</resourceModel>
        </customreports>
        <orders_collection>
            <class>VMR_Customreports_Model_Resource</class>
        </orders_collection>
        <productsordered_collection>
            <class>VMR_Customreports_Model_Resource</class>
        </productsordered_collection>
        <ordersbypaymenttype_collection>
            <class>VMR_Customreports_Model_Resource</class>
        </ordersbypaymenttype_collection>
        <ordersbyshippingmethod_collection>
            <class>VMR_Customreports_Model_Resource</class>
        </ordersbyshippingmethod_collection>
        <accounting_collection>
            <class>VMR_Customreports_Model_Resource</class>
        </accounting_collection>
    </models>
.......
3

The reason you're getting that error is the collection's query returns two rows with the same ID, and Magento collections are programmed to only allow one unique ID for each item

#File: lib/Varien/Data/Collection.php
public function addItem(Varien_Object $item)
{
    $itemId = $this->_getItemId($item);

    if (!is_null($itemId)) {
        if (isset($this->_items[$itemId])) {
            throw new Exception('Item ('.get_class($item).') with the same id "'.$item->getId().'" already exist');
        }
        $this->_items[$itemId] = $item;
    } else {
        $this->_addItem($item);
    }
    return $this;
}

As for the why, you're extending Mage_Reports_Model_Resource_Order_Collection which already extends the Mage_Sales_Model_Resource_Order_Collection class. This later class initialized the collection with the class alias sales/order.

#File: app/code/core/Mage/Sales/Model/Resource/Order/Collection.php
protected function _construct()
{
    $this->_init('sales/order');
    $this
        ->addFilterToMap('entity_id', 'main_table.entity_id')
        ->addFilterToMap('customer_id', 'main_table.customer_id')
        ->addFilterToMap('quote_address_id', 'main_table.quote_address_id');
}

If you look at what the _init method actually does for a collection

#File: app/code/core/Mage/Core/Model/Resource/Db/Collection/Abstract.php
protected function _init($model, $resourceModel = null)
{
    $this->setModel($model);
    if (is_null($resourceModel)) {
        $resourceModel = $model;
    }
    $this->setResourceModel($resourceModel);
    return $this;
}

It sets the model and resource model. Why is this important? Because the abstract db collection that most collections inherit from calls an _initSelect method at the end of its __construct method (which is also after _constrct has been called).

#File: app/code/core/Mage/Core/Model/Resource/Db/Collection/Abstract.php
public function __construct($resource = null)
{
    parent::__construct();
    $this->_construct();
    $this->_resource = $resource;
    $this->setConnection($this->getResource()->getReadConnection());
    $this->_initSelect();
}

The _initSelect method sets the main table of the collection's select query.

#File: app/code/core/Mage/Core/Model/Resource/Db/Collection/Abstract.php
protected function _initSelect()
{
    $this->getSelect()->from(array('main_table' => $this->getMainTable()));
    return $this;
}

And getMainTable works by asking the resource model for a table name,

public function getMainTable()
{
    if ($this->_mainTable === null) {
        $this->setMainTable($this->getResource()->getMainTable());
    }

    return $this->_mainTable;
}
...
public function getResource()
{
    if (empty($this->_resource)) {
        $this->_resource = Mage::getResourceModel($this->getResourceModelName());
    }
    return $this->_resource;
}

So, all that happens when the constructor is called, irrespective of whether you call it explicitly or not.

Now, in your code, after calling the constructor, you recall _init

public function __construct()
{
    parent::__construct();
    $this->_init('sales/order_item');
}

As a reminder, here's what _init looks liks

#File: app/code/core/Mage/Core/Model/Resource/Db/Collection/Abstract.php
protected function _init($model, $resourceModel = null)
{
    $this->setModel($model);
    if (is_null($resourceModel)) {
        $resourceModel = $model;
    }
    $this->setResourceModel($resourceModel);
    return $this;
}

So, you're changing the model and (more importantly) the resource model to use sales/order_item. However, the query's main_table was still set with the sales/order resource model derived table name.

This means in your custom collection, post __construct, anytime the Magento code calls

$this->getMainTable();

It will get the main table for the sales/order_item resource model. With your extra _init in the constructor, calls to getMainTable would have returned the main table for the sales/order resource model. Any other query generating code that relies on resource model behavior would also act differently.

Without tracing the code further, I'm sure you can see how this creates the opportunity for subtle but significant differences in the generated queries.

Re: the question in the comments.

No, it's not a sensible thing to do. The collection system in Magento is designed to load an array of models which have one main table, and several optional support tables (either via joins or extra selects). When you call _init a second time in the PHP __construct method, you're creating a collection with two main tables, and while that might allow you (or your tutorial writer) a way to quickly and cleverly implement a feature, it seems bound to cause confusion, and possible fall afoul of a future update.

Also, you're already heading into not-sensible territory since you're extending a class which already extends a concrete Magento collection. While this is standard OOP, it does fall outside general usage patterns, which means you're on your own when it comes to tracking down weird bugs or behavior you don't understand.

Like anything though, it's all about time/benefit ratios and tradeoffs. Sensible for one team isn't sensible for a different team.

2
  • Thank you for summarizing all that in text. I have attempted to trace what the code is doing and it gets complicated as you show, but one thing that would help is knowing whether having my own constructor with _init() is generally a sensible thing to do or not for my case. I swear I found it in a tutorial or something.
    – user2045
    Jun 20 '13 at 17:56
  • 1
    @user2045 Update the answer at the bottom. Short version: No, but it's complicated and subjective.
    – Alan Storm
    Jun 20 '13 at 19:23
1

I believe the issue here is that your construct is the actual object construct (public), and not Magento's protected construct which is fired after the PHP class constructor:

Mage_Sales_Model_Resource_Order_Item_Collection:

/**
 * Model initialization
 *
 */
protected function _construct()
{
    $this->_init('sales/order_item');
}

Notice that it's not a double-underscore, and that it's protected, not public. You should mimic this in your own class, but use your own model shorthand:

protected function _construct()
{
    $this->_init('vmr/productsordered');
}

Or a similar path to your class.

8
  • Well, fyi I have other similar reports that dont need any constructor whatsoever. Why does this one?
    – user2045
    Jun 20 '13 at 3:16
  • Also, I tried your constructor and it doesn't work, fwiw
    – user2045
    Jun 20 '13 at 8:16
  • What is the full path to your model
    – philwinkle
    Jun 20 '13 at 11:54
  • VMR_Customreports_Model_Resource_Productsordered_Collection
    – user2045
    Jun 20 '13 at 13:19
  • I think you meant $this->_init('customreports/productsordered') but I tried that and it didnt work either.
    – user2045
    Jun 20 '13 at 13:24

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.