[com_menus] improve alias check and generation by andrepereiradasilva · Pull Request #10756 · joomla/joomla-cms

andrepereiradasilva · 2016-06-08T10:22:50Z

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 B to menu-item-a. It should give a error message.
Change the alias in Menu item F to menu-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 B to menu-item-a. It should give a error message.
Change the alias in Menu item F to menu-item-e. It should save fine.
Change the alias in Menu item E to menu-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.

infograf768 · 2016-06-08T11:22:15Z

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.

andrepereiradasilva · 2016-06-08T11:54:29Z

@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

infograf768 · 2016-06-08T15:50:50Z

I have tested this item 🔴 unsuccessfully on d884169

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10756.}

infograf768 · 2016-06-08T15:52:42Z

Forget the unsuccesful test

infograf768 · 2016-06-08T15:55:20Z

I have tested this item ✅ successfully on d884169

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10756.}

infograf768 · 2016-06-08T16:01:31Z

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).

infograf768 · 2016-06-09T05:22:36Z

@brianteeman

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?

brianteeman · 2016-06-10T07:36:12Z

I have tested this item ✅ successfully on d884169

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10756.}

brianteeman · 2016-06-10T07:36:56Z

RTC

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10756.}

izharaazmi · 2016-06-14T10:36:35Z

libraries/legacy/table/menu.php

+			// The alias already exists. Send an error message.
+			if ($errorType)
+			{
+				$message .= JText::_('JLIB_DATABASE_ERROR_MENU_UNIQUE_ALIAS' . ($this->menutype != $table->menutype ? '_ROOT' : ''));


Concatenation with undefined variable?

undefined variable?

?

$message was never defined before this line and this line uses .= operator.

you're right! thanks will correct.

joomla-cms-bot · 2016-06-14T11:02:55Z

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.}

izharaazmi · 2016-06-14T11:02:55Z

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?

andrepereiradasilva · 2016-06-14T11:04:36Z

Just removed the dot from the message (shouldn't be a concatenation).
No issues, can still be RTC.

andrepereiradasilva · 2016-06-14T11:08:23Z

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

izharaazmi · 2016-06-14T11:22:16Z

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 😄

andrepereiradasilva · 2016-06-14T11:34:41Z

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.

izharaazmi · 2016-06-14T11:37:05Z

... 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!

infograf768 · 2016-06-14T18:07:23Z

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...

better alias check

e133ad0

joomla-cms-bot added the PR-staging label Jun 8, 2016

andrepereiradasilva added 3 commits June 8, 2016 11:23

improve comment

be78690

cs

7c265f1

cs

d884169

joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Jun 10, 2016

izharaazmi reviewed Jun 14, 2016
View reviewed changes

not definied before so don't use dot for concatenation!

7f32cca

wilsonge merged commit de1803a into joomla:staging Jun 15, 2016

wilsonge added this to the Joomla 3.6.0 milestone Jun 15, 2016

joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Jun 15, 2016

andrepereiradasilva deleted the menu-alias branch June 15, 2016 23:23

brianteeman mentioned this pull request Jul 29, 2016

Bug in the creation of the menu item: Alias menu item #11348

Closed

brianteeman mentioned this pull request Jul 31, 2016

Menu Item Alias doesn't put date in alias field Joomla 3.6 #11372

Closed

infograf768 mentioned this pull request Dec 5, 2016

Improve menu associations #13086

Merged

brianteeman mentioned this pull request Dec 5, 2017

Using the current date as alias in system menu items if alias is empty. #18985

Closed

Uh oh!

Conversation

andrepereiradasilva commented Jun 8, 2016 • edited by brianteeman Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of Changes

Testing Instructions

Observations

Monolingual Site (language filter plugin disabled)

Multilingual Site (language filter plugin enabled)

Uh oh!

infograf768 commented Jun 8, 2016

Uh oh!

andrepereiradasilva commented Jun 8, 2016

Uh oh!

infograf768 commented Jun 8, 2016

Uh oh!

infograf768 commented Jun 8, 2016

Uh oh!

infograf768 commented Jun 8, 2016

Uh oh!

infograf768 commented Jun 8, 2016

Uh oh!

infograf768 commented Jun 9, 2016

Uh oh!

brianteeman commented Jun 10, 2016

Uh oh!

brianteeman commented Jun 10, 2016

Uh oh!

izharaazmi Jun 14, 2016

Choose a reason for hiding this comment

Uh oh!

andrepereiradasilva Jun 14, 2016

Choose a reason for hiding this comment

Uh oh!

izharaazmi Jun 14, 2016

Choose a reason for hiding this comment

Uh oh!

andrepereiradasilva Jun 14, 2016

Choose a reason for hiding this comment

Uh oh!

andrepereiradasilva Jun 14, 2016

Choose a reason for hiding this comment

Uh oh!

joomla-cms-bot commented Jun 14, 2016

Uh oh!

izharaazmi commented Jun 14, 2016

Uh oh!

andrepereiradasilva commented Jun 14, 2016

Uh oh!

andrepereiradasilva commented Jun 14, 2016

Uh oh!

izharaazmi commented Jun 14, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

andrepereiradasilva commented Jun 14, 2016

Uh oh!

izharaazmi commented Jun 14, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

infograf768 commented Jun 14, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

andrepereiradasilva commented Jun 8, 2016 •

edited by brianteeman

Loading

izharaazmi commented Jun 14, 2016 •

edited

Loading

izharaazmi commented Jun 14, 2016 •

edited

Loading