Update installation/model/languages.php

[richard67]

Replaced hard-coded use of extension id 600 by a query for the en-Gb language pack and added quoteName wherever it was missing in the (optional) additional languages installation step of the installation process (installation/model/languages.php).

This is the last place where the hard-coded extension id 600 is used. With this PR we get rid of this.

The change for this is similar as done for the com_installer with PR #5327 .

Testing:

There should be no change, all should work as before ;-)

Details:

Perform a new installation of a Joomla! with the changed file from here in the installation/model folder.

A the (normally final) step after the installation has finished, do NOT press the button to delete the installation folder.

Instead of this, choose the optional step for installing additional languages (button on the right hand side) and go through the language installation, i.e. install some additional language and adjust some of the languages-related setting in the following step.

Then at the end, delete the installation folder on the last page and login to the admin interface.

Verify that additional languages have been installed and changed settings have been applied.

If all this works as it does without this PR, then all is fine.

[richard67]

Updated description, typo correction

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

[waader]

@test works

Remark: I describe the following because it seems that you didn´t expierence this behaviour.

What I did:

• install current staging with and without your patch on mysql and postgresql
• during installation installed 3 additional languages
• activate the "multilingual feature" with "install localised content" and "enable language code plugin" set to yes

The result is the same regardless if the patch is applied or not. But I see some odd behaviour. After the first step - the installation of the 3 different languages - the screen looks like this (mysql and postgresql):

Please notice the "undefined".

After the next step - installation of the multilingual feature - the screen looks like this (mysql and postgresql):

After that I logged in and checked what has been installed and finally inspected the "multilingual status" - at the bottom left. In mysql everything is ok, in postgresql I got this error:

0 Database query failed (error # %s): %s SQL=SELECT u.name, count(cd.language) as counted, MAX(cd.language='') as all_languages FROM w1g9z_users AS u LEFT JOIN w1g9z_contact_details AS cd ON cd.user_id=u.id LEFT JOIN w1g9z_languages as l on cd.language=l.lang_code WHERE EXISTS (SELECT * from w1g9z_content as c where c.created_by=u.id) AND (l.published=1 or cd.language='') AND cd.published=1 GROUP BY u.id HAVING (counted !=4 OR all_languages=1) AND (counted !=1 OR all_languages=0)

This is something that has worked before in the making of J34.

[richard67]

@waader This seems to be 2 other issues:

1. It sounds as if there was something with the multilingual status module or something used by this, so not be a part of the installation.
2. The title "undefined" should be warnings - seems some messages are enqueued without type. This belongs to the installation.

So we could say this PR here is tested with success because no change to before, i.e. no new error.

But we need 2 new PRs for the 2 problems (which might also be caused by the same thins - some data maybe not inserted with the SQL for postgress).

I have no idea where the problem comes from, otherwise I would make the PR immediately.

I hope some other, more experienced contributors than me can have a look.

[richard67]

@test I just submitted test result for @waader using the issue tracker, just in case he accessed via github so he could not click this. So it is not me testing myself ;-)

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

[waader]

@richard67 There was some sort of overlapping. I marked this also as successful.

Thanks for your comments. I will look into it next week.

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

[richard67]

@waader With these messages of unknown type issued by the installer I could maybe have a look, too.

[waader]

That would be great!

[infograf768]

I confirm the issue with "undefined" already before this patch.

[infograf768]

@nikosdion
Could this be a side effect to the changes we did in languages install?

[nikosdion]

@infograf768 Nope. You are confusing two different things because we both call them "installation" (yeah, I know, confuses the crap out of me too). What we changed is the installation of extensions through the Joomla! Extensions Manager. The problem you have is in the Joomla! web installer application (/installation).

I haven't looked at the code but I am pretty sure that the /installation application enqueues messages without a message type. This is something that needs to be fixed in the /installation application on a separate PR.

Likewise, I see two more bugs being reported here:

1. The PostgreSQL driver still doesn't report the error message returned by the database server (this is broken for over two years and I had to fork the driver for use in Akeeba Backup over a year ago)
2. There's a SQL issue with the multilingual installation. Looking at the SQL query I see that database/table/column names are not escaped and there seem to be proprietary MySQL statements in there which won't run on a different db server. Fixing this requires to get the exact error message from the db server which means that the other bug above has to be fixed first.

