[fix] wrong link when tag is in a menu

[phproberto]

Description

#5105 changed the way that tag URLs are returned when a tag has a custom menu item

How to reproduce the issue

1. Install joomla with Test English sample data.
2. In global configuration enable Friendly URLs and URL rewrititing. This can be tested also without URL rewriting but the URLs will contain the index.php part:

1. Duplicate htaccess.txt and rename the new file to .htaccess.
2. On main menu create a new menu item of type Tags > Tagged items selecting the Green tag and Article as content type:

3. Reload the main page. You will see the new menu link in the Main menu. If you put your mouse over the Green tag you will see that the URL shown is http://localhost/jcms3x/tag-test/4-green (being tag-test the menu alias). This is not correct because it should be (and it was before v3.4) http://localhost/jcms3x/tag-test. The issue is that now we are adding the Itemid to the URL instead of using plain index.php?Itemid=xxx if a menu item is found for this tag.

How to test the fix

Apply the patch and just reload the home page. The link should be correct now.

Backward compatibility issues

This tries to fix an already existing B/C issue that will change existing sites URLs.

Additional comments

Please @Hackwar can you review this? Thanks!

[n9iels]

@test I'm not able to reproduce the issue. When I create the menu item, the link was: /joomla-cms/tag-test
I tried to reproduce this issue a MAMP server, with PHP 5.6 and the staging branch of Joomla!

---

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

[Hackwar]

Sorry, but if it simply adds the tag to the URL, then the router is wrong. The helper is perfectly fine here.

[phproberto]

Maybe I haven't explained it correctly. If you have a tag in a menu item you expect that when that tag appears the link points to the tag menu item URL.

This is what happens with current staging:

[Hackwar]

Yes, but that still means that either the _findItemid() method or the router is broken, not the code that you are changing.

[phproberto]

Well it was working like this before changing it at:
https://github.com/joomla/joomla-cms/pull/5105/files#diff-3b871ffd316afe16287f977848af4c5cL91

[Hackwar]

No, it didn't. It worked randomly, since you can see that we created the array once with the view and then again checked for the language, then we said "screw all that" and again used the view. What they did there was so broken that they used that shortcut that was used outside of _findItemid(), which has further issues. But PLEASE don't treat the sympthoms, treat the disease. And that is the broken _findItemid() method.

[phproberto]

Ok. I'll try to see if it can be fixed removing the id segment in the router. I think the _findItem() method is returning the right itemid but the url is not the same than the menu

[joomdonation]

$mId variable in this line of code https://github.com/joomla/joomla-cms/blob/staging/components/com_tags/router.php#L67 is an array (because you can choose different tags for the menu item), so that command never return true and it causes the issue.

I believe change code in that line with the code below will solve the issue:

if ($mView == $view && isset($query['id']) && is_array($mId) && count($mId) == 1 && $mId[0] == $query['id'])

[Hackwar]

Yes, @joomdonation is correct. That still does not fix this correctly for menu items with multiple tags, but that is something for another time...

[joomdonation]

@phproberto Could you check the code I proposed to see whether it works ? if it works, could you update this PR with the new code so that we can get this issue fixed?

[phproberto]

Pull request updated. I finally had to tweak both router and the findItem() method.

This should fix both issues:

• If a tag menu item only contains a single tag the tag id is not set in the URL.
• If a tag menu contains more than one tag the right Itemid is returned for both and the id is added to the URL.

[MATsxm]

@test

Able to reproduce then:
#6291 works as describe also if there're more than 1 tag

thanks

---

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

[joomdonation]

I think $item->query['id'][$position] in these lines of code could be changed to $tagId.

[joomdonation]

@test: Success. The PR solves the original issue and more. It can detect menu item in case there is more than one tag selected for the menu item.

[joomdonation]

Hmm. Abit more clean up as I commented before merge would be better. The code $item->query['id'][$position] in this line (and line 152)
https://github.com/joomla/joomla-cms/blob/staging/components/com_tags/helpers/route.php#L150

should be replaced with $tagId. Same result but shorter and easier to read.

[phproberto]

@joomdonation have you tested your suggestion? I had to add the position because the $tagId was not working so I doubt it works.

[joomdonation]

Yes. I tested. Strange it didn't work in your case. I see no difference between $item->query['id'][$position] and $tagId.

[Hackwar]

$position is the index of the array, while $tagId is the ID of the tag. While the first one will count from 0 up, the other might be 100000000 already. So they are VERY different.

[joomdonation]

@Hackwar So you are saying $item->query['id'][$position] and $tagId might have different value?

[Hackwar]

Sorry, yes, you are right, there shouldn't be a difference between the two. I misunderstood you there.

[joomdonation]

OK. Thanks. I think we can make that for each block simpler with:

if (isset($item->query['id']) && is_array($item->query['id']))
{
    foreach ($item->query['id'] as $tagId)
    {
        if (!isset(self::$lookup[$lang][$view][$tagId]) || count($item->query['id']) == 1)
        {
            self::$lookup[$lang][$view][$tagId] = $item->id;
        }
    }
}

But I guess it is little thing, so if you don't want to make change, that's fine.

[Hackwar]

But that doesn't take into account the menu items with more than one tag. Basically, if you have a menu item with the tags "foo" and "bar", then a URL with tag "foo" would be associated with that menu item.

[joomdonation]

Hmm, it does (at least from my test). And the code I wrote above should be the same with the code was merged (except it is a bit more clear).

[Hackwar]

