1

I've had to re-write a block of a third party module, I only want to re-write one function in the class but my re-write won't work unless I copy across the private functions from the class I extended.

I read this answer from Alan Storm on stack overflow and read that if your class extends another class, it will not have access to private properties of the base class/object.

That's ok, but what is the best practice for class re-writes if the base class had pricate functions? Should I just add them to my class. What about protected properties?

Here is my class if it helps.

class Creed_CustomMenu_Block_Navigation extends WP_CustomMenu_Block_Navigation
{
        const CUSTOM_BLOCK_TEMPLATE = "wp_custom_menu_%d";

    private $_productsCount = null;

    public function showHomeLink()
    {
        return Mage::getStoreConfig('custom_menu/general/show_home_link');
    }

    public function drawCustomMenuItem($category, $level = 0, $last = false)
    {
        if (!$category->getIsActive()) return '';
        $html = array();
        $id = $category->getId();
        // --- Static Block ---
        $blockId = sprintf(self::CUSTOM_BLOCK_TEMPLATE, $id); // --- static block key
        #Mage::log($blockId);
        $collection = Mage::getModel('cms/block')->getCollection()
            ->addFieldToFilter('identifier', array('like' => $blockId . '%'))
            ->addFieldToFilter('is_active', 1);
        $blockId = $collection->getFirstItem()->getIdentifier();
        #Mage::log($blockId);
        $blockHtml = $this->getLayout()->createBlock('cms/block')->setBlockId($blockId)->toHtml();
        // --- Sub Categories ---
        $activeChildren = $this->_getActiveChildren($category, $level);
        // --- class for active category ---
        $active = ''; if ($this->isCategoryActive($category)) $active = ' act';
        // --- Popup functions for show ---
        // $wideBlock; // = false;  // MC: added new condition for setting extra wide static blocks
        global $wideBlock;
        $wideBlock = false;
        if (strpos($blockId,'wide') !== false) {
            $wideBlock = true;
        }
        $drawPopup = ($blockHtml || count($activeChildren));
        if ($drawPopup) {
            $html[] = '<div id="menu' . $id . '" class="menu' . $active . '" onmouseover="wpShowMenuPopup(this, event, \'popup' . $id . '\');" onmouseout="wpHideMenuPopup(this, event, \'popup' . $id . '\', \'menu' . $id . '\')">';
        } else {
            $html[] = '<div id="menu' . $id . '" class="menu' . $active . '">';
        }
        // --- Top Menu Item ---
        $html[] = '<div class="parentMenu">';
        if ($level == 0 && $drawPopup) {
            $html[] = '<a href="javascript:void(0);" rel="'.$this->getCategoryUrl($category).'">';
        } else {
            $html[] = '<a href="'.$this->getCategoryUrl($category).'">';
        }
        $name = $this->escapeHtml($category->getName());
        if (Mage::getStoreConfig('custom_menu/general/non_breaking_space')) {
            $name = str_replace(' ', '&nbsp;', $name);
        }
        $html[] = '<span>' . $name . '</span>';
        $html[] = '</a>';
        $html[] = '</div>';
        $html[] = '</div>';
        // --- Add Popup block (hidden) ---
        if ($drawPopup) {
            // --- Popup function for hide ---
            $html[] = '<div id="popup' . $id . '" class="wp-custom-menu-popup" onmouseout="wpHideMenuPopup(this, event, \'popup' . $id . '\', \'menu' . $id . '\')" onmouseover="wpPopupOver(this, event, \'popup' . $id . '\', \'menu' . $id . '\')">';
            // --- draw Sub Categories ---
            if ($blockHtml)
            {
                if (count($activeChildren))
                {
                    if ($wideBlock) {
                        $html[] = '<div class="block1 grid12-2 mega-menu-list">';
                        $html[] = '<h3 class="title cat-browse">' . Mage::helper('catalog')->__('Browse by category') . '</h3>' ;
                        $html[] = $this->drawWithoutColumns($activeChildren);
                    } else{
                        $html[] = '<div class="block1 grid12-7">';
                        $html[] = $this->drawColumns($activeChildren);
                    }
                    $html[] = '<div class="clearBoth"></div>';
                    $html[] = '</div>';
                }

                // --- draw Custom User Block ---
                if ($wideBlock) {
                    $html[] = '<div id="' . $blockId . '" class="block2 grid12-10">';
                } else{
                    $html[] = '<div id="' . $blockId . '" class="block2 grid12-5">';
                }
                $html[] = $blockHtml;
                $html[] = '</div>';

            }
            else
                if (count($activeChildren)) {
                    $html[] = '<div class="block1 grid-full">';
                    $html[] = $this->drawColumns($activeChildren);
                    $html[] = '<div class="clearBoth"></div>';
                    $html[] = '</div>';
                }
            $html[] = '</div>';
        }
        $html = implode("\n", $html);
        return $html;
    }