Basically, there are lots of bugs but none seems to be related to anything we modified in the extensions installer / updater.

[infograf768]

I had tried to add the message type in 2 places (notice):

if (!$lids)
        {
            // No languages have been selected
            $app->enqueueMessage(JText::_('INSTL_LANGUAGES_NO_LANGUAGE_SELECTED'), 'notice');
        }
        else
        {
            // Install selected languages
            $model->install($lids);
            $app->enqueueMessage(JText::_('INSTL_LANGUAGES_MORE_LANGUAGES'), 'notice');
        }

Without any success

[infograf768]

In fact no need to go as far as installing languages. Not filling the name field of the database will trigger a message with "undefined" as title

[richard67]

@infograf768 Shouldn't we make a new issue for the "undefined" problem? This one here was tested with success.

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

[richard67]

Hmm, I tested now with nightly build Joomla_3.4.0-dev-Development-Full_Package.zip from yesterday (Dec. 6th, 2014) and could not see these "Undefined" titles for the messages.

Maybe there is just an issue with these message titles not to be displayed on the installation screens?

@nikosdion Regarding usage of quoteNames I have corrected all "normal" SQL statements with this PR. But what I have not corrected are those "special" queries where data arrays are built and then bound with the bind function of JTable.
With these I am not sure (yet) how to use the quoteName function. And there are params set with values being json strings, so for this maybe some quioting has to be done, too, but I do not know (yet) how.
How shall we handle this? Shall I correct this PR here for adding quoteName to the "special" SQL statements?
Or can someone help me with these SQL statements in this one file handled by this PR, so we save time because I do not have to do research on API docs?

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

[nikosdion]

@richard67 Something to remember about JDatabaseDriver: $db->q() is an alias to $db->quote() and $db->qn is an alias to $db->quoteName. Please use the short form ($db->q() and $db->qn()) in the code you submit to the Joomla! core, for consistency reasons.

$db->q() is used to quote values (data) whereas $db->qn() is used to quote database, table and table field names. Please make sure you do NOT mix them up, as this will result in SQL errors which are notoriously difficult to debug. This also means that if you have ended up using quoteName (or its alias, qn) against JSON data you're most certainly doing something wrong.

Regarding the code generated by JTable, I agree that it's outside the scope of your PR.

[richard67]

@waader @infograf768 @nikosdion
Aaaaaaah ... I just see I made a mistake when using quoteName on the SQL join in function getAdminId!!!!
I included the table alias prefix into the name to be quoted, i.e. "quoteName('u.id')" instead of 'u.' . "quoteName('id')"!!!
Shame on me (mega blush)!
I correct this with a commit within the next hour, and then please re-test.


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

[richard67]

@nikosdion I know about the difference betwen quote and quoteNames, but I did not know about the short versions. Regarding use of long names I was inspired by @wilsonge 's PR #5329 . And i found the long names in code more often than the short ones, wherever I looked. So please confirm or not: Shall I change in this file all quote and quoteNames to q and qn?

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

[nikosdion]

The correct way would be $db->qn('u') . '.' . $db->qn('id')

Reasoning: the database notation u.id means "the field called id in the table known in this query as u". Both u and id are database entity names. u is the name of a table and id is the name of a filed. Therefore both must go through quoteName to have 100% portable SQL code.

Regarding long vs short names: the standard in Joomla! is to use the short names. They make the terse SQL builder code much more readable. Framework developers have gone as far as providing phpDoc hints for the aliases to let IDEs such as phpStorm, Netbeans and Eclipse provide autocompletion for the short names. While the code is technically valid no matter what you use, the short versions are easier to read. Especially when you construct rather big SQL statements, spanning more than four lines of PHP code and you're doing quoting right.

PS: For the purists among you, yes, in the example above you can have u unquoted because MySQL, PostgreSQL and SQL Server / SQL Azure will accept it. But what happens if two years down the line we add support for some other database engine which mandates all entity names to be quoted? That's why I get a bit anal retentive on properly quoting everything.

[richard67]

