Make sure 'last' class is set on top menu

Previously the 'last' class was not set on the last visible top menu
item when one of the top level categories was disabled.

Also improved:
+ Split up the _getHtml() method to removed the @SuppressWarnings
+ Removed unneeded variables (moved the content of the variables to the
place where they are actually are being used).
+ Removed unused $itemPosition variable in _getHtml method
+ Optimized imports

Author: arnoudhgz

app/code/Magento/Theme/Block/Html/Topmenu.php

@@ -5,9 +5,12 @@
  */
 namespace Magento\Theme\Block\Html;
 
+use Magento\Backend\Model\Menu;
 use Magento\Framework\Data\Tree\Node;
+use Magento\Framework\Data\Tree\Node\Collection;
 use Magento\Framework\Data\Tree\NodeFactory;
 use Magento\Framework\Data\TreeFactory;
+use Magento\Framework\DataObject;
 use Magento\Framework\DataObject\IdentityInterface;
 use Magento\Framework\View\Element\Template;
 
@@ -29,7 +32,7 @@ class Topmenu extends Template implements IdentityInterface
     /**
      * Top menu data tree
      *
-     * @var \Magento\Framework\Data\Tree\Node
+     * @var Node
      */
     protected $_menu;
 
@@ -89,28 +92,29 @@ public function getHtml($outermostClass = '', $childrenWrapClass = '', $limit =
         $this->getMenu()->setOutermostClass($outermostClass);
         $this->getMenu()->setChildrenWrapClass($childrenWrapClass);
 
-        $html = $this->_getHtml($this->getMenu(), $childrenWrapClass, $limit);
+        $transportObject = new DataObject([
+            'html' => $this->_getHtml($this->getMenu(), $childrenWrapClass, $limit)
+        ]);
 
-        $transportObject = new \Magento\Framework\DataObject(['html' => $html]);
         $this->_eventManager->dispatch(
             'page_block_html_topmenu_gethtml_after',
             ['menu' => $this->getMenu(), 'transportObject' => $transportObject]
         );
-        $html = $transportObject->getHtml();
-        return $html;
+
+        return $transportObject->getHtml();
     }
 
     /**
      * Count All Subnavigation Items
      *
-     * @param \Magento\Backend\Model\Menu $items
+     * @param Menu $items
      * @return int
      */
     protected function _countItems($items)
     {
         $total = $items->count();
         foreach ($items as $item) {
-            /** @var $item \Magento\Backend\Model\Menu\Item */
+            /** @var $item Menu\Item */
             if ($item->hasChildren()) {
                 $total += $this->_countItems($item->getChildren());
             }
@@ -121,7 +125,7 @@ protected function _countItems($items)
     /**
      * Building Array with Column Brake Stops
      *
-     * @param \Magento\Backend\Model\Menu $items
+     * @param Menu $items
      * @param int $limit
      * @return array|void
      *
@@ -164,7 +168,7 @@ protected function _columnBrake($items, $limit)
     /**
      * Add sub menu HTML code for current menu item
      *
-     * @param \Magento\Framework\Data\Tree\Node $child
+     * @param Node $child
      * @param string $childLevel
      * @param string $childrenWrapClass
      * @param int $limit
@@ -192,48 +196,41 @@ protected function _addSubMenu($child, $childLevel, $childrenWrapClass, $limit)
     /**
      * Recursively generates top menu html from data that is specified in $menuTree
      *
-     * @param \Magento\Framework\Data\Tree\Node $menuTree
+     * @param Node $menuTree
      * @param string $childrenWrapClass
      * @param int $limit
      * @param array $colBrakes
      * @return string
-     *
-     * @SuppressWarnings(PHPMD.CyclomaticComplexity)
-     * @SuppressWarnings(PHPMD.NPathComplexity)
      */
     protected function _getHtml(
-        \Magento\Framework\Data\Tree\Node $menuTree,
+        Node $menuTree,
         $childrenWrapClass,
         $limit,
         array $colBrakes = []
     ) {
         $html = '';
 
         $children = $menuTree->getChildren();
-        $parentLevel = $menuTree->getLevel();
-        $childLevel = $parentLevel === null ? 0 : $parentLevel + 1;
+        $this->removeChildrenWithoutActiveParent($children);
+        $childLevel = $this->getChildLevel($menuTree->getLevel());
 
         $counter = 1;
-        $itemPosition = 1;
         $childrenCount = $children->count();
 
         $parentPositionClass = $menuTree->getPositionClass();
         $itemPositionClassPrefix = $parentPositionClass ? $parentPositionClass . '-' : 'nav-';
 
-        /** @var \Magento\Framework\Data\Tree\Node $child */
+        /** @var Node $child */
         foreach ($children as $child) {
-            if ($childLevel === 0 && $child->getData('is_parent_active') === false) {
-                continue;
-            }
             $child->setLevel($childLevel);
-            $child->setIsFirst($counter == 1);
-            $child->setIsLast($counter == $childrenCount);
+            $child->setIsFirst($counter === 1);
+            $child->setIsLast($counter === $childrenCount);
             $child->setPositionClass($itemPositionClassPrefix . $counter);
 
             $outermostClassCode = '';
             $outermostClass = $menuTree->getOutermostClass();
 
-            if ($childLevel == 0 && $outermostClass) {
+            if ($childLevel === 0 && $outermostClass) {
                 $outermostClassCode = ' class="' . $outermostClass . '" ';
                 $currentClass = $child->getClass();
 
@@ -244,7 +241,7 @@ protected function _getHtml(
                 }
             }
 
-            if (is_array($colBrakes) && count($colBrakes) && $colBrakes[$counter]['colbrake']) {
+            if ($this->shouldAddNewColumn($colBrakes, $counter)) {
                 $html .= '</ul></li><li class="column"><ul>';
             }
 
@@ -257,7 +254,6 @@ protected function _getHtml(
                 $childrenWrapClass,
                 $limit
             ) . '</li>';
-            $itemPosition++;
             $counter++;
         }
 
@@ -271,14 +267,13 @@ protected function _getHtml(
     /**
      * Generates string with all attributes that should be present in menu item element
      *
-     * @param \Magento\Framework\Data\Tree\Node $item
+     * @param Node $item
      * @return string
      */
-    protected function _getRenderedMenuItemAttributes(\Magento\Framework\Data\Tree\Node $item)
+    protected function _getRenderedMenuItemAttributes(Node $item)
     {
         $html = '';
-        $attributes = $this->_getMenuItemAttributes($item);
-        foreach ($attributes as $attributeName => $attributeValue) {
+        foreach ($this->_getMenuItemAttributes($item) as $attributeName => $attributeValue) {
             $html .= ' ' . $attributeName . '="' . str_replace('"', '\"', $attributeValue) . '"';
         }
         return $html;
@@ -287,27 +282,26 @@ protected function _getRenderedMenuItemAttributes(\Magento\Framework\Data\Tree\N
     /**
      * Returns array of menu item's attributes
      *
-     * @param \Magento\Framework\Data\Tree\Node $item
+     * @param Node $item
      * @return array
      */
-    protected function _getMenuItemAttributes(\Magento\Framework\Data\Tree\Node $item)
+    protected function _getMenuItemAttributes(Node $item)
     {
-        $menuItemClasses = $this->_getMenuItemClasses($item);
-        return ['class' => implode(' ', $menuItemClasses)];
+        return ['class' => implode(' ', $this->_getMenuItemClasses($item))];
     }
 
     /**
      * Returns array of menu item's classes
      *
-     * @param \Magento\Framework\Data\Tree\Node $item
+     * @param Node $item
      * @return array
      */
-    protected function _getMenuItemClasses(\Magento\Framework\Data\Tree\Node $item)
+    protected function _getMenuItemClasses(Node $item)
     {
-        $classes = [];
-
-        $classes[] = 'level' . $item->getLevel();
-        $classes[] = $item->getPositionClass();
+        $classes = [
+            'level' . $item->getLevel(),
+            $item->getPositionClass(),
+        ];
 
         if ($item->getIsCategory()) {
             $classes[] = 'category-item';
@@ -375,7 +369,7 @@ protected function getCacheTags()
     /**
      * Get menu object.
      *
-     * Creates \Magento\Framework\Data\Tree\Node root node object.
+     * Creates Tree root node object.
      * The creation logic was moved from class constructor into separate method.
      *
      * @return Node
@@ -394,4 +388,44 @@ public function getMenu()
         }
         return $this->_menu;
     }
+
+    /**
+     * Remove children from collection when the parent is not active
+     *
+     * @param Collection $children
+     *
+     * @return void
+     */
+    private function removeChildrenWithoutActiveParent(Collection $children)
+    {
+        /** @var Node $child */
+        foreach ($children as $child) {
+            if ($child->getData('is_parent_active') === false) {
+                $children->delete($child);
+            }
+        }
+    }
+
+    /**
+     * Retrieve child level based on parent level
+     *
+     * @param int $parentLevel
+     *
+     * @return int
+     */
+    private function getChildLevel($parentLevel)
+    {
+        return $parentLevel === null ? 0 : $parentLevel + 1;
+    }
+
+    /**
+     * @param array $colBrakes
+     * @param $counter
+     *
+     * @return bool
+     */
+    private function shouldAddNewColumn(array $colBrakes, int $counter)
+    {
+        return count($colBrakes) && $colBrakes[$counter]['colbrake'];
+    }
 }