Modern routing - remove view from query string #18429

csthomas · 2017-10-27T20:15:01Z

Summary of Changes

Build a correct URL for links without layout or when the layout from URL is different than from menu item.

Testing Instructions

Install staging joomla or 3.8.2 with sample data.
On back-end enable modern routing in users.
Optional enable Allow User Registration

Go to home page and:

check module login links,
try to login with wrong password and check URL address after that
check URL address on profile edit form

Before	After
`/index.php/password-reset?view=reset`	`/index.php/password-reset`
`/index.php/username-reminder?view=remind`	`/index.php/username-reminder`
`/index.php/login?view=login`	`/index.php/login`
`/index.php/registration-form?view=registration`	`/index.php/registration-form`
`/index.php/your-profile?view=profile&layout=edit`	`/index.php/your-profile?layout=edit`

Expected result

Links with own menu item does not have a view parameter in query string.

Documentation Changes Required

No

joomdonation · 2017-11-22T09:59:32Z

libraries/src/Component/Router/Rules/StandardRules.php

@@ -182,14 +182,22 @@ public function parse(&$segments, &$vars)
 	 */
 	public function build(&$query, &$segments)
 	{
+		if (isset($query['Itemid'], $query['view']) === false)


Maybe just if (!isset($query['view'], $query['Itemid'])) will make the code easier to read?

Yes, it will be easier to read.

joomdonation · 2017-11-22T10:55:15Z

libraries/src/Component/Router/Rules/StandardRules.php

 		// Get the menu item belonging to the Itemid that has been found
 		$item = $this->router->menu->getItem($query['Itemid']);

-		if (!isset($query['view']))
+		if ($item === null)


Do we also want to check and make sure the menu item is from the the component which we are process the routing? For example, change code to:

if ($item === null || $item->component != 'com_' . $this->router->getName())

joomdonation · 2017-11-22T11:01:36Z

So I looked at the code changes and also tested, it is working fine and solve the mentioned issue. However, there are some changes in the code which I am unsure if it is necessary, Maybe less change like I did here will still do the same thing https://github.com/joomla/joomla-cms/compare/staging...joomdonation:routing-fix-simpler?expand=1

csthomas · 2017-11-22T12:24:31Z

A few changes is done because I want to present more explicit way for routing (layout).
If there is no layout parameter in menu query this means that the default layout is 'default'.
I want to stay as it is now. My goal is #17746.

joomdonation · 2017-11-22T13:45:16Z

If I understand correctly, the additional changes you added to this PR is only needed if someone pass layout=default in JRoute::_ call (which is wrong, I think).

The original code looks fine to me. We are working on a PR for staging, so I would stay on safe side, the less changes, the better, only the necessary change to address the issue should be implemented.

joomdonation · 2017-11-29T15:42:28Z

libraries/src/Component/Router/Rules/StandardRules.php

@@ -198,7 +206,25 @@ public function build(&$query, &$segments)
 		{
 			$view = $views[$query['view']];

-			if (isset($item->query[$view->key], $query[$view->key]) && $item->query[$view->key] == (int) $query[$view->key])
+			if ($view->key === false)


To make code consistent with other places in routing code, I think this should be changed back to if (!$view->key)

I wanted the code to be clearer. Ok I will change it back.

joomdonation · 2017-11-29T15:45:04Z

libraries/src/Component/Router/Rules/StandardRules.php

+				$item->query[$view->key] = 0;
+			}
+
+			if (isset($query[$view->key]) && $item->query[$view->key] == (int) $query[$view->key])


Personal, I like the original code https://github.com/joomla/joomla-cms/blob/staging/libraries/src/Component/Router/Rules/StandardRules.php#L201 instead of this change (requires added few lines of code above this line)

The original code does not work for content edit form (Create Article).
Link to generate SEF is index.php?option=com_content&view=form&layout=edit&a_id=0
But in menu link is index.php?option=com_content&view=form&layout=edit.
In this situation I assume the value of key (a_id) is 0.

As I can see, link to create new article doesn't have a_id=0 in URL. Not sure where you get it? I tried to create a menu item to link to Create article menu option and the none sef link is index.php?option=com_content&view=form&layout=edit (not contains a_id=0) . So the original code should still work fine

Found it, for some reasons, a_id=0 is passed in some places in the code. Not sure why it is needed.

Hi @csthomas

When we create a menu item to link to Create Article menu option, Joomla somehow adds a_id=0 automatically to the URL of the menu item. You can see it by running this code (103 is the menu item I created to Create Article menu option)

$item = JFactory::getApplication()->getMenu()->getItem(103); print_r($item->query);

It displays Array ( [option] => com_content [view] => form [layout] => edit [a_id] => 0 ) (note a_id = 0)

So as I can see, this change is not needed.

OK, it is set automatically at https://github.com/joomla/joomla-cms/blob/staging/libraries/src/Component/Router/Rules/MenuRules.php#L231

I added it for some unit test that test StandardRule class without loading MenuRule class, some time ago. If we can trust that MenuRule always is called before StandardRule then OK. I will remove:

// If item has no key set, we assume 0. if (!isset($item->query[$view->key])) { $item->query[$view->key] = 0; }

joomdonation · 2017-11-29T15:55:12Z

I have tested this item ✅ successfully on 3d32326

Works as described, done a code review, too. I am not happy with some changes in the code, but It might be personal coding style only

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/18429.}