@nikosdion I know what the "u.id" means ;-) I am expert in Oracle SQL, I only have a bit problems with how joins are specified in other SQL languages. But the other basics I know about. I will change the file and update this PR here for using "paranoid quoting" and having corrected my mistake with the table alias prefix ('u.'). For both I will update this one SQL statement (the join) in function getAdminId.
The other thing, those queries where the bind function of JTable is used, for these I assume that the JTable functions use the unquoted data and names and will do quotung themselves, or is this qrong?

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

[wilsonge]

What Nic said. Personally I use the long names for quoteName because that's what we use in the entire of the CMS. And it's just a consistency thing. But he's right about how to set things out.

The other thing is that doing $db->from($db->quoteName('#__users', 'u')) will give you FROM "jos_users" AS "u" without needing to hardcode the AS :)

[richard67]

@wilsonge does this also work with others than "from", e.g. "select($db->qn('id', 'i'))" instead of "$db->qn('u') . '.' . $db->qn('id')"?

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

[wilsonge]

Yes it does :)

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

[richard67]

@wilsonge So the following old statement

    // Select the required fields from the updates table.
    $query
        ->clear()
        ->select('u.id')
        ->from('#__users as u')
        ->join('LEFT', '#__user_usergroup_map AS map ON map.user_id = u.id')
        ->join('LEFT', '#__usergroups AS g ON map.group_id = g.id')
        ->where('g.title = ' . $db->q('Super Users'));

would then be

    // Select the required fields from the updates table.
    $query
        ->clear()
        ->select($db->qn('id', 'u'))
        ->from($db->qn('#__users', 'u'))
        ->join('LEFT', $db->qn('#__user_usergroup_map') . ' AS ' . $db->qn('map') . ' ON ' . $db->qn('user_id', 'map') . ' = ' . $db->qn('id', 'u'))
        ->join('LEFT', $db->qn('#__usergroups') . ' AS ' . $db->qn('g') . ' ON ' . $db->qn('group_id', 'map') . ' = ' . $db->qn('id', 'g'))
        ->where($db->qn('title', 'g') . ' = ' . $db->q('Super Users'));

???

Or could the join be beautified further?

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

[richard67]

I've just corrected one error with SQL names quoting introduced with 1st step of this PR in function getLanguageList, and I have reverted the SQL statement with the join getAdminId which I wanted to discuss above with @wilsonge (he has not answered yet) back to its original state. Now the localized test data (3 articles and 3 menus) seems to be installed correctly.
@waader @Infograf can you re-test this PR here?
@wilsonge The names quoiting of the SQL with the join (see above) did not work, so I reverted it to the original statement.

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

[richard67]

Hmm, I tried to add again names quoting for this last SQL statement with the join, but now line length is too long for Travis ... and when I break the lines, Travis will complain about opening and closed brackets for the join() function not being in the same line ... I will in some hour see what I can do.

[richard67]

@wilsonge Yes, have found out myself. It needs one final commit, then all SQL syntax will be fine and Travis will be happy. I just wait Travis finishing this one and then correct my multi-line function call's code style.

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

[richard67]

Now all is ready for re-test.

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

[waader]

@richard67 et all Thanks for investigating this further. I took a copy of the current staging branch and applied your patch. Leaving the "undefined" problem aside, installation went fine with mysql.

When using postgresql I get another result though (please have a look at the screenshot):

Now not even the selected additional languages got installed.

[richard67]

@waader Does this also happen without my patch?

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

[waader]

@richard67 As I read your question I knew what I had forgotten: I didn´t set the appropriate directory rights, hence the problem. So there is another small unrelated problem here: JLIB_INSTALLER_ABORT_NOINSTALLPATH is not very descriptive.

Regarding the postgresql sql error: as Nikosdion explained, there is a problem in the statement itself.

@retest works!

[richard67]

Could you open separate, new issues in the issues tracker for the "Undefined" title and the postgresql issue? I think we should keep issues separate, and I will not have much time within the next days do follow it myself.

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

[nikosdion]

Regarding the JLIB_INSTALLER_ABORT_NOINSTALLPATH untranslated error, I guess this is because the en-GB.lib_joomla.ini file is not loaded. In fact, I see that it's only loaded in the InstallationModelDatabase model, lines 103:111. I would move that code to InstallationApplicationWeb, dispatch method, right before line 181.

