? Success

User tests: Successful: Unsuccessful:

avatar richard67
richard67
5 Dec 2014

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.

avatar richard67 richard67 - open - 5 Dec 2014
avatar jissues-bot jissues-bot - change - 5 Dec 2014
Labels Added: ?
avatar richard67 richard67 - change - 5 Dec 2014
The description was changed
avatar richard67 richard67 - change - 5 Dec 2014
The description was changed
avatar brianteeman brianteeman - change - 6 Dec 2014
Category Installation
avatar brianteeman brianteeman - change - 6 Dec 2014
The description was changed
avatar richard67 richard67 - change - 6 Dec 2014
The description was changed
avatar richard67 richard67 - change - 6 Dec 2014
The description was changed
avatar richard67
richard67 - comment - 6 Dec 2014

Updated description, typo correction

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

avatar richard67 richard67 - change - 6 Dec 2014
The description was changed
avatar waader
waader - comment - 6 Dec 2014

@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):

j34_1

Please notice the "undefined".

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

j34_2

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.

avatar richard67
richard67 - comment - 6 Dec 2014

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

avatar richard67 richard67 - test_item - 6 Dec 2014 - Tested successfully
avatar richard67
richard67 - comment - 6 Dec 2014

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

avatar waader waader - test_item - 6 Dec 2014 - Tested successfully
avatar waader
waader - comment - 6 Dec 2014

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

avatar richard67
richard67 - comment - 6 Dec 2014

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

avatar waader
waader - comment - 6 Dec 2014

That would be great!

avatar infograf768
infograf768 - comment - 7 Dec 2014

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

avatar infograf768
infograf768 - comment - 7 Dec 2014

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

avatar nikosdion
nikosdion - comment - 7 Dec 2014

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

avatar infograf768
infograf768 - comment - 7 Dec 2014

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

avatar infograf768
infograf768 - comment - 7 Dec 2014

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

avatar richard67
richard67 - comment - 7 Dec 2014

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

avatar richard67
richard67 - comment - 7 Dec 2014

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.

avatar nikosdion
nikosdion - comment - 7 Dec 2014

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

avatar richard67
richard67 - comment - 7 Dec 2014

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

avatar richard67
richard67 - comment - 7 Dec 2014

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

avatar nikosdion
nikosdion - comment - 7 Dec 2014

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.

avatar richard67
richard67 - comment - 7 Dec 2014

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

avatar wilsonge
wilsonge - comment - 7 Dec 2014

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

avatar richard67
richard67 - comment - 7 Dec 2014

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

avatar wilsonge
wilsonge - comment - 7 Dec 2014

Yes it does :)

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

avatar richard67
richard67 - comment - 7 Dec 2014

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

avatar richard67
richard67 - comment - 7 Dec 2014

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.

avatar richard67
richard67 - comment - 7 Dec 2014

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.

avatar wilsonge
wilsonge - comment - 7 Dec 2014

Ahh the select thing is wrong the

$db->select($db->qn('id', 'u'))

will give you SELECT "id" AS "u" which is why it doesn't work (but why I said it would work). The "u"."id" you need to do as Nic said

avatar richard67
richard67 - comment - 7 Dec 2014

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

avatar richard67 richard67 - change - 7 Dec 2014
The description was changed
avatar richard67
richard67 - comment - 7 Dec 2014

Now all is ready for re-test.

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

avatar waader
waader - comment - 8 Dec 2014

@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):
j34_r1

Now not even the selected additional languages got installed.

avatar richard67
richard67 - comment - 8 Dec 2014

@waader Does this also happen without my patch?

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

avatar waader
waader - comment - 8 Dec 2014

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

avatar richard67
richard67 - comment - 8 Dec 2014

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.

avatar nikosdion
nikosdion - comment - 8 Dec 2014

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.

avatar waader
waader - comment - 8 Dec 2014

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.

avatar richard67
richard67 - comment - 8 Dec 2014

Ok, thanks a lot up to now.

avatar infograf768
infograf768 - comment - 9 Dec 2014

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.

avatar nikosdion
nikosdion - comment - 9 Dec 2014

@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" ;)

avatar infograf768
infograf768 - comment - 9 Dec 2014
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.

avatar nikosdion
nikosdion - comment - 9 Dec 2014

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.

avatar infograf768
infograf768 - comment - 9 Dec 2014

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

avatar nikosdion
nikosdion - comment - 9 Dec 2014

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

avatar phproberto
phproberto - comment - 9 Dec 2014

For the undefined alert messages please check:

#5366

avatar richard67
richard67 - comment - 9 Dec 2014

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?

avatar waader
waader - comment - 9 Dec 2014

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

avatar zero-24 zero-24 - alter_testresult - 15 Dec 2014 - richard67: Not tested
avatar richard67
richard67 - comment - 16 Dec 2014

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

avatar nikosdion
nikosdion - comment - 20 Dec 2014

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

avatar brianteeman brianteeman - alter_testresult - 20 Dec 2014 - nikosdion: Tested successfully
avatar brianteeman brianteeman - change - 20 Dec 2014
Status Pending Ready to Commit
avatar brianteeman brianteeman - change - 20 Dec 2014
Labels Added: ?
avatar infograf768 infograf768 - close - 21 Dec 2014
avatar zero-24 zero-24 - close - 21 Dec 2014
avatar infograf768 infograf768 - change - 21 Dec 2014
Status Ready to Commit Closed
Closed_Date 0000-00-00 00:00:00 2014-12-21 09:08:07
avatar infograf768
infograf768 - comment - 21 Dec 2014

@test OK here too. Thanks. Merging.

avatar zero-24 zero-24 - change - 14 Oct 2015
Labels Removed: ?

Add a Comment

Login with GitHub to post a comment