@mbabker Please review the changes now. I have kept the magic method and it does not ignore any errors. If you suggest I can also remove that altogether.

I started digging into the test problems. We're in dependency hell.

Problem 1 - TestMockApplicationCms::getMethods() needs to include the getName method
Problem 2 - For JPaginationTest, need to add this line to configure the application mock in the setUp method (note that there will be some places that need to change the mock return to administrator in that test class)

$app->expects($this->any())->method('getName')->willReturn('site');

Problem 3 - JApplicationCms::getRouter() is static, it cannot be mocked, so we have to work with a real router (or use Reflection and push a mock into the JRoute::$_router property)
Problem 4 - JRouter::getInstance() does not support constructor injection of the application and menu objects for the JRouterSite subclass, so there is no way to inject stuff, meaning that code is falling onto the defaults
Problem 5 - The JRouterSite constructor calls JApplicationCms::getInstance('site'), this tries to create a JApplicationSite object with default behaviors since there isn't a mock application set up in JApplicationCms::$instances, and this ultimately leads up to a session start failure
Problem 6 - I got too deep into this and said "no more" 😆

So if you change https://github.com/izharaazmi/joomla-cms/blob/c7dd17d70bebe41230c28140612eccce5d727506/libraries/joomla/application/route.php#L87 to be if (!isset(self::$_router[$client])) (i.e. allow $client to be null), then unit tests pass and effectively replicate the current behaviour (admittedly the incorrect behaviour).

Well, good thing JRouter isn't abstract then.

If we are going the way in the pr #16940 that we try to move away from the static JRoute class and make the function as part of the router class itself, then we can solve this issue here as well. The SiteRouter and AdministratorRouter would then both have a route function which creates a link then accordingly to Site or Administrator.

It would then end up in a way that you can do Joomla\CMS\Router::getInstance('site')->route() or when you want the admin url Joomla\CMS\Router::getInstance('administrator')->route(). Or if you want to go trough the app then something like CMSApplication::getInstance('site')->getRouter()->route() will also work.
In J4 it lets you then even inject the proper router and you can do the route (or if you want to name it link) function.

We need to make sure that Joomla\CMS\Router::getInstance('administrator')->route() approach doesn't interfere with the session start thing too.

I think JRoute methods can be a proxy to this method too. We can keep it or drop it in 4.0

I have a grey recall about something like we are trying to drop the getInstance()methods. Isn't that true?

approach doesn't interfere with the session start thing too

Good question, but I don't think there is an issue as all what the _ function does is to add some stuff around the router->build function.

Yes the getInstance will become deprecated, but there will be then other ways to get a router. So the important part is that the function should not be static and be on the router itself.

The JRoute::link is currently failing when trying to implement router instantiation for the other client application already. Thats why I am concerned for the same thing in the proposed route method.

Don't get you. Can you elaborate it more please?

Please read comment above by @mbabker. If that doesn't help too, sure I
would try to elaborate myself.

In the test suite we basically need to use Reflection and drop a mock router into JRoute when testing, or use Reflection and drop mock application classes into JApplicationCms. Since JApplicationCms::getRouter() is static, it can't be mocked, and that's really where trying to fix the test issues in this PR starts falling apart.

If we move the function to the router then we have the application injected. Would that help?

No. The base router class and the admin router class don't need the application object, so those are fine generally. JRouter::getInstance('site')->route() would still run into the same problems because you cannot pass additional constructor parameters into the getInstance() method for subclasses, all it knows how to handle is an options array. So going through that route, the only way to get the app and menu params for JRouterSite objects is through the constructor defaults, which right now is JApplication::getInstance('site') to get the app and $app->getMenu() to get the menu.

In our testing environment, it's really quite complex to make this all work right. There are too many dependencies that have to get set up right somewhere (either pushing router mocks into JRoute::$_router or JRouter::$instances or pushing application mocks into JApplicationCms::$instances at a minimum, then mocking the internals; though considering it's the JPagination tests we're talking about here as the primary failure point, those shouldn't be trying to validate "real" routing behaviors anyway).

The reason this has all been working fine up until the request we made was implemented (stop allowing a null client), as George hinted to, is because it has been possible to work with a null client value internal to JRoute::_() because $app-> getName() isn't mocked meaning you don't get a real response from that method, so the resulting call ends up as $app->getRouter(null) which returns a JRouter instance (the base class, not one of the application subclasses). Obviously this isn't the "right" behavior as in normal use of the CMS it should be pretty freakin' impossible to do that.

