[#34558] - [4.0] Don't trim trailing slash in SEF URL's of home menu items on multilingual sites
- Fixed in Code Base
- 25 Jun 2021
- Medium
- Build: 4.0-dev
- # 34558
- Diff
- richard67:4.0-dev-fix-routing-for-home-menu-items
User tests: Successful: Unsuccessful:
Pull Request for Issue #34523 .
Summary of Changes
Don't strip off the trailing slash for home menu items when SEF URL's are switched on.
Thanks to @brianteeman who pointed me to the right place of code with his PR #34538 .
Testing Instructions
Create a multlingual site and check that the URL's for
- home pages (item "Home" in "Main Menu" in the right sidebar when using Sample Data Blog)
- other pages one level below the home page (e.g. first item "Blog" in the "Main MenuBlog" in the header when using Sample Data Blog)
- other pages more than one level below the home page (e.g. "Help -> About your home page" in the "Main MenuBlog" in the header when using Sample Data Blog)
Test with different SEF settings in Global Configuration - Site.
Check also the rel="alternate" head links with your browser inspector.
Do the same checks for a monolingual site, except of the last one with the head links.
Test not only menu items belonging to com_content but also for other components, e.g. com_contact, or for some 3rd party extension.
Actual result BEFORE applying this Pull Request
See issue #34523 (description and later comments).
Expected result AFTER applying this Pull Request
Multilingual sites
Check of the head links: In all the below cases there should be rel="alternate" head links only for the other languages but not for the current one.
I. "Search Engine Friendly URLs" = "Yes", "Use URL Rewriting"= "Yes", "Add Suffix to URL" = "No":
Associations and links in mod_languages to the default (home) pages of different languages should be:
No trailing slash should be appended to non default pages:
- https://www.example.com/en/my-article
- https://www.example.com/en/my-category-blog
- https://www.example.com/en/whatever/whatever-else
...
II. "Search Engine Friendly URLs" = "Yes", "Use URL Rewriting"= "No", "Add Suffix to URL" = "No":
Same as I. except of the /index.php being appended after the domain (or the subfolder if Jooma is installed in a subfolder).
III. "Search Engine Friendly URLs" = "Yes", "Use URL Rewriting"= "Yes", "Add Suffix to URL" = "Yes":
Associations and links in mod_languages to the default (home) pages of different languages should be same as I. and II. before.
The .html suffix should be appended to non default pages:
- https://www.example.com/en/my-article.html
- https://www.example.com/en/my-category-blog.html
- https://www.example.com/en/whatever/whatever-else.html
...
IV. "Search Engine Friendly URLs" = "Yes", "Use URL Rewriting"= "No", "Add Suffix to URL" = "Yes":
Same as III. except of the /index.php being appended after the domain (or the subfolder if Jooma is installed in a subfolder).
V. "Search Engine Friendly URLs" = "No"
There should not be any difference between with and without this PR.
Monolingual sites
There should not be any difference between with and without this PR.
Documentation Changes Required
None.
| Status | New | ⇒ | Pending |
joomla-cms-bot
- | Category | ⇒ | Libraries |
| Labels |
Added:
?
|
||
| Title |
|
||||||
Adding the "Release Blocker" label as inherited from the issue.
In the absence of automated tests etc could you add some comments to the code explaining what is being done at each step to avoid any "corrections" in future
In the absence of automated tests etc could you add some comments to the code explaining what is being done at each step to avoid any "corrections" in future
@brianteeman Will add some comments and am just trying to get help for adding unit tests.
| Labels |
Added:
?
|
||
After installing patch, I created different banners for each language, but the same banner (english by default) always shows in the pages. Only if I assign a different client for each language's banners, the banners change on every language
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/34558.
After installing patch, I created different banners for each language, but the same banner (english by default) always shows in the pages. Only if I assign a different client for each language's banners, the banners change on every language
@Quiviro And without the patch it works as desired?
I think I might have to make changes in the parseSefRoute method, too, to get the language from a SEF URL right after the changes in buildSefRoute.
Update: And possibly the parseFormat needs some change too? That can become a rabbit hole.
| Title |
|
||||||
Back to draft. I have to check later or tomorrow if I need to make more changes.
Anyway this this is not related to #34523 and does not fix it that is the main problem in the router.
@joeforjoomla What do you mean with “not related”? @Quiviro ‘s comment? Or the fix of this PR?
@richard67 i mean that this PR does not fix the main bug in the J4 router to generate home page links in the format en.html, es.html, etc
@richard67 i mean that this PR does not fix the main bug in the J4 router to generate home page links in the format en.html, es.html, etc
@joeforjoomla You’ve obviously not tested it.
@richard67 obviously i've tested it. Nothing changes, menus have still same .html suffix for home page. So if it should have worked, i say you that it does not work.
@richard67 obviously i've tested it. Nothing changes, menus have still same .html suffix for home page. So if it should have worked, i say you that it does not work.
@joeforjoomla I don't know how you have tested but here links of home menu items of a multilingual site have no ".html" but a "/" at the end.
@richard67 you right my fault, it seems to have fixed the bug. Thanks and sorry.
As the submitter of the original issue #34523 I can confirm this PR fixes the issue!
I tested the multilingual setup with sef, sef_suffix, sef_rewrite in all combinations I could think of. Looks good.
I also tested a monolingual website with different combinations of sef-settings. No changes there.
Thanks @richard67 and others involved for this fix. Good work!
As the submitter of the original issue #34523 I can confirm this PR fixes the issue!
?
I tested the multilingual setup with sef, sef_suffix, sef_rewrite in all combinations I could think of. Looks good.
I also tested a monolingual website with different combinations of sef-settings. No changes there.Thanks @richard67 and others involved for this fix. Good work!
@elpescador-nl Unfortunately I'm not sure yet if this PR breaks other things, see the comment above. So it might be that I have to make changes. I'd be happy if you could test again after that. I'll let people know when it's ready for testing and will remove draft status then.
I have tested this item
Based on my limited testing everything seems to work as intended. (there should be unit tests to verify that)
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/34558.
| Labels |
Added:
?
Removed: ? |
||
@brianteeman Sorry, just when you had tested I have pushed a change. But that should not change how it works, it's only cleaner code. But I'm still not sure if not anything else is broken.
| Title |
|
||||||
After installing patch, I created different banners for each language, but the same banner (english by default) always shows in the pages. Only if I assign a different client for each language's banners, the banners change on every language
@Quiviro I can't reproduce your finding when using for each language a separate banner module and for each banner a separate category, all in the right language. Did you have only different banner for the different languages but used the same banner category with language = "All" for them? And did you use one and the same module for the banners with language "All", or did you use separate modules for each language? Please report back how you did get your issue and if you get it also without this PR.
@brianteeman Could you just briefly test again if it still works?
@elpescador-nl Could you also test again and report back the test result by going to the issue tracker here https://issues.joomla.org/tracker/joomla-cms/34558 , using the "Test this" button at the top left corner, then selecting the appropriate test result and then submitting? Thanks in advance.
I have tested this item
I tested...
a multilingual site:
- without sef: no changes
✅ - sef:
example.com/index.php/en/example.com/index.php/en/article✅ - sef + sef_rewrite:
example.com/en/example.com/en/article✅ - sef + sef_rewrite + sef_suffix:
example.com/en/example.com/en/article.html✅
a monolingual site:
- without sef: no changes
✅ - sef: no changes
✅ - sef + sef_rewrite: no changes
✅ - sef + sef_rewrite + sef_suffix: no changes
✅
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/34558.
Sorry @richard67 now I can't reproduce the issue with the banners.
Yesterday I created one banner and one module for every language, no client, no category. Only the default language banner shows, even when I switch languages.
Now, all the banners show in every language, although every module and banner has only one language assigned. Same behaviour with or without the patch.
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/34558.
Sorry @richard67 now I can't reproduce the issue with the banners.
Yesterday I created one banner and one module for every language, no client, no category. Only the default language banner shows, even when I switch languages.
Now, all the banners show in every language, although every module and banner has only one language assigned. Same behaviour with or without the patch.
@Quiviro Thanks for reporting back. Good to know it's not related to this PR.
Anyway, the five expected results in multilingual seems to works well
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/34558.
| Title |
|
||||||
@Hackwar Could you
-
Have a look on this PR and also on the referenced issue #34523 (description and comments) and either review my PR here on GitHub or let me know by a comment if it is in general right?
Maybe you don't like it because you would prefer the "/en.html", "/de.html" and so on and the easier code without this PR, but please keep in mind that it's currently partly broken because in some case you still get redirected e.g. to "/en/", "/de/" and so on. -
The issue and this PR show the need for unit tests for the router, but not only for the site router. If I knew how and there to start for the site router, I could then add the necessary test cases. Could you help me with that?
Thanks in advance.
I tried to compare with J3 code and found the three lines which causes the issue:
https://github.com/joomla/joomla-cms/blob/4.0-dev/libraries/src/Router/SiteRouter.php#L485
https://github.com/joomla/joomla-cms/blob/4.0-dev/libraries/src/Router/SiteRouter.php#L496
https://github.com/joomla/joomla-cms/blob/4.0-dev/libraries/src/Router/SiteRouter.php#L503
In the first two lines of code, we always add / character to the variables, thus we need the trim command in the third line to remove the / character and it causes the error. In J3, we only add that character if $tmp is not empty.
To be safe, I think we can use the same logic with J3 (which has been working for years). The modified code could be https://github.com/joomla/joomla-cms/compare/4.0-dev...joomdonation:routing_fix?expand=1 (less changes, same with J3 logic and it works).
@joomdonation Maybe you should make a new PR with your suggested code?
Update: Clarified on Glip. I've just made the changes here.
| Labels |
Added:
?
Removed: ? |
||
@brianteeman @elpescador-nl Could you test again? Thanks in advance.
I have tested this item
I've tested again with the latest commit with the same successful results for both multilingual as well as monolingual.
Thanks for all the efforts.
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/34558.
I have tested this item
Tested on both mono language and multilingual website, it fixed the issue and everything else is still working as before.
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/34558.
| Status | Pending | ⇒ | Ready to Commit |
RTC
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/34558.
| Status | Ready to Commit | ⇒ | Fixed in Code Base |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2021-06-25 16:33:18 |
| Closed_By | ⇒ | wilsonge | |
| Labels |
Added:
?
Removed: ? |
||
Thanks!
Thanks all!
Drone failure seems not to be related to this PR but a false alarm from RIPS.