[#10756] - [com_menus] improve alias check and generation
- Fixed in Code Base
- 15 Jun 2016
- Medium
- Build: staging
- # 10756
- Diff
- andrepereiradasilva:menu-alias
- Success continuous-integration/travis-ci/pr The Travis CI build passed Details
User tests: Successful: Unsuccessful:
Summary of Changes
This PR makes several changes on the menu alias generate process.
- Improves alias generation
- Improves alias conflict detection
Also solves some issues in the current alias generate process:
-
In a monolingual site (language filter plugin disabled), create a menu item with the same alias as other menu item you have in the same/other menu root but with different language.
You'll notice it will save with success but it shouldn't.
-
In a multilingual site (language filter plugin enabled), create a menu item with the same alias as other menu item with All language you have in the same/other menu root but in a different language.
You'll notice it will save with success but it shouldn't.
-
Create a menu item of System Links -> Any type
You'll notice the generate alias is the current date, not menu item title.
Testing Instructions
This needs to be tested with the proper attention.
- Use latest staging and apply patch
- Install one adicional language (example: fr-FR)
- Create the following menu structure:
Menu Type 1:
- Menu item A: Language All, Alias: menu-item-a
- Menu item B: Language en-GB, Alias: menu-item-b
- Menu item C: Language fr-FR, Alias: menu-item-c
Menu Type 2:
- Menu item D: Language All, Alias: menu-item-d
- Menu item E: Language en-GB, Alias: menu-item-e
- Menu item F: Language fr-FR, Alias: menu-item-f
- Apply patch
- Set the site to monolingual (Disable Language Filter System plugin)
- Change the alias in
Menu item Btomenu-item-a. It should give a error message. - Change the alias in
Menu item Ftomenu-item-a. It should give a error message. - Now set the site to multilingual (Enable Language Filter System plugin)
- Change the alias in
Menu item Btomenu-item-a. It should give a error message. - Change the alias in
Menu item Ftomenu-item-e. It should save fine. - Change the alias in
Menu item Etomenu-item-a. It should give a error message. - Now create a menu item of System Type -> Menu alias and let it auto generate the alias. It should generate fine (without the date as alias).
- Play a little with the alias and check everything is fine (test with and without unicode in global config).
Finally do a code review of the changes.
Observations
The alias cannot be duplicated in the following cases:
Monolingual Site (language filter plugin disabled)
- The menu item is at root level and has the same alias of other menu item that is at root level of any other menu.
- The menu item is not at root level, but was the has the same alias of other menu item that has the same parent.
Multilingual Site (language filter plugin enabled)
- The menu item language is "All", is at root level and has the same alias of other menu item (any language) that is at root level of any other menu.
- The menu item language is NOT "All", is at root level and has the same alias of other menu item (same or "All" language) that is at root level of any other menu.
- The menu item language is "All", is not at root level, but was the has the same alias of other menu item (any language) that has the same parent.
- The menu item language is "All", is not at root level, but was the has the same alias of other menu item (same or "All" language) that has the same parent.
@infograf768 please check if all is working fine.
| Status | New | ⇒ | Pending |
| Labels |
Added:
?
|
||
| Category | ⇒ | Components |
@infograf768 maybe adding a date to a menu item alias of the external link to "save alias" is not the correct behaviour.
Imagine the following scenario.
i have a parent menu item with a external link menu item type. And a child with a article menu item type.
In the before this patch scenario we get a SEF URL something like
/2016-06-08-11-40-42/test-menu-item-article
After this patch we get a /test-menu-item-external-link/test-menu-item-article
I have tested this item
This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10756.
Forget the unsuccesful test
I have tested this item
This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10756.
Note: I understand you have prepared the PR to use different strings in the future, but is it really necessary at this point? For the moment, we just want to correct the bug (with ALL).
You have already tested this. The only difference is that this PR does not change the error message as there was a debate on the link with target blank.
Can you please confirm thsi is working as it solves a long standing possible bug letting save a menu item alias with same alias as another with same parent when language ALL is concerned?
I have tested this item
This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10756.
| Status | Pending | ⇒ | Ready to Commit |
| Labels |
Added:
?
|
||
This PR has received new commits.
CC: @brianteeman, @infograf768
This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10756.
Why we are having checks inside store method instead of check itself? Isn't it the whole purpose of check method to be!
Was there any specific reason?
Just removed the dot from the message (shouldn't be a concatenation).
No issues, can still be RTC.
Why we are having checks inside store method instead of check itself? Isn't it the whole purpose of check method to be!
Was there any specific reason?
I had that doubt also, didn't know why was like that, so reused what existed.
See https://github.com/joomla/joomla-cms/blob/staging/libraries/legacy/table/menu.php#L187
IMO, the data should be not strongly coupled with configuration as much as possible. If we can allow same alias for different language selection, that should be true even when multilingual is off (of course by still checking the language attribute). Because we won't be able to revalidate if the settings change.
And I don't think this matter is so straight forward. There must be some association check involved so as to allow alias sharing only for those items which just differ in language and not the content.
E.g. - Following can be a justifiable set to share same alias:
en = How to install Joomla?
es = Cómo instalar Joomla?
But not for the following set:
en = How to change user permissions?
es = Cómo instalar Joomla?
I don't know what we should do about this, so thought to share here.
PS: es isn't my native language
IMO, the data should be not strongly coupled with configuration as much as possible.
I totally agree, but IMHO this is a very special case, is not everyday we change a site from multi-lingual to mono-lingual and vice-versa.
If we can allow same alias for different language selection, that should be true even when multilingual is off (of course by still checking the language attribute).
If we allow the same alias for two menu items in the root level in a mono-lingual site (not matter the language) we'll get the same url in the frontend for two menu items which cannot happen.
@infograf768 please check this.
... we'll get the same url in the frontend for two menu items ...
I undestand. Thats why I said
... I don't think this matter is so straight forward. There must be some association check involved ...
Edit:
But in that case we can choose to display the menu item which either matches '*' language or the site default language, ignoring other specific languages if monolingual is active and multiple items match for the queried alias. Yeah, too much complicated!
If we allow the same alias for two menu items in the root level in a mono-lingual site (not matter the language) we'll get the same url in the frontend for two menu items which cannot happen.
@infograf768 please check this.
indeed. pr is fine by solving this possible issue and i can't believe nobody saw that issue before...
wilsonge
- | Status | Ready to Commit | ⇒ | Fixed in Code Base |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2016-06-15 23:22:29 |
| Closed_By | ⇒ | wilsonge |
| Labels |
Removed:
?
|
||
Pleas set the $this->type= 'url' to empty to get a date as the alias is no use for an url and it will avoid an error if the alias is used for another menu item with same parent. THis was the reason it was implemented such originally.