BUG: Sample Data blog various issues #17930

infograf768 · 2017-09-11T07:52:04Z

Pull Request for Issue #17917

Thanks @imanickam for your findings

Issues

The sample data blog can't work with languages using non latin glyphs because aliases are set to the same time stamp for all aliases and menutype is empty in that case.
This is due to the use of JApplicationHelper::stringURLSafe()

The menutype can use non latin glyphs but can't work for some languages because it should be limited to 24 characters in the db and it is defined in the existing code using the title which may be longer in the ini file.

The catid is hard coded as ['9'] although it should use the correct catid depending on the sample data blog installed.
This is specially true when installing multiple sample data blogs in a multingual site (after switching admin language).

Changes in this PR

Truncating menutypes and not using JApplicationHelper::stringURLSafe() as it is not anyway necessary there. Adding numbering.

Hardcoding categories aliases to cope with the time stamp.

Hardcoding aliases for menu items to cope with the time stamp.

Adding numbering and default sample to articles aliases to make sure each alias is different including with non latin glyphs titles.

Testing instructions.

This should be tested on a monolanguage site as well as on a multilingual site.

Monolanguage site:
Install the Tamil language pack on a clean installation of staging with no sample data.
ta-IN_joomla_lang_full_3.8.0v1.zip
Switch to Tamil and in the Control panel, install the sample data blog

Reinstall a clean site in the same conditions and check all is OK too with this French pack
fr-FR_joomla_lang_full_3.8.0v1.zip

Multilingual site:
Install a clean staging as multilingual. At time of installation, install both 3.7.5 Tamil and French language packs. Set Multilang to yes with content.

In admin, install the 2 packs above to update Tamil and French to 3.8.0.1
Switch languages in back-end and each time, install sample data for these languages (English too if desired).

Verify that both Oldest Posts modules and Mostly Read Posts are correctly set to use the Blog category in their languages.

Load frontend and switch languages. Check that all is fine.
We will now have a correctly functionning sample data blog.

IMHO this should go into 3.8.0

@imanickam
@Bakual
@mbabker

mbabker · 2017-09-11T11:56:31Z

plugins/sampledata/blog/blog.php

-			// Calculate menutype.
-			$menu['menutype'] = JApplicationHelper::stringURLSafe($menu['title']);
+			// Calculate menutype. The number of characters allowed is 24.
+			$type = JHtmlString::truncate($menu['title'], 23, true, false);


Please do not direct call a JHtml method. JHtml::_('string.truncate', $menu['title'], 23, true, false);

mbabker · 2017-09-11T11:57:36Z

The hardcoding of aliases really seems more like a hack than a practical fix...

Bakual · 2017-09-11T12:47:09Z

The catid fix (obviously) is fine.
The menutype fix looks fine as well. Sanitising is done by the table as well and looks more strict than stringUrlSafe.
The alias fix for non-latin however needs something else. As Michael already wrote it is a hack. Imho it also counteracts the separation of text and code. Eg when you want to adjust a title, you would have to adjust the alias in code as well.
What do you think when we just add the counter to the alias generation? Eg $article['alias'] = JApplicationHelper::stringURLSafe($article['title'] . '-' . $i);. This way in worst case you get the counter as alias but in the other cases you get a nice translated alias, just with a number following.

infograf768 · 2017-09-11T14:12:33Z

@mbabker @Bakual
concerning the aliases.
We can indeed do that for articles as we have alraedy a counter $i. After all I just placed it first and added sample for now.
Not a big difference with
$article['alias'] = $i . 'sample' . JApplicationHelper::stringURLSafe($article['title']);
I will do the change if it blocks the merge.

For categories, we have no counter and I do not know how to add one. We have only 2 categories.
What do you think?

mbabker · 2017-09-11T14:15:05Z

My concern isn't so much the fact we need changes (nobody's perfect), rather at a glance it seems like the way some things are handled seems more like a low level hack to work around an issue than actually fixing an issue (and we have a bad history of taking the low level hacks and not coming back to fix the deeper issue).

infograf768 · 2017-09-11T14:19:36Z

The problem is that we have many languages using non-latin glyphs.
Therefore we have to find a solution. Without using the global transliterator (we already discussed about that and found out it is not available for all versions of PHP), there is no other way in this feature to get acceptable aliases.

Will change the alias for articles to @Bakual proposal. Not a big deal.
For categories, I have no way and it is not imho as hacky as it seems.

Bakual · 2017-09-11T16:25:56Z

For categories, I have no way and it is not imho as hacky as it seems.

For categories and menuitems we could hardcode a number instead of the counter. Eg take the number that is already in the title language constant.
Thinking about it, we could also move the number suffix outside the stringUrlSafe method (stringURLSafe($article['title']) . '-' . $i). This way worst case would become 2017-09-11-16-25-32-1 instead of just 1.

Bakual · 2017-09-11T16:28:12Z

seems more like a low level hack to work around an issue than actually fixing an issue

If we wanted to fix this in the stringUrlSafe method, we could add miliseconds to its fallback string generation. That could work as I doubt we add multiple articles in the same milisecond 😄

Bakual · 2017-09-11T17:35:19Z