For me would be interesting to know if we want to introduce more static functions or try to work more with objects.

@izharaazmi Check izharaazmi#3 and merge it please. That should fix all the test issues.

IMO this is good to merge. If we could get a couple tests here it would honestly be a pretty good improvement to have in 3.8.

... it would honestly be a pretty good improvement to have in 3.8.

After this is tested and merged the article manager can get a preview icon per row ...

Also a lot of 3rd party extensions developers will like this, and then their users too
Finally i will be able to replace the code that i am using now to genererate frontend SEF URLs in backend

I ll find time to test

The feature is really cool, but I really would like to see this implemented in a none static way.

We can look at transitioning JRoute to a non-static service at a later point. In the 3.x code unless you're in a handful of spots you're still going to have to go through a static global to do this, might as well consolidate it into one spot and in the 4.x code we can look at how to transform this into a proper OOP API.

but I really would like to see this implemented in a none static way

This is a static helper method for Router::build, nothing more, I don't see any real need for it to be non-static

I just did some testing of this. Nearly works but in the admin we need to enable sef urls otherwise i always end up with non-sef urls. I think moving this line https://github.com/joomla/joomla-cms/blob/staging/libraries/src/Joomla/CMS/Application/SiteApplication.php#L403 into the main CMSApplication class should do the trick :)

It's this line:

https://github.com/joomla/joomla-cms/blob/staging/libraries/src/Joomla/CMS/Application/SiteApplication.php#L403

That needs to be moved from the SiteApplication to the CMSApplication's getRouter method and should probably be tweaked to read this instead:

$options['mode'] = isset($options['mode']) ? $options['mode'] : \JFactory::getConfig()->get('sef');

Right now when you go through the admin app's getRouter method to get a site router, you won't get the correct SEF mode because we are only setting it for the site application. So as is you're only getting non SEF URLs for frontend content from the backend.

Sorry I didn't notice its still against 3.8-dev. I'll fix it.

Updated Pull Request location. Looks like there's a couple of extra conflicts as a result. But nearly there!!

Resolved the conflicts.

At this stage I would like to follow with @mbabker in #16879 (comment) and do the following changes:

• Make real method stubs for JRoute::site() and JRoute::administrator() removing the magic method.
• Make the link method protected to prevent having multiple syntax to do the same thing.

Please advise.

In my personal opinion as I'm currently building a new application api in Joomla 4, I'd rather remove the JRoute::site() and JRoute:: administrator() methods and if you're going cross site force people to use JRoute::link()

If I am not overthinking 😄 , are you doing something like removing strict references to site and administrator as the only supported applications?

...force people to use JRoute::link()

So, should I just remove the magic method and do not have these methods at all in the first place?

If I am not overthinking 😄 , are you doing something like removing strict references to site and administrator as the only supported applications?

Yes

So, should I just remove the magic method and do not have these methods at all in the first place?

In my opinion yes. See what @mbabker says

Can we get this in 3.8?

If this can get tested out no later than beta 3 then yes.

I'll try and get this tested. But would love to see it tested and added, planning a new feature to 'View' Menu Items from the backend which loads the link in a new window/tab and this would allow SEF Routing nicely.

quick test (with default test content):

JRoute::link('site', 'index.php?option=com_content&view=article&id=17');
=> /index.php?id=17 wrong (note: article does not have a menu item)

JRoute::link('site', 'index.php?option=com_content&view=article&id=6');
=> /index.php/single-article correct (note: article has a menu item)

JRoute::link('site', 'index.php?option=com_content&view=category&layout=blog&id=14');
=> /index.php/14-sample-data-articles?layout=blog seems correct

JRoute::link('site', 'index.php?option=com_content&view=category&layout=blog&id=27');
=> /index.php/article-category-blog correct

half working 😉
But maybe it more an issue with the Content router than with this patch, hard to say.

But maybe it more an issue with the Content router than with this patch, hard to say.

It is I think. If you add the category id into the url then it should work ok :)

In theory that article ID 17 should still get a SEF URL unless all of its parent categories too have no menu item (or its only parent is a featured or archived view).

The sef_advanced mode is broken for com_content.

It is I think.

Yeap, I just tried default method JRoute::_( 'index.php?option=com_content&view=article&id=17'); at the site, and got same result.

As it do not related to the current patch, I am going to mark it as success.

I have tested this item ✅ successfully on b3136c4

I have tested this item ✅ successfully on b3136c4