[waader]

I will open other tickets but first I have to do some investigations eg regarding the error reporting of the database drivers that are not reported back to the UI. I suspect that there is a problem with the data structure of the error message of the database driver. Probably a multi-dimensional array with a lot of status codes that have to be transformed appropriately.

[richard67]

Ok, thanks a lot up to now.

[infograf768]

Concerning JLIB_INSTALLER_ABORT_NOINSTALLPATH
In fact, we do have some JLIB_ strings in the installation xx-XX.ini for cases like this.
I would hate though asking all TTs to update their installation lang just for that unique constant...

@nikosdion
The loading of the language in InstallationModelDatabase :

// Load the selected language
        if (JLanguage::exists($currentLang, JPATH_ADMINISTRATOR))
        {
            $lang->load('joomla', JPATH_ADMINISTRATOR, $currentLang, true);
        }
        // Pre-load en-GB in case the chosen language files do not exist.
        else
        {
            $lang->load('joomla', JPATH_ADMINISTRATOR, 'en-GB', true);
        }

is loading the back-end version of a possibly installed language (and en-GB if not exist).
In fact, if I understand well, it would most of the time work only for other languages than en-GB when using a custom distro which already includes the same back-end language as the installation lang used.
Let's say the error happens for only one specific language among others a user tries to install when installing Joomla (and I doubt this would ever happen), it would be better to load specifically the lib_joomla.ini from front-end to take into account lang packs with a reduced admin part.
Practically though, I doubt the case would ever happen.

[nikosdion]

@infograf768 Here's the thing. The installer is using the Joomla! library classes to do stuff such as creating and modifying databases and files, installing extensions etc. The Joomla! library classes use the library translation strings.

Duplicating those strings in the installation translation file is extremely bad for two very good reasons:

• You are duplicating the translation effort
• For every subminor release of Joomla! someone has to go through all code paths and figure out which language strings may be used. This is the kind of work that takes experienced developers several person-months.

We don't have the luxury of time or people to spare on this. It makes much more sense to include the Joomla! library translation files in the installation folder and have the installation application load them. Otherwise it is absolutely certain that you will miss a language string or fifty and the potential Joomla! users who see them will, understandably, think that Joomla! is broken.

The other thing you can do is load the en-GB translation of the library translation files before loading the installer translation files in the user's language. This way the string you've missed will appear in English instead of Gibberish. Between seeing LIB_WHATEVER_ERR_FILE and "The file cannot be copied" I'll take the latter, even if the optimal string in my own language (Greek) would be "Το αρχείο δεν μπόρεσε να αντιγραφεί". That's an easy fix and people will report this bug as an "untranslated string" instead of "the Joomla! installer is broken; it spits out gibberish when I try to install your darn CMS" ;)

[infograf768]

You are duplicating the translation effort
For every subminor release of Joomla! someone has to go through all code paths and figure out which language strings may be used. This is the kind of work that takes experienced developers several person-months.

You are correct. We did this at a time I guess when we had no other way or nobody cared.
Loading the lib.joomla.ini systematically for the whole installation is the way to go.

[nikosdion]

Yep. It's up to you and the PLT to decide if it's going to be the translation file for the user's language (leads to a bigger installation ZIP file) or the en-GB file (same installation ZIP file size). Anyway, the arguments for one or the other solution are above my pay grade.

[infograf768]

We could use the code from database in InstallationApplicationWeb as you suggested. I would only load the lib_joomla.ini though.

[nikosdion]

That's easy and only takes a single line of code:
JFactory::getLanguage()->load('lib_joomla', JPATH_SITE, 'en-GB', true, true);

[phproberto]

For the undefined alert messages please check:

#5366

[richard67]

So this PR is ok now, and we handle the undefined alert messages with PR #5366, and the postgresql with another one to be created, or shall we handle the postgresql problem with this one here?

[waader]

@richard67 Lets handle that with another pr cause it´s unrelated.

[richard67]

Anyone else beside waader who likes to test this PR here?

[nikosdion]

@test +1 Works for me. We now have two successful tests.

[infograf768]

@test OK here too. Thanks. Merging.