I reviewed your change and it indeed fixes the issue. A good catch. However, I see 2 theoretical issues:

1. What if someone extends from this class? Since $lookup is not private, they could access this and this change could break that.
2. Coincidentally, the class is inherited by a RouteHelper class in com_tags, but that class overwrites the original lookup and it is also dead code at this point.

I would consider the risk for this to break anything minimal, but if we want to be nitpicking here, this would be a b/c break. For me, it is a weighing between fixing the bug and introducing a minor b/c break.

What if someone extends from this class? Since $lookup is not private, they could access this and this change could break that.

Lots of Joomla classes can be extended and break smth. It's not a problem. We have an open source "easily extendable" project, I am sure that is should not contain the words "private" or "final" at all. Otherwise, it looks like commercial code. And making $lookup private will break extensions which can potentially inherit from class.

Coincidentally, the class is inherited by a RouteHelper class in com_tags, but that class overwrites the original lookup and it is also dead code at this point.

I don't see any overrides in TagsHelperRoute.

I would consider the risk for this to break anything minimal, but if we want to be nitpicking here, this would be a b/c break. For me, it is a weighing between fixing the bug and introducing a minor b/c break.

To be honest, I don't see any b/c breaks, it's just a bug fix. The people are waiting for this bug to be fixed since Apr.

Create multiple tagged items from different component but with the same view name.

@AlterBrains can you give an example what exactly to test?

You need two components, each with items which can be tagged using native Joomla tags, and items should have the same "view" name in request.
A test requires custom component with article view. Unfortunately, we don't have such extension.

@AlterBrains thanks for the answer. I don't understand it, no test.

This pull request has been automatically rebased to 5.2-dev.

I have tested this item ✅ successfully on 53d3d3c

I have tested this item ✅ successfully on 53d3d3c

I've also tested it succesfully.

I have tested this item ✅ successfully on 53d3d3c

RTC

I fear that this change can lead to some implications for extension developers when they are using this class as the lookup array is now built differently. Is it possible to make it in a more backwards compatible way?

@laoneo I don't see any other way. We change the structure of protected RouteHelper::$lookup array which is only accessed directly by RouteHelper, the output of all other RouteHelper methods stays same and there are no changes for all extensions which use RouteHelper methods.

Don't think that there are any implications. Only if a developer extends RouteHelper and applies custom access to $lookup but I doubt that we have cases. Or at least they are absolutely rare.

It's a protected variable, so very likely that it gets used by extending classes in 3rd party extensions. The problem is that this break can't be used in extensions which for example have compatibility to J4.

What about having the normal lookup and when that one fails, use the one with the extension parameter? At least that's how I would do it.

The problem is, that it doesn't fail. If you have 2 components which have the view "category" and use their own category implementation, they (can) collide here and you have a hit in the lookup for category X, but it is actually the category X of component B instead of component A.

Difficult then, but yeah I see your point.

@Hackwar Hm.. but RouteHelper::getCategoryRoute() requires $extension argument, we can use it!

I can update patch and add optional $extension argument for lookupItem(), it will be used in call from getCategoryRoute().
lookupItem() will fill $extension for new RouterHelper instance and it won't use request via:

        if ($this->extension === null) {
            $this->extension = $app->getInput()->getCmd('option');
        }

Hence, RouterHelper::getCategoryRoute() will always use the correct $extension.

Agree?

The problem isn't the input to the methods, but the lookup table itself. The attribute is protected, but not private, so a class extending from this class could try to read $lookup and fail because the structure is wrong.

Ready. Anyway, it's a bug. I believe 3rd-party classes should adopt to bug fixes in the core, but it's not the core who adopts to unknown classes :(

We are still discussing this.

Please fix the codestyle.

@Hackwar Fixed, thanks for reporting!

I have tested this item ✅ successfully on b40f9ed

I have tested this item ✅ successfully on 5e0256a

Will it ever be solved?

Will it ever be solved?

@Majoroff Needs as always a second successfull test for a next step.

I have tested this item ✅ successfully on 5e0256a

We are still discussing this.

@Hackwar Any news about that?

RTC

Just merge it. It can't get worse than it is already...

This pull request has been automatically rebased to 5.3-dev.

@laoneo All done.

Sorry, but this here is still a bc break as it requires now the extension name in the array.

Unfortunately, I don't see any other way for now. I would prefer to fix a long-living bug which affects the CMS right now and produce the minor bc break with close-to-zero probability.

Let's look at this for 6.0 release. @AlterBrains can you rebase into 6.0 please?

@Bodge-IT Rebased.

( FYI, I removed the RTC label as it was rebased to 6 at @Bodge-IT's request and I couldn't test it for 6 with the RTC flag on it.)

( FYI, I removed the RTC label as it was rebased to 6 at @Bodge-IT's request and I couldn't test it for 6 with the RTC flag on it.)

Reminder: Patch tester has the advanced option that enables patch of RTC prs.

( FYI, I removed the RTC label as it was rebased to 6 at @Bodge-IT's request and I couldn't test it for 6 with the RTC flag on it.)

Reminder: Patch tester has the advanced option that enables patch of RTC prs.

Thanks for that - I didn't know that :( but I do NOW :p

I have tested this item ✅ successfully on 4213971

SEF URL off
same tag on Article, Article Category, and Contact

We discussed this in the maintainers meeting and as Allon already said it's still a b/c break.
Solution suggestion:

Don't touch the lookupItem method,
Create a new lookupItem method with a different name, proxy the lookupItem to the new method, Maybe lookupItemByExtension and create a new $lookup property like $lookupByExtension and use this but maintain a copy without extension in the $lookup property. Also a getExtension() method would make sense.

And deprecate $lookup and lookupItem.

an alternative could be to detect if a second argument has been provided to lookupItem and use it for the extension.
another way could be by removing the usage of the lookupItem method and directly create a object in the getCategoryRoute method.

or create a new getCategoryRouteByExtension and change the order of the parameters (swap $extension and $language) and remove the InvalidArgumentException check.

Many options, I prefer to clean up variant with new methods but maybe to radical. So the first variant should fit the need.

@HLeithner I have another idea for 6.0:

What if Joomla\Component\Tags\Site\Helper::getItemRoute() resolves SiteRouter from container and get component router via getComponentRouter() and next call new method getTaggedItemRoute()

getTaggedItemRoute() is added to Joomla\CMS\Component\Router\RouterBase and return empty string by default.

Next, on empty string, getItemRoute() will fall back to current behaviour calling CMSRouteHelper->getRoute()

So, we keep the b/c but custom components can define own getTaggedItemRoute() in own router and provide correct route.

It's very simple fix which paves the way for a custom component to control the route of tagged item.
getTaggedItemRoute() can be added to all core components as well, they will just utilize the existing RouterHelper:getXxxRoute methods.