Forget those comments about the datetime stuff. I somehow wrongly assumed that is generated by the stringUrlSafe method. But it's done by the table. We could of course also add something like that.
(https://github.com/joomla/joomla-cms/blob/staging/libraries/src/Table/Content.php#L199-L204)

infograf768 · 2017-09-11T18:28:32Z

would not hardcoding a number be similar to hardcoding the more informative text i used? i mean as 'hacky' ?

AlexRed · 2017-09-11T18:31:08Z

I have tested this item ✅ successfully on 88daa64

Patch ok for me

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

Bakual · 2017-09-11T19:41:53Z

would not hardcoding a number be similar to hardcoding the more informative text i used? i mean as 'hacky' ?

It at least separates the text from code and still allows for translated alias. But yeah, it still is a bit of a hack. If we wanted to do a real fix, we would have to change the lines in the tables and return miliseconds instead of just seconds there. That would work. Not sure if it's worth it.

infograf768 · 2017-09-12T10:51:46Z

In this last commit, I took off ALL hardcoded stuff and forced using unicodeslugs aliases temporarily when the language is not using latin glyphs (idea from @imanickam ).
This works in multilang as well as monolang with same testing instructions.

below examples in multilang

Results for menu items

Result for categories

Result for articles

Example of url obtained when clicking on an article title link from the blog category
/index.php/ta/வலைப்பதிவு/18-தங்கள்-வலைப்பதிவிற்கு-வரவேற்கிறோம்-ta-in

@mbabker
@Bakual
@imanickam
@AlexRed

Please test again.

imanickam · 2017-09-12T15:53:31Z

I have tested this item ✅ successfully on cca4fef

I have tested this/these change/changes successfully in the following three scenarios:

(a) Install of Sample Data using the Sample Data Plugin in English (en-GB) - The back-end language was English - Mono lingual site
(b) Install of Sample Data using the Sample Data Plugin in Tamil (ta-IN) - The back-end language was Tamil - Mono lingual site
(c) Install of Sample Data using the Sample Data Plugin in Tamil (ta-IN) - The back-end language was Tamil - Multilingual site

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

infograf768 · 2017-09-14T08:10:20Z

@Bakual @AlexRed
We need a second tester

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

AlexRed · 2017-09-14T08:26:04Z

I have tested this item ✅ successfully on cca4fef

Patch ok for me

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

ghost · 2017-09-14T08:30:17Z

RTC after two successful tests.

mbabker · 2017-09-14T12:31:05Z

plugins/sampledata/blog/blog.php

+		// Set unicodeslugs if alias is empty
+		if (trim(str_replace('-', '', $alias) == ''))
+		{
+			JFactory::getConfig()->set('unicodeslugs', 1);


We should probably reset this to whatever value it was beforehand. Right now this causes a side effect of changing the setting for everything that happens after this call.

Would it be limited to the method concerned?

I mean limited to the method

Adding
$unicode = JFactory::getConfig()->get('unicodeslugs', 1);

And at the end of the method
JFactory::getConfig()->set('unicodeslugs', $unicode);

And this for each method?

There shouldn't be anything happening after that code which could produce any side effects. Only the AJAX response is left to be done by com_ajax.
But yeah, better safe than sorry 😄

Anytime you're making a configuration change if it's supposed to be temporary to work around an issue it should be immediately reverted to the state it was before just to make sure nothing unexpected happens. As this one specifically is the first conditional hit in this step, if the configuration is changed then as is everything calling stringURLSafe afterwards (or anything relying on the unicodeslugs param) is arbitrarily left with unicodeslugs = 1. Granted, the chances of this particular thing causing side effects is low, it is best practice to always reset a temporary change.

Will modify asap. I had tested though that after this temporary change, the real value is not changed in configuration.php when the script has finished running.

infograf768 · 2017-09-14T16:04:46Z

OK now?

BUG: Sample Data blog various issues

9c85323

joomla-cms-bot added the PR-staging label Sep 11, 2017

infograf768 mentioned this pull request Sep 11, 2017

Non-Latin Alphabet Language's Sample Data Installation using mod_sampledata Fails #17917

Closed

mbabker reviewed Sep 11, 2017

View reviewed changes

Correcting truncate code

609f4c2

Simplifying articles aliases creation

88daa64

Using unicode aliases when language is not using latin glyphs

ba47e90

correct truncate (again)

cca4fef

joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Sep 14, 2017

infograf768 added this to the Joomla 3.8.0 milestone Sep 14, 2017

mbabker reviewed Sep 14, 2017

View reviewed changes

Makung sure unicodeslugs is reverted to the configuration settings

b26b55b

Tweak config reset

be9978d

joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Sep 16, 2017

infograf768 deleted the sampledata_blog branch June 11, 2018 05:30

infograf768 mentioned this pull request Sep 4, 2019

[4.0] Multilingual sample data: Changing aliases to unicode for articles and categories for non latin languages #20759

Merged

Uh oh!

BUG: Sample Data blog various issues #17930

BUG: Sample Data blog various issues #17930

Uh oh!

Conversation

infograf768 commented Sep 11, 2017 • edited by joomla-cms-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issues

Changes in this PR

Testing instructions.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mbabker commented Sep 11, 2017

Uh oh!

Bakual commented Sep 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

infograf768 commented Sep 11, 2017

Uh oh!

mbabker commented Sep 11, 2017

Uh oh!

infograf768 commented Sep 11, 2017

Uh oh!

Bakual commented Sep 11, 2017

Uh oh!

Bakual commented Sep 11, 2017

Uh oh!

Bakual commented Sep 11, 2017

Uh oh!

infograf768 commented Sep 11, 2017

Uh oh!

AlexRed commented Sep 11, 2017

Uh oh!

Bakual commented Sep 11, 2017

Uh oh!

infograf768 commented Sep 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

imanickam commented Sep 12, 2017

Uh oh!

infograf768 commented Sep 14, 2017

Uh oh!

AlexRed commented Sep 14, 2017

Uh oh!

ghost commented Sep 14, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

infograf768 commented Sep 14, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

infograf768 commented Sep 11, 2017 •

edited by joomla-cms-bot

Loading

Bakual commented Sep 11, 2017 •

edited

Loading

infograf768 commented Sep 12, 2017 •

edited

Loading