?
avatar OctavianC
OctavianC
31 Jan 2018

Steps to reproduce the issue

Itemid was supposed to be inherited from the current menu item - this is how it's always been as far as I remember (1.5.0) and what the SEF docs say.

  • SEF turned off (doesn't matter, issue occurs regardless of SEF but it's easier to debug)
  • Create an Articles (com_content) menu item - doesn't matter which type
  • Open /components/com_content/content.php
  • Replace it with the code:
var_dump(JRoute::_('index.php?option=com_content&view=test'));
var_dump(JRoute::_('index.php?view=test'));
die;
  • Access the menu item.

Expected result

string(63) "/j384/index.php?option=com_content&view=test&Itemid=121" string(63) "/j384/index.php?view=test&option=com_content&Itemid=121"

Actual result

string(48) "/j384/index.php?option=com_content&view=test" string(63) "/j384/index.php?view=test&option=com_content&Itemid=121"

System information (as much as possible)

3.8.4

Additional comments

This is a regression from #19099
If you replace libraries/src/Router/SiteRouter.php with the one from 3.8.3 everything works as it should.
If there's no option= in the URL, then the Itemid gets inherited correctly, but I don't think developers should have to rewrite all URLs because of this (what I consider to be...) B/C change.

avatar OctavianC OctavianC - open - 31 Jan 2018
avatar joomla-cms-bot joomla-cms-bot - change - 31 Jan 2018
Labels Added: ?
avatar joomla-cms-bot joomla-cms-bot - labeled - 31 Jan 2018
avatar roland-d
roland-d - comment - 31 Jan 2018

@csthomas Can you have a look at this?

avatar OctavianC
OctavianC - comment - 31 Jan 2018

Fix is rather easy for this case, but since that MR dealt with several other cases, I'm not sure if this would break them? Just update the createUri() function to include the else clause that was removed by the PR:

protected function createUri($url)
{
	// Create the URI
	$uri = parent::createUri($url);

	// Get the itemid form the URI
	$itemid = $uri->getVar('Itemid');

	if ($itemid === null)
	{
		if (!$uri->getVar('option'))
		{
			$option = $this->getVar('option');

			if ($option)
			{
				$uri->setVar('option', $option);
			}

			$itemid = $this->getVar('Itemid');

			if ($itemid)
			{
				$uri->setVar('Itemid', $itemid);
			}
		}
		else
		{
			if ($itemid = $this->getVar('Itemid'))
			{
				$uri->setVar('Itemid', $itemid);
			}
		}
	}
	else
	{
		if (!$uri->getVar('option'))
		{
			if ($item = $this->menu->getItem($itemid))
			{
				$uri->setVar('option', $item->component);
			}
		}
	}

	return $uri;
}
avatar franz-wohlkoenig franz-wohlkoenig - change - 31 Jan 2018
Status New Discussion
avatar franz-wohlkoenig franz-wohlkoenig - change - 31 Jan 2018
Category Router / SEF
avatar csthomas
csthomas - comment - 31 Jan 2018

There was a change in routing and now

Itemid was supposed to be inherited from the current menu item

only when you do not add an option=... to link.

For full link like JRoute::_('index.php?option=com_content&view=test') there is no inheritance.

but there is inheritance for JRoute::_('index.php?view=test') but I'm not sure if this is correct.

In general, you can not inherit Itemid from other component.

avatar OctavianC
OctavianC - comment - 31 Jan 2018

It's a B/C change and breaks several of our extensions. I don't see why this change was made and I don't really see the reasoning behind omitting option so that everything works - especially when the current URI option matches, perhaps there should be a change to account for that?
Something in the likes of:

else
{
	if ($this->getVar('option') === $uri->getVar('option') && ($itemid = $this->getVar('Itemid')))
	{
		$uri->setVar('Itemid', $itemid);
	}
}
avatar csthomas
csthomas - comment - 31 Jan 2018

A full link should not inherit Itemid.

If there is no menu item for full link then it should be without Itemid (/component/content/article/1) or with default (current lang) for multilingual website (/en/component/content/article/1).

I did it for one purpose. The full link can not depend on the page on which it is generated.

avatar brianteeman
brianteeman - comment - 31 Jan 2018

Itemid is the glue that holds the site together

If there is no itemid then what are you expecting to be used to display modules or tempplate?

avatar OctavianC
OctavianC - comment - 31 Jan 2018

Sorry but I still think the reason for this is flawed:

  • It breaks compatibility - I've been writing URLs as index.php?option=com_test&view=articles as far as I remember and I don't think I'm the only one. Why should extensions break? The PR had to change test cases (test cases were removed, why?!) so this clearly wasn't thought through.
  • I don't think it takes into account "subpages" (think of index.php?option=com_test&view=article&id=1 - this would inherit the original Itemid and display a nice SEF URL, such as /articles/1-my-article
  • It magically works when omitting the option.

