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

Fixing routing bug in Joomla 3.1 #1560

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@mbabker

This comment has been minimized.

Member

mbabker commented Jul 17, 2013

I'm going to make a bold suggestion here. We have JHelperRoute now which IIRC was built to help with code cleanup in these route classes (com_tags does just that). Maybe its time to do the same with the other classes so that we don't have to duplicate the language lookup query (and that inherently makes the same lookup available immediately to all route helpers starting when it is introduced).

@elinw

This comment has been minimized.

Contributor

elinw commented Jul 17, 2013

Yup I already made a comment about that coming next in the tracker on the other issue with category deduplication. Thre is so much useless duplication See #1559

@wilsonge

This comment has been minimized.

Contributor

wilsonge commented Jul 18, 2013

Can this fix not be fast-tracked into 3.1.2 before a better class is designed to avoid the duplication?

@mbabker

This comment has been minimized.

Member

mbabker commented Jul 18, 2013

I'd personally take it as is to get the bug fixed then work out optimization for 3.2. My "optimal" solution is as suggested above, finally having our route helpers extend the newer JHelperRoute and avoiding the duplicated code. The view class changes IMO are nice but not 100% related to this.

@wilsonge

This comment has been minimized.

Contributor

wilsonge commented Jul 18, 2013

Completely agreed :) That's what I was trying to say (just less well!)

@Hackwar

This comment has been minimized.

Member

Hackwar commented Jul 18, 2013

Please first accept this PR and then later nuke JHelperRoute. As I described in several places already, the concept of the *HelperRoute classes is wrong both in practice and in system-theory. It is one of the three big problems in Joomla routing and we should do everything, but please don't encourage people to use this flawed and broken concept. I provided code that implements the routing in a proper fashion on the mailinglist. Please look at that and evaluate it first before forcing us deeper down the HelperRoute-nightmare. Thank you.

@mbabker

This comment has been minimized.

Member

mbabker commented Jul 18, 2013

Works for me, I was just thinking out loud.

@infograf768

This comment has been minimized.

Member

infograf768 commented Jul 20, 2013

This totally breaks associations in multilanguage.
[20-Jul-2013 06:40:05 UTC] PHP Fatal error: Call to a member function getPath() on a non-object in /Applications/MAMP/htdocs/testwindows/trunkgitnew/components/com_content/helpers/route.php on line 88

@infograf768

This comment has been minimized.

Member

infograf768 commented Jul 20, 2013

Tested further:
It looks like this is related to the Similar Tags module when present on the page.

@infograf768

This comment has been minimized.

Member

infograf768 commented Jul 20, 2013

Correction again:
It looks like this happens when the Category is set to registered and not public and one is not logged in.

@Hackwar

This comment has been minimized.

Member

Hackwar commented Jul 20, 2013

I thought we could live without that extra check, but apparently... I added a check to fix this.

@infograf768

This comment has been minimized.

Member

infograf768 commented Jul 20, 2013

We still have, when the category is registered (same conditions as above)
Notice: Undefined index: id in /Applications/MAMP/htdocs/testwindows/trunkgitnew/components/com_content/router.php on line 59

@Hackwar

This comment has been minimized.

Member

Hackwar commented Jul 20, 2013

That looks more like an additional/unrelated bug in the router. I have to say however, that I would point my finger at the code that invokes ContentHelperRoute instead of ContentHelperRoute itself. Right now, ContentHelperRoute is "bug free".

@roland-d

This comment has been minimized.

Contributor

roland-d commented Jul 26, 2013

There seems to be an issue with the sample data routing. Clean install of master using sample data, Click on Article Category List -> click on Subcategory Extension -> click on Subcategories Modules. You now get an empty page. The URL changes from article-category-list/20-extensions to article-category-list/22-extensions/. The same thing happens still with this PR applied.

@Hackwar

This comment has been minimized.

Member

Hackwar commented Jul 26, 2013

I discovered a few more bugs in the routing of Joomla, but I will open another PR for those issues. Maybe you need to rebuild your category/menu table to fix this.

@roland-d

This comment has been minimized.

Contributor

roland-d commented Jul 28, 2013

Hey Hannes,

For "rebuilding" the category/menu table I am not sure, this is using the Joomla sample data.There is an issue with the core code it is not clear yet what has gone wrong. @elinw is looking at that.

@Hackwar

This comment has been minimized.

Member

Hackwar commented Sep 26, 2013

Hi Michael, why did you close this one? This PR is still valid for Joomla 3.1.x. So far you only merged the PR for Joomla 2.5. Please re-open it and merge it. Thanks.

@infograf768

This comment has been minimized.

Member

infograf768 commented Sep 26, 2013

this has been committed
5adbd01

@Hackwar

This comment has been minimized.

Member

Hackwar commented Sep 26, 2013

Ok, sorry, I misinterpreted the message by github here apparently.

@Hackwar Hackwar deleted the Hackwar:routing_bug branch Jan 6, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment