14

This is a question regarding a good programming practices in Magento.

I need to show (in the category product list) the product with its related products in thumbnails. So I edited mypackage/mytheme/template/catalog/product/list.phtml with something like this

<?php 
    $related=$_product->getRelatedProductIds();
    if(count($related)>0){
        echo '<div class="a'.$ap.'"></div>';
        echo '<div class="li_p"><ul>';
        foreach($related as $rela){
            $rela_nom=Mage::getModel('catalog/product')->load($rela);
            echo '<li><a href="'.$rela_nom->getProductUrl().'"> <img src="'.$this->helper('catalog/image')->init($rela_nom, 'small_image')->resize(20).'" width="20" height="20"> </a><li>';
        }
        echo '</ul></div>';
    }
?>

And it works very well.

But my question is: Is this correct to instantiate a model class on the phtml files?

If is not, what would be the best way to achieve this functionality? I mean, what file is better to edit or what class is better to add, where? A helper?

Can you provide a little example or give me a glance of what files are better to edit.

10

I want to disagree with Sonassi's answer :)

Initiating a model in the template is bad practice. Sometimes it is needed and sometimes I do it too. But if it is possible you should prevent to add code to your pHTML files and only echo given things.

It is Separation of concerns. Don't mix up html and coding stuff. This should be in the Block class.

  • 3
    I also agree with your disagreement :) But loading a single model outside of a loop isn't the end of the world. Otherwise it becomes a case of abstraction infinitum - adding additional classes existing merely for separation of a single line of code from the view. It just adds to rewrite overheads, not to mention maintenance. – Ben Lessani - Sonassi Feb 22 '13 at 18:10
  • You have too mich time if you want to correct all my spelling mistakes, thanks for that :-) – Fabian Blechschmidt Feb 22 '13 at 22:53
  • Btw, you are right sonassi :-) It is just something we should be careful with. I saw SQL querys in phtml-files... NO NO :-) – Fabian Blechschmidt Feb 25 '13 at 10:17
4

There is nothing wrong with loading a model in a phtml file. But it depends on why you are doing it.

If you need the entire model and all the data associated with it, then you may as well load the whole model.

But if you just need the Product URL (from your example), then you could just load the correct collection

$_product->getRelatedProductCollection();

Then iterate through that as necessary

<?php $_relatedCollection = $_product->getRelatedProductCollection(); ?>
<?php foreach ($_relatedCollection as $_item): ?>
<li>
  <a href="<?php echo $_item->getProductUrl(); ?>">
    <img src="<?php echo $this->helper('catalog/image')->init($_item, 'small_image')->resize(20); ?>" width="20" height="20">
  </a>
<li>
<?php endforeach; ?>
3

Want to put my 5 cents here. We should respect architecture principes which are used in Magento. Main architectural pattern used in Magento is MVC. In case of Magento "View" part contains of several things (block, template, layout). Blocks were created to move out data preparation logic from template to some other class to make templates more cleaner and readable for frontend developers. Here i want to agree with Fabian.

As for Sonassi's concerns about too many unneeded classes i suggest to look forward to push based MVC. In this case we look on controller as commander who defines which block and what data should have. Action in controller could contain required code to load data and put it into block (via magic setters) before rendering.

3

I agree with Fabian Blechschmidt that it is bad practice and you should respect Separation of Concerns.

To add a constructive suggestion:

This is something, the Block classes are meant for. In your case you would have to rewrite Mage_Catalog_Block_Product_List to add the desired functionality:

public function hasRelatedProducts()
{
    return count($this->getRelatedProductIds()) > 0;
}
public function getRelatedProducts()
{
    $products = array();
    foreach ($this->getRelatedProductIds() as $id) {
        $products[] = Mage::getModel('catalog/product')->load($id);
    }
    return $products;
}

It should be obvious how to use these methods in the template.

Note: Rewrite does not mean to edit the core file. Follow the customizing tutorial if you don't know how to rewrite a block.

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.