I agree it shouldn't inherit the Itemid if the option is different, so that's why I'm going to create a PR that fixes this.

But I honestly think #19099 should be reverted entirely.

avatar Bakual
Bakual - comment - 31 Jan 2018

For full link like JRoute::('index.php?option=com_content&view=test') there is no inheritance.
but there is inheritance for JRoute::
('index.php?view=test') but I'm not sure if this is correct.

That certainly makes no sense and looks wrong. It shouldn't matter at all if the option is passed or not. Actually I would say it should be passed always anyway.

A full link should not inherit Itemid.

Of course it should, especially if the option matches.

If there is no menu item for full link then it should be without Itemid (/component/content/article/1) or with default (current lang) for multilingual website (/en/component/content/article/1).

There should be no case where NO menu item is assigned since this will break a lot (templatestyle, modules). So at least the default menuitem should be assigned.

avatar joomla-cms-bot joomla-cms-bot - change - 31 Jan 2018
Closed_By franz-wohlkoenig joomla-cms-bot
avatar joomla-cms-bot joomla-cms-bot - close - 31 Jan 2018
avatar franz-wohlkoenig franz-wohlkoenig - change - 31 Jan 2018
Status Discussion Closed
Closed_Date 0000-00-00 00:00:00 2018-01-31 09:12:06
Closed_By franz-wohlkoenig
avatar joomla-cms-bot
joomla-cms-bot - comment - 31 Jan 2018
avatar franz-wohlkoenig
franz-wohlkoenig - comment - 31 Jan 2018

closed as having Pull Request #19498

avatar brianteeman
brianteeman - comment - 31 Jan 2018

I am keeping this open for now. It will be more visible for new posters and will enable further discussion as to should it be reverted in its entirety or just partially as suggested in #19408

avatar brianteeman brianteeman - change - 31 Jan 2018
Status Closed New
Closed_Date 2018-01-31 09:12:06
Closed_By joomla-cms-bot
avatar brianteeman brianteeman - reopen - 31 Jan 2018
avatar csthomas
csthomas - comment - 31 Jan 2018

I understand the routing as, joomla uses menu item to URL only if it match

  • ex Itemid for article should be use only for certain article.
  • for top category, which does not have own menu item and there is no menu item for categories view, the link should look like /component/content/category/[id-category-alias]

If there is no itemid then what are you expecting to be used to display modules or tempplate?

Modules assignment equal to "On all pages" or "On all pages except those selected" will be visible.
Default template will be loaded.

If there's no option= in the URL, then the Itemid gets inherited correctly

IMO a link like JRoute::_('index.php?view=test') also should not inherit Itemid. Only links like &start=... or index.php?start=... should inherit Itemid.
I forgot about it when I added PR.

avatar brianteeman
brianteeman - comment - 31 Jan 2018

That is a break in 13 years of behaviour.

Feeling annoyed at myself that I was working on j4 stuff and never tested or fully understood your PR

avatar Bakual
Bakual - comment - 31 Jan 2018

Modules assignment equal to "On all pages" or "On all pages except those selected" will be visible.
Default template will be loaded.

THERE SHOULD ALWAYS BE AN ITEMID.

avatar franz-wohlkoenig franz-wohlkoenig - change - 31 Jan 2018
Status New Discussion
avatar csthomas
csthomas - comment - 31 Jan 2018

Itemid was supposed to be inherited from the current menu item - this is how it's always been as far as I remember (1.5.0) and what the SEF docs say.

I did not see any text that say: Always inherit Itemid from current page.
There is only sentence:

echo \Joomla\CMS\Router\Route::_('index.php?view=article&id=1&catid=20');

Notice that it is possible to leave out the parameters option and Itemid. option defaults to the name of the component currently being executed, and Itemid defaults to the current menu item's ID.

I understood it as there is no "option" parameter, then use inheritance Itemid otherwise no.

In PR #19501 I added additional inheritance when there is option parameter but view is missing.

avatar OctavianC
OctavianC - comment - 31 Jan 2018

That doesn't make any sense.

avatar csthomas
csthomas - comment - 31 Jan 2018

Because I can not convince anybody, I will not convince any more. Do what you have to do.

If you want to revert all this "improvement" then there is a list to potentially PRs to revert:
#19415
#19295
#19268
#19099

avatar brianteeman
brianteeman - comment - 3 Feb 2018

Closed as the routing changes have been reverted for 3.8.5 with #19512

avatar brianteeman brianteeman - close - 3 Feb 2018
avatar brianteeman brianteeman - change - 3 Feb 2018
Status Discussion Closed
Closed_Date 0000-00-00 00:00:00 2018-02-03 17:17:05
Closed_By brianteeman

Add a Comment

Login with GitHub to post a comment