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

Tags router: fix Lookup when multilang is enabled #40460

Closed
wants to merge 4 commits into from

Conversation

Fedik
Copy link
Member

@Fedik Fedik commented Apr 23, 2023

Pull Request for Issue #40454 .

Summary of Changes

Adjust buildLookup to handle multilanguage correctly.
To behavior similar to MenuRules

Testing Instructions

Please follow #40454,
Or, on test installation, enable multilanguage, and check links for tags at home page

Actual result BEFORE applying this Pull Request

The links starts with /component/....

Expected result AFTER applying this Pull Request

The links linked to existing tag menu

Link to documentations

Please select:

  • Documentation link for docs.joomla.org:
  • No documentation changes for docs.joomla.org needed
  • Pull Request link for manual.joomla.org:
  • No documentation changes for manual.joomla.org needed

@richard67
Copy link
Member

richard67 commented Apr 23, 2023

Hmm, the PR doesn't work for me as described in the issue.

On a clean 4.2.9 with multilingual and a menu item of type "Tagged items" in one of the published languages, only those tags which have been selected in that menu item's settings have a link without the "component" thing but to that menu item's page in popular tags. All other tags have the "component" link.

On a 4.3-dev with this patch applied all tags in popular tags have a link without "component". All links point to the "Tagged items" page. The tags which have been selected in that menu item's settings just link to that page, e.g. https://www.joomla-43-dev.vmubu01.vmnet2.local/index.php/de/test-tagged-items-menu-item, and the others with their id number, e.g. https://www.joomla-43-dev.vmubu01.vmnet2.local/index.php/de/test-tagged-items-menu-item/2.

I'm not sure if I have done everything right in my test, so I'm not posting an unsuccessful test result. But maybe @MacJoom could check it, too, as he was the author of the issue.

@MacJoom
Copy link
Contributor

MacJoom commented Apr 23, 2023

I did a quick test on the site where i reproduced the issue - i have quite the opposite result - none of the 'component' is gone but the idemid is gone. So i had http://bug4-2.test/joomla-cms/index.php/en/component/tags/tag/millions-en-gb?Itemid=104 before the patch and now i have http://bug4-2.test/joomla-cms/index.php/en/component/tags/tag/millions-en-gb before update to 4.3.0 i had http://bug4-2.test/joomla-cms/index.php/en/tagged-content/millions-en-gb - and i double checked that i applied patch 40460 and not 40455 (not applied). Do i need to apply other patches? - Will have another look tomorrow. Thank you

@Fedik
Copy link
Member Author

Fedik commented Apr 23, 2023

Maybe something missed in the tests?

In my test I open /en/popular-tags for installation with default "Test data", and have following:

---- 4.2
/en/compact-tagged Green
/en/tagged-items Yellow
/en/all-tags/red Red
/en/all-tags/lime Lime

---- 4.3-dev (with recent changes)
/en/component/tags/tag/green Green
/en/component/tags/tag/yellow Yellow
/en/component/tags/tag/red Red
/en/component/tags/tag/lime Lime

---- 4.3-dev after patch
/en/compact-tagged Green
/en/tagged-items Yellow
/en/all-tags/red Red
/en/all-tags/lime Lime

@richard67
Copy link
Member

@Fedik I haven't used testing sample Data. I have uses multilingual sample data (installed once) plus blog sample data (installed for each language by changing backend language for each installation). I will be out now so I can't do further checks. Maybe later tonight.

@richard67
Copy link
Member

@MacJoom Your result I get when being on the home page. I can reproduce that. When being on the "Tagged Items" page for which I have created a menu item, I get the results which I have described in my comment.

@Fedik
Copy link
Member Author

Fedik commented Apr 23, 2023

Hmhm, can you write a structure of your testing menu?
I have this:

Path Menu type Selected tag
/en/all-tags "List All Tags"
/en/compact-tagged "Compact List of Tagged Items" selected "Green" tag
/en/tagged-items "Tagged Items" selected "Yellow" tag

With this I have results like I wrote #40460 (comment)

When I unpublish "/all-tags" then after patch I get:

/en/compact-tagged Green
/en/tagged-items Yellow
/en/component/tags/tag/red Red
/en/component/tags/tag/lime Lime

@richard67
Copy link
Member

@Fedik I haven't used testing sample data. I have used multilingual sample data plus for each language blog sample data. These kinds of sample data do not include any menu item of the tags component. Then I have added a new menu item of type "Tagged items" and have set that menu item to German language. Then I checked on the German home page, where I get the result described by @MacJoom , and I tested when going to the page of that "Tagged items" menu item, then I get my results.

@Fedik
Copy link
Member Author

Fedik commented Apr 23, 2023

There really something mixed up :)

This part works okay for me:

Do an update to 4.3
Check the links in popular tag. 'component' is back for all the links

I mean, now, after the patch, the Tags (wich have a menu) have a links without /component.

And when I do "Tagged items" only for German, then I get:
/de/tagged-items for German language
/en/component/tags/tag/lime for English language
That is also correct.

@MacJoom
Copy link
Contributor

MacJoom commented Apr 24, 2023

