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

[5.2] Multilang: Remove restrictions for home language menu items #43862

Merged
merged 10 commits into from
Aug 18, 2024

Conversation

Hackwar
Copy link
Member

@Hackwar Hackwar commented Jul 29, 2024

Summary of Changes

The multilanguage system currently requires each home menu item to be in a separate menu, making the setup unnecessarily complex. There is no reason for forcing this behavior and this PR removes that. With this PR, the restriction is removed and you can put all home menu items into one menu. In the backend menu, the menu listing is changed to display the home icon for menus with more than one home menu item.

Testing Instructions

Setup a multi language website and try to save all home language menu items in one menu.

Actual result BEFORE applying this Pull Request

Error when saving.

Expected result AFTER applying this Pull Request

No errors.

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

@brianteeman
Copy link
Contributor

I created a multilingual site with 4 languages and the multilingual sample data.
I moved all the menu items to one menu and deleted the other menus

That all worked as expected.

It just left the check in the status module to be updated as the information is no longer correct

image

@coolcat-creations
Copy link
Contributor

Tested Successfully but 2 things to consider:

  1. Should there be another Sample Data Mode for the menus?
  2. When I filter the menu items by language the nesting is not displayed correctly, should there be another filter ?

@brianteeman
Copy link
Contributor

When I filter the menu items by language the nesting is not displayed correctly, should there be another filter ?

Can you post a screenshot as it looks ok to me
image

@coolcat-creations
Copy link
Contributor

It looks like this after filtering altough its nested:
grafik

@brianteeman
Copy link
Contributor

Unable to replicate (or i dont understand)

image

@coolcat-creations
Copy link
Contributor

indeed - yours is still nested -mine not

@brianteeman
Copy link
Contributor

indeed - yours is still nested -mine not

please post the full screenshot

@coolcat-creations
Copy link
Contributor

Sorry was my mistake, the nested Item was set to "all"
I am still confused that all is not really "All" but just "All" - I mean if something is set to all it should apply for every other language too.

@coolcat-creations
Copy link
Contributor

I have tested this item ✅ successfully on bc7029b

Works as expected.


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

@brianteeman
Copy link
Contributor

I have made a pr to your branch for the multilangstatus issue mentioned above

@joomla-cms-bot joomla-cms-bot added the Language Change This is for Translators label Jul 30, 2024
@brianteeman
Copy link
Contributor

I have tested this item ✅ successfully on 1fd1d4e


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

@Quy Quy removed the Language Change This is for Translators label Jul 30, 2024
@Quy
Copy link
Contributor

Quy commented Jul 30, 2024

RTC


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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Jul 30, 2024
@Quy Quy added Language Change This is for Translators RTC This Pull Request is Ready To Commit and removed RTC This Pull Request is Ready To Commit labels Jul 30, 2024
@pe7er pe7er enabled auto-merge (squash) August 10, 2024 12:34
@pe7er pe7er self-assigned this Aug 10, 2024
@pe7er pe7er disabled auto-merge August 10, 2024 15:20
With the PR there are two exceptions if as database is PostgreSQL is used.
`ERROR: column "m.language" must appear in the GROUP BY clause or be used in an aggregate function`

If the columns in GROUP BY are simple added again, there are multiple
menu entries if multiple home set in one menu.

Solution is to use the aggregat function MAX.

Tested with
* PostgreSQL PDO
* MySQL with MySQLi
* MySQL with MySQL PDO
* MariaDB with MySQL PDO
* MariaDB with MySQLi

On base of
* PR 43862 with Patch Tester
* npm update joomla-cypress 1.1.1
* this PR fix-postgres
* npx cypress run --spec tests/System/integration/install/multi-lang-menu.cy.js
Test Support PR 43862 with
✓ install Japanese language pack  (9784ms)
✓ install Ukrainian language pack (8487ms)
✓ install German language pack (8309ms)
✓ enable plugin 'System - Language filter' (1903ms)
✓ create '2nd menu' (1484ms)
✓ create 1st menu entry de as HOME in '2nd menu' (5601ms)
✓ create 2nd menu entry ja as HOME in '2nd menu' (4936ms)
✓ create 3rd menu entry uk as HOME in '2nd menu' (5392ms)
✓ create '3rd menu' (1434ms)

* checked five times that the HOME symbol is displayed if more than one Home menu entry is set,
  otherwise the correct language (uk-UA, ja-JP or de-DE) is displayed for each of the three entries
  if there is only one menu entry with Home
@Hackwar
Copy link
Member Author

Hackwar commented Aug 13, 2024

We had an issue with PostgreSQL in the admin menu, but thanks to @muhme this is now fixed. @brianteeman @coolcat-creations would you be able to test this again? Basically it would be enough to make sure that the backend menu still functions as expected.

@Hackwar
Copy link
Member Author

Hackwar commented Aug 16, 2024

Back to pending.


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

@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Aug 16, 2024
@brianteeman
Copy link
Contributor

I have tested this item ✅ successfully on 7d82dcd

still ok


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

@fgsw
Copy link

fgsw commented Aug 18, 2024

On a nightly build:

  1. Installation Multilingual Sample Data (en-GB, de-DE)
  2. Installation Testing Sample Data
  3. Move all Menu items of both languages to "Main Menu"

See Icon "Home Default" (white House) in "Main Menu Testing" and "Main Menu":

Untitled 2

Test System Information:
PHP Built On Darwin Air.local 23.6.0 Darwin Kernel Version 23.6.0: Mon Jul 29 21:13:00 PDT 2024; root:xnu-10063.141.2~1/RELEASE_X86_64 x86_64
Database Type mysql
Database Version 8.0.35
Database Collation utf8mb4_unicode_ci
Database Connection Collation utf8mb4_0900_ai_ci
Database Connection Encryption None
Database Server Supports Connection Encryption Yes
PHP Version 8.3.8
Web Server Apache/2.4.58 (Unix) OpenSSL/1.1.1u mod_fastcgi/mod_fastcgi-SNAP-0910052141
WebServer to PHP Interface cgi-fcgi
Joomla! Version Joomla! 5.2.0-alpha4-dev Development [ Uthabiti ] 23-July-2024 16:01 GMT
Joomla Backward Compatibility Plugin Disabled
User Agent Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:129.0) Gecko/20100101 Firefox/129.0

@Hackwar
Copy link
Member Author

Hackwar commented Aug 18, 2024

@fgsw That is the expected behavior. If a menu contains more than one home menu item, it should show the house icon next to it. We can't really display the different flags, since that would already get ugly with 2 flags.

If you could mark this as tested successfully, then I would try to still get this into beta 1.

@fgsw
Copy link

fgsw commented Aug 18, 2024

I have tested this item ✅ successfully on ea5b52c

@Hackwar Thanks for comment. I thought the icon is only for home-menu-item-all languages.


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

@Hackwar
Copy link
Member Author

Hackwar commented Aug 18, 2024

RTC


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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Aug 18, 2024
@bembelimen bembelimen merged commit 638d7b0 into joomla:5.2-dev Aug 18, 2024
2 of 3 checks passed
@bembelimen
Copy link
Contributor

Thx

@alexandreelise
Copy link
Contributor

Awesome work Super Joomlers! Thanks for this feature

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Language Change This is for Translators PR-5.2-dev
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants