? Success

User tests: Successful: Unsuccessful:

avatar williamsandy
williamsandy
5 Sep 2016

Pull Request for Issue #11279

Summary of Changes

Although this pull request #5929 did a great Job of fixing issues caused by MySQL specific queries using a group by clause, there were still a few issues left.

This pull request was created to address those issues:

  1. It kind of fell over when queries used an alias for columns and tables without using the 'AS' keyword. Although it is good practice to use the AS keyword, it is not compulsory.
  2. No support for 'SELECT ALL GROUP BY' queries on a single table such as 'SELECT * from Mytable GROUP BY catid'
  3. Failed to get table fields when the table prefix is hardcoded in the query or explicitly already given by the direct use of $db->getPrefix().
  4. No check to see if the query contains any joins before trying to get columns for any join tables.
  5. The 'select' and 'join' functions of the JDatabaseQuery object can be called any number of times and in any order. So this means currently depending on what point the group function is called you may or may not have the complete select and join table lists.

Changes applied in this PR fixes issues #11278 and #11279

Testing Instructions

Documentation Changes Required

avatar williamsandy williamsandy - open - 5 Sep 2016
avatar williamsandy williamsandy - change - 5 Sep 2016
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 5 Sep 2016
Category Libraries
avatar joomla-cms-bot joomla-cms-bot - change - 5 Sep 2016
Labels Added: ?
avatar waader
waader - comment - 5 Sep 2016

I made a quick check and noticed that eg. some warnings are gone. But it is not easy to test this one properly as there are currently some problems with a joomla mssql installation. Take for example this one: #11932

This particular problem exists with or without your patch. I am not able to judge without more investigation if this falls in the scope of your patch or not.

Or try out a multilingual installation. There you will find error messages all over the place.

How would you tackle this problem?

avatar williamsandy
williamsandy - comment - 6 Sep 2016

@waader, to test this PR you can test to see if issues #11278 and #11279 have been resolved. This PR only fixes the 'select group by' issue caused by MySQL focus queries. MySQL does not require you to put all the columns in your select clause in the group by clause because it will work it out internally. This is not standard SQL so hence the issue with MSSQL.

I did a quick look into PR #11853 and can confirm that this is the patch causing the issue #11932 you raised.

The problem caused in issue #11932 is due to line 200 in the newly added JHelperUsergroups class.

So in the 'total' function they are doing the following:

$query = $db->getQuery(true)
->select('count(id)')
->from('#__usergroups');
$db->setQuery($query, 0, 1);

I am happy to see in the query they are doing the count on the primary key as opposed to doing count(*) as what is usually done. This means this query can utilize the primary key index as opposed to having to go through the whole table.

However, I can not see why we are setting a limit on a simple count query. The result will always be a single scalar value. I can see that PR #111853 is all about performance improvements but in this case limiting the results returned from a simple count makes no difference because you already paid the expense on the count operation already.

I suggest changing line 200 to:

$db->setQuery($query);

This will stop Joomla from blowing up every time it wants to check the user's permissions in MSSQL. waader I am surprised with PR #11853 applied that you still can login into Joomla with MSSQL!

Okay, the reason we get away with the limit on the count query in MySQL is that MySQL just uses the LIMIT clause, and it doesn't care.

So this

$query = $db->getQuery(true)
->select('count(id)')
->from('#__usergroups');
$db->setQuery($query, 0, 1);

is converted to:

'SELECT COUNT(id) from #_usergroups LIMIT 0, 1'

WHERE as in MSSQL this is not the case, and you have to do the following:

Using the WITH clause and CTE (MSSQL 2005+)

WITH cte AS
( SELECT count(id) , ROW_NUMBER() OVER (ORDER BY (select 0)) AS RowNumber FROM #__usergroups)
SELECT * FROM cte WHERE cte.RowNumber BETWEEN 1 AND 1;

Or not using the WITH clause (MSSQL 2000+)

SELECT * FROM ( SELECT count(id) , ROW_NUMBER() OVER (ORDER BY (select 0)) AS RowNumber FROM #__usergroups) A WHERE A.RowNumber BETWEEN 1 AND 1 ;

In either case, you are creating a derived table where every column has to be named, which is not the case and hence the error you are getting in MSSQL.

So waader I suggest changing line 200 as I suggested in the libraries/cms/helper/usergroups.php file to get Joomla working again with PR #11853 on SQL Server.

avatar mbabker
mbabker - comment - 21 May 2017

This PR needs to be synchronized to the current development branch to be tested/reviewed.

avatar joomla-cms-bot joomla-cms-bot - change - 21 May 2017
The description was changed
avatar joomla-cms-bot joomla-cms-bot - edited - 21 May 2017
avatar franz-wohlkoenig franz-wohlkoenig - change - 22 May 2017
Status Pending Information Required
avatar joomla-cms-bot joomla-cms-bot - change - 22 May 2017
Category Libraries MS SQL Libraries
avatar franz-wohlkoenig
franz-wohlkoenig - comment - 27 May 2017

If this PR get no Response, it will be closed at 22th June 2017.

avatar joomla-cms-bot joomla-cms-bot - close - 22 Jun 2017
avatar franz-wohlkoenig franz-wohlkoenig - change - 22 Jun 2017
Status Information Required Closed - No Reply
Closed_Date 0000-00-00 00:00:00 2017-06-22 06:40:15
Closed_By franz-wohlkoenig
avatar franz-wohlkoenig
franz-wohlkoenig - comment - 22 Jun 2017

closed as stated above.


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

avatar joomla-cms-bot
joomla-cms-bot - comment - 22 Jun 2017

Add a Comment

Login with GitHub to post a comment