? ? Pending

User tests: Successful: Unsuccessful:

avatar Bakual
Bakual
11 Jul 2018

Pull Request for Issue #21073 .

Summary of Changes

Adds a table check prior to storing.

Testing Instructions

Create the blog sampledata and check the menutype of the created menus.

Expected result

They should contain no spaces and be lowercased.

Actual result

Contain spaces and uppercase characters

Documentation Changes Required

None

avatar Bakual Bakual - open - 11 Jul 2018
avatar Bakual Bakual - change - 11 Jul 2018
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 11 Jul 2018
Category Unit Tests Repository Administration com_admin SQL Postgresql MS SQL com_associations com_banners
avatar brianteeman
brianteeman - comment - 11 Jul 2018

Oops

avatar Bakual
Bakual - comment - 11 Jul 2018

Better now :)

avatar brianteeman brianteeman - test_item - 11 Jul 2018 - Tested unsuccessfully
avatar brianteeman
brianteeman - comment - 11 Jul 2018

I have tested this item ? unsuccessfully on 3e436e0

There is an error in the sample data plugins. Response is invalid.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/21080.
avatar brianteeman
brianteeman - comment - 11 Jul 2018

Note that the multilingual sample plugin does not have this problem. menus are created correctly there


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

avatar Bakual Bakual - change - 12 Jul 2018
Labels Added: ?
avatar joomla-cms-bot joomla-cms-bot - change - 12 Jul 2018
Category Unit Tests Repository Administration com_admin SQL Postgresql MS SQL com_associations com_banners Front End Plugins
avatar Bakual
Bakual - comment - 12 Jul 2018

I don't know why you got an invalid response. It works for me fine.
One reason may be that the check did return false, and since my code didn't test it may have triggered some unhandled error somewhere. I've changed now the code so it checks for exceptions and return values. Which was also the main difference to the multilingual code.

avatar brianteeman
brianteeman - comment - 12 Jul 2018

the invalid response was me checking it in j4 not staging

avatar brianteeman
brianteeman - comment - 12 Jul 2018

the pr works correctly in j3

avatar brianteeman brianteeman - test_item - 12 Jul 2018 - Tested successfully
avatar brianteeman
brianteeman - comment - 12 Jul 2018

I have tested this item successfully on 2648ed0


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

avatar Bakual
Bakual - comment - 12 Jul 2018

Ah, that explains it :)

avatar infograf768
infograf768 - comment - 12 Jul 2018

While testing, I found another possible issue:
we have
'title' => JText::_('PLG_SAMPLEDATA_BLOG_SAMPLEDATA_MENUS_MENU_' . $i . '_TITLE') . $langSuffix,
and
$type = JHtml::_('string.truncate', $menu['title'], 23, true, false);

If the title is the same in 2 or more languages and truncate takes off the lang code , the menu type maybe exactly the same.

Afaik, it is forbidden to create multiple menus with the same Menu Type value.

EDIT: also, a Menu can have the same title but not the same menu type, which is bizarre as it is the title which is displayed in the Select Menu dropdown field.

What do you think?

avatar infograf768
infograf768 - comment - 12 Jul 2018

a Menu can have the same title but not the same menu type,

This is a general behavior, unrelated to this PR.

avatar infograf768
infograf768 - comment - 12 Jul 2018

Will propose a PR for the menu Title general issue.

See #21085

avatar Bakual
Bakual - comment - 12 Jul 2018

If the title is the same in 2 or more languages and truncate takes off the lang code , the menu type maybe exactly the same.

I doubt the changes for that happening is great, but yes it may happen if the languages are close enough or didn't translate the english text.
But we're talking about a blog sampledata here. I don't think the case were you install the same sampledata for multiple close or non-translated language packs is often. If you install it for german, french and italian and all of those packs have the sampledata translated, then it will work fine ?

avatar infograf768
infograf768 - comment - 12 Jul 2018

Hmm. I have not tested in German, but I should with de-DE, de-AT, etc. to see what happens

avatar brianteeman
brianteeman - comment - 12 Jul 2018

you can resolve this in the plugin by truncating the title first and then adding the language code

avatar Bakual
Bakual - comment - 12 Jul 2018

you can resolve this in the plugin by truncating the title first and then adding the language code

Would be an option, yes. However I really doubt we run into that issue anytime. Keep in mind that

  • a menu title has to be longer than 23 characters
  • a second language must have a menu title with the exact same first 23 characters.

For the blog sample data, I don't even see a title that may get longer than 23 characters even in german ?.
But yeah, if someone wants to have a take at it, go ahead.

avatar infograf768
infograf768 - comment - 13 Jul 2018

I have tested this item successfully on 2648ed0


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

avatar infograf768 infograf768 - test_item - 13 Jul 2018 - Tested successfully
avatar infograf768 infograf768 - change - 13 Jul 2018
Status Pending Ready to Commit
avatar infograf768
infograf768 - comment - 13 Jul 2018

RTC

Similar patch for 4.0 ?


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

avatar Bakual
Bakual - comment - 13 Jul 2018

Similar patch for 4.0 ?

George will merge staging up to 4.0. If something is needed afterwards, we will do that.

avatar infograf768 infograf768 - close - 13 Jul 2018
avatar infograf768 infograf768 - merge - 13 Jul 2018
avatar infograf768 infograf768 - change - 13 Jul 2018
Status Ready to Commit Fixed in Code Base
Closed_Date 0000-00-00 00:00:00 2018-07-13 07:58:06
Closed_By infograf768
Labels Added: ? ?
Removed: ?
avatar infograf768
infograf768 - comment - 13 Jul 2018

Thanks.

Add a Comment

Login with GitHub to post a comment