@izharaazmi & @andrepereiradasilva great PR, have been using a hacky way so far to generate frontend SEF links from the backend, so much welcome :-)

I have been playing around with the patch, and for all links I tried the output was similar in the frontend as in the backend. Tried varies combinations, SEF on/off, htaccess on/off, stable/experimental router. So seems to work correctly!

@mbabker Its post beta4 now. Is there still any change getting this in?

Conflicts resolved.

I have tested this item ✅ successfully on b3136c4

Love this PR.

Both in frontend as backend the expected url's show up, both with SEF enabled and disabled.

RTC after two successful tests.

I have tested this and really need this PR to be added in Joomla soon, It's been month how long do we have to wait. Please consider releasing this soon

@asfaqueali

Unfortunately this cannot be added into joomla! Until 3.9 since new features cannot go into 3.8 as that would break versioning.

@tonypartridge yeah I understand, but its been almost 4 months and release date of 3.9 is not defined so far.

It won’t be merged into 3.9 until 3.9 is ready to be merged to the staging branch.

@wilsonge is the 3.9 release lead I believe and won’t miss it out I’m sure.

On 9 Feb 2018, 07:51 +0000, Asfaque Ali Ansari notifications@github.com, wrote:

@tonypartridge yeah I understand, but its been almost 4 months and release date of 3.9 is not defined so far.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

Merged - awesome work :)

@tonypartridge @wilsonge Thanks for merging it. @izharaazmi thanks for this commit.

Thanks George! I’ll try and get onto the view in site button next week for menu items

On 9 Mar 2018, 04:40 +0000, Asfaque Ali Ansari notifications@github.com, wrote:

@tonypartridge @wilsonge Thanks for merging it. @izharaazmi thanks for this commit.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

Hi guys,

I am not having a successful experience using the new JRoute::link method. It simply doesn't appear to work in my plugin at all. The code is below. The calls to link are commented-out because they simply don't do anything.

// Build article links
$rel_link = str_replace('/administrator','',JRoute::_(ContentHelperRoute::getArticleRoute($article->id,$article->catid)));
$abs_link = str_replace('/administrator','',JURI::base().$rel_link);
$abs_link = str_replace(':/','://',str_replace('//','/',$abs_link));
// $abs_link = JRoute::link('site',$abs_link);
// $rel_link = JRoute::link('site',$rel_link);

As you can see, I still have to remove the "administrator//" part of back end links and for back end links I still get no SEF URL out of JRoute::link. This particular plugin has a feature that allows either an absolute or relative link to be inserted into the template being used to email a notification that an article was just published or updated.

I'm hoping someone here can show me the err of my ways and, if applicable, point me to where I should have posted this comment.

Thanks,
Bruce

@bascherz please open a new Issue (link to this PR) as comments on closed Pull Requests didn't get much notice.

@bascherz - Your code is wrong, you are trying to JRoute a JRouted URL you cannot do that, you need to call the url non-routed i.e. with index.php?option=com_content etc. So try removing the JRoute::_() on the $rel_link = . Also you shouldn't need to replace administrator.

Documentation for this is available at https://docs.joomla.org/J3.x:Supporting_SEF_URLs_in_your_component#Creating_Frontend_SEF_URLs_in_the_Administrator as of approximately 5 minutes ago :)

Documentation for this is available at https://docs.joomla.org/J3.x:Supporting_SEF_URLs_in_your_component#Creating_Frontend_SEF_URLs_in_the_Administrator as of approximately 5 minutes ago :)

Somewhat a brute force method, but it does work. Some of my URLs are not SEF (for whatever reason) and that threw me off a bit as well. No need to open an issue @franz-wohlkoenig.

Here's the code now. Category ID apparently is not needed.

$rel_link = JRoute::link('site','index.php?option=com_content&view=article&id='.$article->id);
$abs_link = JURI::base().$rel_link;

Thanks everyone.

It's not required but probably helpful to get exactly the same url as the frontend

Also missing language ;)

		if ($article->language && Multilanguage::isEnabled())
		{
			// Get the sef prefix for the language
			$query->clear()
				->select('sef')
				->from($db->quoteName('#__languages'))
				->where($db->quoteName('published') . '= 1')
				->where($db->quoteName('lang_code') . ' = ' . $db->quote($article->language));
			$db->setQuery($query);

			$sef = $db->loadResult();

			if ($article->language != '*')
			{
				$lang = '&lang=' . $sef;
			}
		}