pages-plugin: Setting isActive for URL type in menu's doesn't work

I noticed that for my menu item with type URL the active class wasn’t set. Discovered that in classes/Menu.php the Request::path() is converted to a full url with URL::to() and later compared to a URL that is not.

Below you can see the code from line 136. You can see at the end that the $currentUrl is compared to the $item->url which is only a path.

$currentUrl = Request::path();

if (!strlen($currentUrl)) {
    $currentUrl = '/';
}

$currentUrl = Str::lower(URL::to($currentUrl));

$activeMenuItem = $page->activeMenuItem ?: false;
$iterator = function($items) use ($currentUrl, &$iterator, $activeMenuItem) {
    $result = [];

    foreach ($items as $item) {
        $parentReference = new MenuItemReference;
        $parentReference->title = $item->title;
        $parentReference->code = $item->code;
        $parentReference->viewBag = $item->viewBag;

        /*
         * If the item type is URL, assign the reference the item's URL and compare the current URL with the item URL
         * to determine whether the item is active.
         */
        if ($item->type == 'url') {
            $parentReference->url = $item->url;
            $parentReference->isActive = $currentUrl == Str::lower($item->url) || $activeMenuItem === $item->code;
        }
        else {
            /*
             * If the item type is not URL, use the API to request the item type's provider to
             * return the item URL, subitems and determine whether the item is active.
             */
            $apiResult = Event::fire('pages.menuitem.resolveItem', [$item->type, $item, $currentUrl, $this->theme]);

I believe the converted $currentUrl is only used later on in the else statement for firing the pages.menuitem.resolveItem event. It would be more logical to convert it here right? This would result in following code;

$currentUrl = Str::lower(Request::path());

if (!strlen($currentUrl)) {
    $currentUrl = '/';
}

$activeMenuItem = $page->activeMenuItem ?: false;
$iterator = function($items) use ($currentUrl, &$iterator, $activeMenuItem) {
    $result = [];

    foreach ($items as $item) {
        $parentReference = new MenuItemReference;
        $parentReference->title = $item->title;
        $parentReference->code = $item->code;
        $parentReference->viewBag = $item->viewBag;

        /*
         * If the item type is URL, assign the reference the item's URL and compare the current URL with the item URL
         * to determine whether the item is active.
         */
        if ($item->type == 'url') {
            $parentReference->url = $item->url;
            $parentReference->isActive = $currentUrl == Str::lower($item->url) || $activeMenuItem === $item->code;
        }
        else {
            /*
             * If the item type is not URL, use the API to request the item type's provider to
             * return the item URL, subitems and determine whether the item is active.
             */
            $apiResult = Event::fire('pages.menuitem.resolveItem', [$item->type, $item, URL::to($currentUrl)

, $this->theme]);

About this issue

  • Original URL
  • State: closed
  • Created 8 years ago
  • Comments: 15 (4 by maintainers)

Most upvoted comments

please fix

Hi LaravelNick.

To fix this you need to open the file plugins/rainlab/pages/classes/Menu.php and edit the following lines;

Change following line 130 $currentUrl = Request::path(); To 130 $currentUrl = Str::lower(Request::path());

Remove following line 136 $currentUrl = Str::lower(URL::to($currentUrl));

Change following line 159 $apiResult = Event::fire('pages.menuitem.resolveItem', [$item->type, $item, $currentUrl, $this->theme]); To 159 $apiResult = Event::fire('pages.menuitem.resolveItem', [$item->type, $item, URL::to($currentUrl), $this->theme]);

Good luck 😉