csthomas · 2017-11-29T16:40:52Z

I reverted the code with non strict comparison to the false.

ghost · 2017-11-30T09:24:40Z

@joomdonation can you please retest?

joomdonation · 2017-11-30T09:27:32Z

@franz-wohlkoenig Will do after work. Maybe you can test it as well, just follow the instructions or ask us if needed. We need one more test anyway

ghost · 2017-11-30T09:38:26Z

Cannot test next Days as i'm not at Home.

joomdonation · 2017-12-02T13:23:03Z

I have tested this item ✅ successfully on 6396a6d

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/18429.}

laoneo · 2017-12-07T07:03:38Z

I have tested this item ✅ successfully on 6396a6d

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/18429.}

ghost · 2017-12-07T07:45:30Z

Ready to Commit after two successful tests.

joomla-cms-bot added PR-staging Unit/System Tests labels Oct 27, 2017

csthomas mentioned this pull request Oct 27, 2017

Deterministic version of URL routing #17746

Closed

csthomas changed the title ~~Modern routing - fix urls with layout~~ Modern routing - fix URL with layout and remove PHP Notice from URL with invalid Itemid Oct 27, 2017

csthomas mentioned this pull request Nov 3, 2017

[3.8-dev] New Router issues + Fatal error #16496

Closed

Modern routing - preserve different layout

a908c46

csthomas force-pushed the routing_build_with_layout branch from cee53c9 to a908c46 Compare November 16, 2017 10:32

This was referenced Nov 21, 2017

Home page wrong url when home is a featured articles using 3.8 new routing system #18771

Closed

[4.0] com_users routing issue #18774

Closed

joomdonation reviewed Nov 22, 2017

View reviewed changes

Check if item component is equal to router name, thanks @joomdonation

92078fe

csthomas mentioned this pull request Nov 24, 2017

[4.0] Multilingual. Home page wrong title and url when home is a featured menu item #16691

Closed

Do not unsed 'default' query layout if the menu item layout is not set

3d32326

joomdonation reviewed Nov 29, 2017

View reviewed changes

Revert the change with strict comparision to false

3a45164

Remove a few lines

6396a6d

csthomas changed the title ~~Modern routing - fix URL with layout and remove PHP Notice from URL with invalid Itemid~~ Modern routing - remove view from query string Dec 6, 2017

joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Dec 7, 2017

mbabker added this to the Joomla 3.8.3 milestone Dec 7, 2017

mbabker merged commit 27e730b into joomla:staging Dec 7, 2017

joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Dec 7, 2017

csthomas deleted the routing_build_with_layout branch December 7, 2017 13:08

Quy mentioned this pull request Jan 31, 2018

[Router] RulesStandard Supports default layout #17497

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modern routing - remove view from query string #18429

Modern routing - remove view from query string #18429

csthomas commented Oct 27, 2017 •

edited by joomla-cms-bot

joomdonation Nov 22, 2017

csthomas Nov 22, 2017

joomdonation Nov 22, 2017

joomdonation commented Nov 22, 2017

csthomas commented Nov 22, 2017

joomdonation commented Nov 22, 2017

joomdonation Nov 29, 2017

csthomas Nov 29, 2017

joomdonation Nov 29, 2017

csthomas Nov 29, 2017

joomdonation Nov 30, 2017

joomdonation Nov 30, 2017

joomdonation Dec 2, 2017

csthomas Dec 2, 2017 •

edited

joomdonation commented Nov 29, 2017

csthomas commented Nov 29, 2017

ghost commented Nov 30, 2017

joomdonation commented Nov 30, 2017

ghost commented Nov 30, 2017

joomdonation commented Dec 2, 2017

laoneo commented Dec 7, 2017

ghost commented Dec 7, 2017

Modern routing - remove view from query string #18429

Modern routing - remove view from query string #18429

Conversation

csthomas commented Oct 27, 2017 • edited by joomla-cms-bot

Summary of Changes

Testing Instructions

Expected result

Documentation Changes Required

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joomdonation commented Nov 22, 2017

csthomas commented Nov 22, 2017

joomdonation commented Nov 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

csthomas Dec 2, 2017 • edited

Choose a reason for hiding this comment

joomdonation commented Nov 29, 2017

csthomas commented Nov 29, 2017

ghost commented Nov 30, 2017

joomdonation commented Nov 30, 2017

ghost commented Nov 30, 2017

joomdonation commented Dec 2, 2017

laoneo commented Dec 7, 2017

ghost commented Dec 7, 2017

csthomas commented Oct 27, 2017 •

edited by joomla-cms-bot

csthomas Dec 2, 2017 •

edited