Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SiteRouter throws 404 when parsing other Uris instead of returning variables #42766

Open
mavrosxristoforos opened this issue Feb 6, 2024 · 10 comments

Comments

@mavrosxristoforos
Copy link

Steps to reproduce the issue

Edit any system plugin and add this:

public function afterRoute() { // or afterRender, anything after routing the main component.
    $router = \Joomla\CMS\Factory::getContainer()->get(SiteRouter::class);
    $u = \Joomla\CMS\Uri\Uri::getInstance(Uri::base().'component/content/article/2-demo-article');
    // replace the above with the ID and alias of one of your articles
    $variables = $router->parse($u, false);
    var_dump($variables);
}

Expected result

$variables populated with option, view, id.

Actual result

Throws a 404 error.

System information (as much as possible)

Tested in both J4 and 5.

Additional comments

So, the Joomla\CMS\Router\Router is supposedly able to parse any Uri with the option to $setVars = false.
As far as I understand, this is supposed to mean that you can parse any Uri and get its variables, regardless of the active menu item.
Correct me if I'm wrong.

What actually happens is that while most of the process -I read through a lot of the code- is menu-independent, the default StandardRules, at line 72 and the default NoMenuRules, at line 71 use the active menu item as a guide on how to parse the passed Uri object.

If you replace these lines with $active = null, the expected variables return correctly.
I'm relatively unfamiliar with the rest of the routing system, so I'm probably missing a lot.
Is this the expected behavior or is this a bug?

@mavrosxristoforos mavrosxristoforos changed the title Is the SiteRouter supposed to work like that? SiteRouter throwing 404 when parsing instead of returning variables Feb 6, 2024
@mavrosxristoforos mavrosxristoforos changed the title SiteRouter throwing 404 when parsing instead of returning variables SiteRouter throws 404 when parsing instead of returning variables Feb 6, 2024
@mavrosxristoforos mavrosxristoforos changed the title SiteRouter throws 404 when parsing instead of returning variables SiteRouter throws 404 when parsing other Uris instead of returning variables Feb 6, 2024
@Hackwar
Copy link
Member

Hackwar commented Mar 22, 2024

I created a PR to fix this with #43118. Thus I'm closing this issue.

@Hackwar Hackwar closed this as completed Mar 22, 2024
@mavrosxristoforos
Copy link
Author

@Hackwar Thank you for your effort. However, I think that returning an empty array doesn't fix the expected functionality. Since the StandardRules and NoMenuRules rely on the active menu item, there's always something left in the $uri->getPath() if you are trying to parse another URL that is not the active menu item. Did you find something different in your tests?

@Hackwar
Copy link
Member

Hackwar commented Mar 22, 2024

The thing is that the router currently sets the active menu item to the one it parsed in the current run. So when parsing the URL, the active menu item actually is changed, which of course is wrong. But the code now would parse this "correctly". I'm still working on getting the rest cleaned up.

@mavrosxristoforos
Copy link
Author

I'd be happy to help if you want me to.

@Hackwar
Copy link
Member

Hackwar commented Mar 22, 2024

Be my guest. I can use all the help for routing I can get. Ideally could you contact me on the Joomla Mattermost on https://joom.la/chat ? Then we could coordinate that. Just send me a DM

@Hackwar Hackwar reopened this Apr 3, 2024
@Hackwar
Copy link
Member

Hackwar commented Apr 3, 2024

We discussed the PR in the maintainer meeting and decided that my approach wasn't correct. The PR also didn't really solve your issue, I think. Seems like I misunderstood you. The main problem is, that the parse method has side effects from setting the active menu item. My idea would be to introduce a new interface for component routers and when those component routers implement that new interface, the parse() method would have an additional parameter with the current parsed query parameters as well and not just the segments. That way we wouldn't have to read the active menu item.

@mavrosxristoforos
Copy link
Author

As much as I would like to follow your thinking, I'm not that familiar with the routing process. In my initial comment, I mentioned the points of the default rules that the active menu item is being used. Removing them seems to work for me, but of course I cannot imagine how this would affect the millions of installations of Joomla.

@Hackwar
Copy link
Member

Hackwar commented Apr 8, 2024

The problem is, that the component router when parsing does not get the currently discovered menu item ID as input, but just can read that from the active menu item in the global application object. That means we have to change that global state, which affects other things and is VERY bad. So we somehow have to get that information directly into the component router. I'm still looking into how we could do that.

@mavrosxristoforos
Copy link
Author

I understand. This may be a dumb suggestion, but you can judge it: What if you just use the $setVars parameter of the Router to determine if you should use the menu item? Or even add a different parameter? Then, anyone that tries to use the Router::parse would come across this parameter and know what to do with his own code.

@Hackwar
Copy link
Member

Hackwar commented Apr 8, 2024

We don't have the $setVars available at the point where we would have to decide this. However we will probably change the component router to something like this:

public function parse(&$segments, &$vars)

where $vars contains the parsed queries so far, among others the Itemid. I was unsure if this works with our interface, etc. right now, but it does and we hopefully can do this in 5.2 or 5.3.

@Hackwar Hackwar added the bug label Nov 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants