User tests: Successful: Unsuccessful:
#5105 changed the way that tag URLs are returned when a tag has a custom menu item
Test English
sample data. Friendly URLs
and URL rewrititing
. This can be tested also without URL rewriting but the URLs will contain the index.php
part: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.
Apply the patch and just reload the home page. The link should be correct now.
This tries to fix an already existing B/C issue that will change existing sites URLs.
Please @Hackwar can you review this? Thanks!
Labels |
Added:
?
|
Sorry, but if it simply adds the tag to the URL, then the router is wrong. The helper is perfectly fine here.
Yes, but that still means that either the _findItemid() method or the router is broken, not the code that you are changing.
Well it was working like this before changing it at:
https://github.com/joomla/joomla-cms/pull/5105/files#diff-3b871ffd316afe16287f977848af4c5cL91
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.
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
$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'])
Yes, @joomdonation is correct. That still does not fix this correctly for menu items with multiple tags, but that is something for another time...
@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?
Pull request updated. I finally had to tweak both router and the findItem() method.
This should fix both issues:
Category | ⇒ | Tags |
Able to reproduce then:
#6291 works as describe also if there're more than 1 tag
thanks
Status | Pending | ⇒ | Closed |
Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2015-03-07 09:35:37 |
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.
@joomdonation have you tested your suggestion? I had to add the position because the $tagId
was not working so I doubt it works.
Yes. I tested. Strange it didn't work in your case. I see no difference between $item->query['id'][$position] and $tagId.
$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.
Sorry, yes, you are right, there shouldn't be a difference between the two. I misunderstood you there.
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.
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.
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).
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".
Sorry Hannes. I still don't get your point :D. Just want to make sure we are talking about the same thing:
The code I am talking about is in the _findItem method.
I am talking about how to find the correct menu item (Itemid) use to link to single tag.
Lets say we have two tags: foo and bar.
We then create two menu items: One choose both foo and bar tags. One just choose foo tag only
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
When we try to find Itemid for the link to the bar tag, it should return ID of the first menu item because:
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.
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:
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.
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).
@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:
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
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.
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 ?
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".
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.
@joomdonation See my mail to the mailinglist. Short: com_tags should be dropped asap
@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:
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.
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...
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.
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.
As I wrote on the mailinglist: Decouple it, move it to a seperate project and do whatever you want to do there.
Milestone |
Added: |
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
@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.