    public function drawWithoutColumns($children)
    {
        $html = '';
        // --- explode by columns ---
        $columns = 0;
        if ($columns < 1) $columns = 1;
        $chunks = $this->_explodeByColumns($children, $columns);
        // --- draw columns ---
        $lastColumnNumber = count($chunks);
        $i = 1;
        foreach ($chunks as $key => $value)
        {
            if (!count($value)) continue;
            $class = '';
            if ($i == 1) $class.= ' first';
            if ($i == $lastColumnNumber) $class.= ' last';
            if ($i % 2) $class.= ' odd'; else $class.= ' even';
            $html.= '<div class="column' . $class . '">';
            $html.= $this->drawMenuItem($value, 1);
            $html.= '</div>';
            $i++;
        }
        return $html;
    }
    protected function _getActiveChildren($parent, $level)
    {
        $activeChildren = array();
        // --- check level ---
        $maxLevel = (int)Mage::getStoreConfig('custom_menu/general/max_level');
        if ($maxLevel > 0)
        {
            if ($level >= ($maxLevel - 1)) return $activeChildren;
        }
        // --- / check level ---
        if (Mage::helper('catalog/category_flat')->isEnabled())
        {
            $children = $parent->getChildrenNodes();
            $childrenCount = count($children);
        }
        else
        {
            $children = $parent->getChildren();
            $childrenCount = $children->count();
        }
        $hasChildren = $children && $childrenCount;
        if ($hasChildren)
        {
            foreach ($children as $child)
            {
                if ($this->_isCategoryDisplayed($child))
                {
                    array_push($activeChildren, $child);
                }
            }
        }
        return $activeChildren;
    }

    private function _isCategoryDisplayed(&$child)
    {
        if (!$child->getIsActive()) return false;
        // === check products count ===
        // --- get collection info ---
        if (!Mage::getStoreConfig('custom_menu/general/display_empty_categories'))
        {
            $data = $this->_getProductsCountData();
            // --- check by id ---
            $id = $child->getId();
            #Mage::log($id); Mage::log($data);
            if (!isset($data[$id]) || !$data[$id]['product_count']) return false;
        }
        // === / check products count ===
        return true;
    }

    private function _getProductsCountData()
    {
        if (is_null($this->_productsCount))
        {
            $collection = Mage::getModel('catalog/category')->getCollection();
            $storeId = Mage::app()->getStore()->getId();
            /* @var $collection Mage_Catalog_Model_Resource_Eav_Mysql4_Category_Collection */
            $collection->addAttributeToSelect('name')
                ->addAttributeToSelect('is_active')
                ->setProductStoreId($storeId)
                ->setLoadProductCount(true)
                ->setStoreId($storeId);
            $productsCount = array();
            foreach($collection as $cat)
            {
                $productsCount[$cat->getId()] = array(
                    'name' => $cat->getName(),
                    'product_count' => $cat->getProductCount(),
                );
            }
            #Mage::log($productsCount);
            $this->_productsCount = $productsCount;
        }
        return $this->_productsCount;
    }

    [REST OF CODE OMITTED FOR BREVITY]
}
3

Best practice in Magento is to not use private methods at all but now you have to deal with it so here we go.

The problem is, you cannot just override a private method in your class because there will be two different methods (your class does not know about the existence of the original private method):

  • WP_CustomMenu_Block_Navigation::_isCategoryDisplayed()
  • Creed_CustomMenu_Block_Navigation::_isCategoryDisplayed()

and dependent on the class from where _isCategoryDisplayed() is called, one or the other will be used.

So your best choice is usually to override all methods that call the private method you change and replace the call. To avoid confusion and make further extension easier I would choose a different name for the new method and make it protected. Yes, this means much more duplicated code than if the method was not private in the first place.

It is also possible to change visibility with Reflection and have less duplication as a result but this is also an ugly hack so I don't recommend it if you don't know exactly what you are doing, so no details here. Don't try this at home, kids

  • thanks this is helpful. I looked in core/mage and found they use plenty of private functions there e.g. Mage_Catalog_Model_Category::_getAttribute. Why would the core code have private functions if it's not Best Practice, just curious, maybe the community module has good reason to use private functions, what do you think? Thanks – Holly Dec 2 '14 at 12:29
  • 1
    Because the Magento rewrite system relies on inheritance for extension, usual convention is to choose protected over private. I couldn't actually find official sources for that, so you are right, it's okay to use private methods if you have reasons for it (like keeping the option to change or remove them without breaking the protected interface). But as you see, this can make extension difficult. – Fabian Schmengler Dec 2 '14 at 13:36
  • thanks, @fschmengler. I actually found a way to refactor my code in Creed_CustomMenu_Block_Navigation so there was no need to call on it's parent class's private functions. It's DRY too :) – Holly Dec 2 '14 at 13:50

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.