The code that you proposed above will assign the itemid to each tag that is associated with the menu item. So if you have &Itemid=42&tag[0]=foo&tag[1]=bar, then you will have a lookup array that looks like this: array('foo' => 42, 'bar' => 42). If I now look up a URL with tag "foo", it will automatically only point to Itemid 42, which is wrong, since it would have to be "foo" and "bar".

[joomdonation]

Sorry Hannes. I still don't get your point :D. Just want to make sure we are talking about the same thing:

1. The code I am talking about is in the _findItem method.
2. I am talking about how to find the correct menu item (Itemid) use to link to single tag.
3. Lets say we have two tags: foo and bar.
4. We then create two menu items: One choose both foo and bar tags. One just choose foo tag only
5. When we try to Itemid for the link to the foo tag, it should return ID of the second menu item since we have a menu item which link directly to that tag
6. When we try to find Itemid for the link to the bar tag, it should return ID of the first menu item because:
7. We don't have any menu item to link directly to bar tag
8. We have a menu item which link to different tags and one of them contain bar tag

If that's expected result (and I see that's the code @phproberto implemented - and I tested it with that direction), then the code I proposed above give the same result - just a bit shorter.

I just want to make sure the code is correct. If you see that it is correct, please don't waste much time explain to me.

[Hackwar]

No, that is not correct. Based on that, the result of the link would point to a page that displays content only for both "foo" and "bar". Let me do a similar list:

1. We have the tags "foo" and "bar".
2. We have an article that is tagged "foo", one that is tagged "foo" and "bar" and one that is only tagged "bar".
3. We have a menu item that links to "foo" and "bar".
4. We now try to find a menu item for the tag "foo" (or for "bar", doesn't matter). In that case, we have to say that there is no menu item, because we only have a menu item for "foo" AND "bar".
5. We now search for "foo" and "bar" and that will result in a URL to the above mentioned menu item and that page will only display the article tagged "foo" and "bar".
6. We now add a second menu item that links to "foo" and that displays both the article for "foo" and for "foo" and "bar".

What we would have to do is to create a unique key from the tags that are requested, like

$key = md5(strtolower(implode('|', $tags)));

and use that both for creating the lookup array and for looking up the actual value. Otherwise menu items with more than one tag will not be properly assigned.

I guess your test worked fine because you created the single-tag-menu later and thus now when it is loaded, that menu item overwrites the other menu item in the lookup table. Right now we are always only comparing one tag with another, not a set of tags against a set of tags.

[Hackwar]

I've done some code in #6358, but at this point I would rather remove com_tags completely. This is a huge pile of horseshit.

[brianteeman]

Please mind your.language on the public lists.
On 7 Mar 2015 14:20, "Hannes Papenberg" notifications@github.com wrote:

I've done some code in #6358
#6358, but at this point I
would rather remove com_tags completely. This is a huge pile of horseshit.

—
Reply to this email directly or view it on GitHub
#6291 (comment).

[joomdonation]

@Hackwar With the menu item which you created to link to "foo" and "bar", when users click on it, do you expect that only item tagged with both two tags returned or any articles which is tagged with one of the two tags? If the later case is correct, then the code seems still correct to me:

1. Click on the menu item (which has the link http://localhost/jcms/index.php/foo-and-bar-tag), I see 3 articles: Bar article, Foo and bar article, Foo article
2. Now the bar tag has will has the following link http://localhost/jcms/index.php/foo-and-bar-tag/9-bar-tag (it uses Foo And Bar Tag menu item as active menu item). Click on that tag I see two articles: Bar article, Foo and bar article.
3. Same for foo tag, it has the following link http://localhost/jcms/index.php/foo-and-bar-tag/8-foo-tag (still uses Foo And Bar Tag menu item as active menu item) and has two articles: Foo and bar article, Foo article)

Seems it is correct ?

[Hackwar]

The idea for tags is to filter your data further down, so if you have two tags, your results should always have to include both tags.

I have never seen a tagging implementation where 2 tags return MORE results than just one tag. When I'm searching on google for "Joomla installation", I don't expect to get all pages that contain the word "Joomla" and all pages that contain the word "installation" in them, regardless if it is about a plumbing installation, but I expect the pages that contain both the word "Joomla" and "installation".

[joomdonation]

Seem that is not how tags works in Joomla for now. So I don't see anything wrong with the code which I proposed. If there is anything wrong, then it is how tag is implemented :D.

[Hackwar]

@joomdonation See my mail to the mailinglist. Short: com_tags should be dropped asap

[joomdonation]

@Hackwar I understand your feeling. I even see some other strange code in com_tags which I discussed with @phproberto this morning. However, I don't think we can / should drop it for now:

1. I think it is not easy to de-couple or remove it as other extensions like weblinks, contacts (core and third party extensions rely on it). If you want to remove it, you will have to have com_tags 2.0 ready :D.
2. I don't think we have enough resources to re-write it for now. There are more important things which we should focus on first such as your new router, new MVC, new Media manager...
3. End users still use it. Seem I don't see issues report about how tag works. As a third party extensions developer, I see my customers ask me to integrate with Joomla core tags. So I think it is OK for end-users to use it that way.
4. So for now, I think we can fix bugs, clean code ... to improve it as much as possible. And we will have to live with it now until there is replacement.

[Hackwar]

As I wrote on the mailinglist: Decouple it, move it to a seperate project and do whatever you want to do there.

[sitthykun]

I don't test it before but now I am facing this problem even I are using 3.4.6.
Unfortunately, I don't see any solution yet.
My idea, tag url should not store full url; just store tail of url it will solve this.

Sorry if I confused what you were solved the problem.

Thanks

---

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