@Fedik - i just found that it makes a difference if you have just one tagged item in the menu or more than one. The link on the home page is correct if you have just one tagged item in the menu (http://bug4-3.test/joomla-cms/index.php/en/tagged-items) but if you have at least two items in the menu tagged-items, the links will have 'component' in it. - Thank you for your effort.

@MacJoom
Copy link
Contributor

MacJoom commented Apr 24, 2023

Just for clarification: In 4.2.9 sample data installed, multilanguage, with a menu for tagged content (alias tagged-content) with 1 tag (millions), on the home page you will get the link /en/tagged-content (not more). with 2 tags however (millons and love) you will get /en/tagged-content/millions-en-gb and /en/tagged-content/love-en-gb. After an update to 4.3.0: if you have 2 Tags in the menu the link for this tags have changed from 4.2.9 including 'component' (and with the itemid, other issue) . With just 1 tag you get the same link as in 4.2.9 (even without the itemid) - the problem arises only if you have more than one tag in the tag menu - hth

@Fedik
Copy link
Member Author

Fedik commented Apr 24, 2023

I have updated the patch, now it should work with multiple tags in menu.

@MacJoom
Copy link
Contributor

MacJoom commented Apr 24, 2023

Something changed. But the links are still not the same as in 4.2.9. If i have tagged items 'millions' and 'love' in the tagged content menu, both links now go to /en/tagged-content. in 4.2.9 the links are different. /en/tagged-content/millions-en-gb and /en/tagged-content/love-en-gb


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/40460.

@Fedik
Copy link
Member Author

Fedik commented Apr 24, 2023

Okay, so there a menu: /tagged-content wich have selected tag millions (alias millions-en-gb) and love (alias love-en-gb), right?

For 4.2
millions linked as /tagged-content/millions-en-gb
love linked as /tagged-content/love-en-gb

And after my path, they both:
millions linked as /tagged-content/
love linked as /tagged-content/

And when ignore the last commit:
millions linked as /component/tags/tag/millions-en-gb
love linked as /component/tags/tag/love-en-gb

Hmhm, it sounds that my last commit is wrong, but 4.2 behavior is strange.
Maybe that was actualy a bug that was fixed in #39114 ?

@MacJoom
Copy link
Contributor

MacJoom commented Apr 24, 2023

The findings in your last comment are correct. But the behaviour in 4.2.9 looks correct to me. The links lead to two different pages one for the tag 'millions' and one for the tag 'love'. #39114 broke this behaviour.

@Fedik
Copy link
Member Author

Fedik commented Apr 24, 2023

But the behaviour in 4.2.9 looks correct to me. The links lead to two different pages one for the tag 'millions' and one for the tag 'love'

It is strange behvaior, I would say 😉
I would agree that it is correct, when tags would be nested, like:

- million
-- child of million

And link would be

/tagged-content/child-of-million

To me correct paths is:
/list-view/single-view for single Item when there a manu for Items list;
/component/list-view/single-view for single Item when manu for Items list does not exists;
/single-view for single Item when there a manu for it;

What 4.2 have is: /single-view/single-view

tbh, I do not know, what else I can do here.
At least it works for "signle selected" tag.

Maybe @Hackwar can give some hint, or agree or disagree with me :)

@MacJoom
Copy link
Contributor

MacJoom commented Apr 24, 2023

I may look strange - but it was like this in 4.2 and we cannot change this in 4.3


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/40460.

@tramber91
Copy link

As request by @richard67 i copy my last post in this thread

Did the test concerning Batch 2 but same issue (Not working on my side)

same link (see below after the batch)
/fr/component/tags/tag/france?Itemid=106

should be (like in Joomla version 4.2)
/fr/tags-pays/france

I add/Change new codes in initial Router.php file (4.3.0)

i also did the test with SEF diseable
Without alias
result is
/index.php?option=com_tags&view=tag&id[0]=7:france&Itemid=106&lang=fr
Should be
/index.php?option=com_tags&view=tag&id[0]=7:france&Itemid=2622&lang=fr

the router not found the menu id 2622 (Menu Tags: All tags parent Id Countries (Pays))
https://biomecanique.en-toutes-lettres.fr/fr/tags-pays?parent_id=4
If you start from this link (above), you select Tag "France", you have list of article with tag "France"
You select one article and click on France tag
You move to Home page instead of page with tag="France "we have just seen before.

I did also the test including previous batch modification. Same think but without the ?Itemid=106

@richard67
Copy link
Member

As request by @richard67 i copy my last post in this thread

The original comment in the issue was from 9 hours ago, so that test did not include the last 2 or so commits.

@Fedik
Copy link
Member Author

Fedik commented Apr 24, 2023

that test did not include the last 2 or so commits.

Can ignore them, I have added one then reverted it :)

@Hackwar
Copy link
Member

Hackwar commented Apr 24, 2023

Please have a look at #40474. The problem isn't the buildLookup(), but the preprocess method.

@richard67
Copy link
Member

I know. I only wanted to make the time context of that comment clear.

@Fedik
Copy link
Member Author

Fedik commented Apr 24, 2023

@Hackwar I see you made the same, but in another way ;)
The diference that you use

foreach ([$lang, '*'] as $language) {

And I use approach from

$attributes[] = 'language';
$values[] = [$language, '*'];
$items = $this->router->menu->getItems($attributes, $values);
foreach ($items as $item) {

With this, the buildLookup() builds the lookup array for each language.

But maybe I missed something.

However there some other issue, probably with build(), some URL produce diferent path than it was in 4.2 even if Itemid are found.

@Hackwar
Copy link
Member

Hackwar commented Apr 24, 2023

The current buildLookup() builds the whole lookup table at once instead of doing it language by language upon request. There is no performance gain in doing it lazy like you proposed. The problem is not creating the lookup, but actually looking up the right entry in the lookup table. In preprocess, we are overwriting $language with the default language when the language is *, which means that all tag menu items with language * are ignored.

@MacJoom
Copy link
Contributor

MacJoom commented Apr 26, 2023

HI @tramber91 could you test #40474 - and please report if your links still break. Thank you very much

@Fedik Fedik deleted the tags-route-lookup branch April 28, 2023 14:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants