Thanks for the fix. I installed mssql with the test sample data. When you go to the frontend you will get the following warning:

Warning: Invalid argument supplied for foreach() in joomla34\libraries\joomla\database\query\sqlsrv.php on line 388

Could you have a look at this?

Please fix the codestyle issues that Travis is complaining about.

@waader Yes, I'll take a look.

@Hackwar Not a problem.

@waader Please take a look at my latest commit. I've fixed the issue you described.

@Imcculley works great! Well done!

I was a bit to quick. There is a problem in the backend. When I go in the article manager for example I get this message: 42000 [Microsoft][SQL Server Native Client 11.0][SQL Server]Incorrect syntax near the keyword 'ORDER'.SQL=SELECT [a].[id] AS [value], [a].[title] AS [text] FROM [ghu8q_viewlevels] AS [a] GROUP BY ORDER BY [a].[ordering] ASC,[title] ASC

Can you have look at this problem?

This is a different issue, altogether. This is related to the order function of the query class. If you would be so kind as to open an issue for this specific problem and I would be glad to take a look.

Actually, on second thought, it could be related.

Let me take a look and get back to you.

@waader Please take a look at my latest commit.

@waader Have you had a chance to test my latest commit?

@Imcculley Sorry for the delay, I was not in front of my computer for some days.

A quick test was successful but I will do some more testing later this day. As the sql server implementation has serveral problems it is not easy sometimes to differentiate between the various causes.

Is there any word as to if/when this PR will be accepted?

A PR to be comitted needs a least two successful tests.

"The problem" I have with this patch is that it took my some time to chart all the problems with a mssql installation. Besides that there are some problems that prevent extensive testing. To illustrate this I created #6073 as an example.

So I did compared installations with different testings data and compared the result to a installation with your patch applied.

Meanwhile there is only one difference that I could find. Install joomla with the "testing sample data" and in the "All Modules" menu select "Search" and put an abitrary search term in the search box.

Without your patch I get:
42000 [Microsoft][SQL Server Native Client 11.0][SQL Server]Column 'pudcl_contact_details.name' is invalid in the select list because it is not contained in either an aggregate function or the GROUP BY clause.SQL=SELECT a.name AS title, '' AS created, a.con_position, a.misc, CASE WHEN DATALENGTH(a.alias) != 0 THEN (CAST(a.id as NVARCHAR(10))+':'+a.alias) ELSE CAST(a.id as NVARCHAR(10)) END as slug, CASE WHEN DATALENGTH(c.alias) != 0 THEN (CAST(c.id as NVARCHAR(10))+':'+c.alias) ELSE CAST(c.id as NVARCHAR(10)) END as catslug, (a.name+','+a.con_position+','+a.misc) AS text,('Contacts'+' / '+c.title) AS section,'2' AS browsernav , ROW_NUMBER() OVER (ORDER BY a.name DESC) AS RowNumber FROM pudcl_contact_details AS a INNER JOIN pudcl_categories AS c ON c.id = a.catid WHERE (a.name LIKE '%joomla%' OR a.misc LIKE '%joomla%' OR a.con_position LIKE '%joomla%' OR a.address LIKE '%joomla%' OR a.suburb LIKE '%joomla%' OR a.state LIKE '%joomla%' OR a.country LIKE '%joomla%' OR a.postcode LIKE '%joomla%' OR a.telephone LIKE '%joomla%' OR a.fax LIKE '%joomla%') AND a.published IN (1,2) AND c.published=1 AND a.access IN (1,1) AND c.access IN (1,1) GROUP BY a.id, a.con_position, a.misc, c.alias, c.id

With your patch applied I get:
42000 [Microsoft][SQL Server Native Client 11.0][SQL Server]Each GROUP BY expression must contain at least one column that is not an outer reference.SQL=SELECT a.title, a.description AS text, '' AS created, '2' AS browsernav, a.id AS catid, CASE WHEN DATALENGTH(a.alias) != 0 THEN (CAST(a.id as NVARCHAR(10))+':'+a.alias) ELSE CAST(a.id as NVARCHAR(10)) END as slug , ROW_NUMBER() OVER (ORDER BY a.title DESC) AS RowNumber FROM mmj38_categories AS a WHERE (a.title LIKE '%test%' OR a.description LIKE '%test%') AND a.published IN (1,2) AND a.extension = 'com_content'AND a.access IN (1,1) GROUP BY a.id,a.title,a.description,a.alias,'','2'

I can´t say if this is good or bad. Can you have a look at this? When this is settled it´s a successful test for me.

@waader Thanks for taking the time to explain the process to me.

I'll have a look at that problem ASAP.

@waader #6082 should fix the group by issue

@6082 + the one change in content.php I proposed fixes the issues in the search component. When I then activate your patch I get the following error:

[Microsoft][SQL Server Native Client 11.0][SQL Server]Each GROUP BY expression must contain at least one column that is not an outer reference.SQL=SELECT a.title, a.description AS text, '' AS created, '2' AS browsernav, a.id AS catid, CASE WHEN DATALENGTH(a.alias) != 0 THEN (CAST(a.id as NVARCHAR(10))+':'+a.alias) ELSE CAST(a.id as NVARCHAR(10)) END as slug , ROW_NUMBER() OVER (ORDER BY a.title DESC) AS RowNumber FROM e73fz_categories AS a WHERE (a.title LIKE '%modul%' OR a.description LIKE '%modul%') AND a.published IN (1,2) AND a.extension = 'com_content'AND a.access IN (1,1) GROUP BY a.id,a.title,a.description,a.alias,'','2'

@lmcculley Can you have also have a look at that? Thanks!

@waader Yes, not a problem.

@waader Please have a look at my latest patch.

Any news on this PR?

I will came back to this PR but before that I have to check some other issues with mssql to make this a valid test.

I tested the original problem - #5915 - with the current staging branch and I could not reproduce the problem any more. Can you confirm that? Can you give me, for testing purposes, another example of a malformed group by statement that your patch corrects.

The thing is: when a group by statement is malformed it is more often than not only a problem for mssql but also for postgresql. See #6159 for example. I found this problem with postgresql but it also exists in mssql. Another case is #6135. This corrects a sorting problem in postgresql but it also corrects a much severe problem in mssql. You now could insert/edit articles what wasn´t possible before.

So in the meantime I am not sure if it is not better to solve each problem on its own.

@waader this boils down to a single question: Is it better to ensure every single query is mysql, postgresql and mssql compliant, or is it better to ensure the driver does the job, creating a truly agnostic framework.

Also, I think this patch is necessary because of all of the third-party tools that would need to be updated in order to be ms sql/postgresql compliant.

let me disagree the issues listed here are caused by bad SQL query or better by MySQL query only
for this reason i don't think we need to "fix" the GROUP BY clause by code
we need to write portable SQL query

@alikon I honestly have a very hard time understanding what you said, other than "we need to write portable SQL query". Please, try to clarify your statement.

The point of an abstraction is to allow developers to use it, and remove themselves from the particularities of any one implementation. If you're going to provide an abstraction to any kind of service that has multiple, different underlying implementations (e.g. databases), that abstraction should be able to handle as many edge cases as possible. For example how MySQL handles GROUP BY clauses vs MS SQL and PostgreSQL.

Sorry for my brevity I'm on phone
What I want to say is that SQL have a standard
So from my POV developers should write SQL standard And not sql dialect like mysql
The group by here is written in mysql dialect
Should be written instead in SQL language
This is why I dislike your PR on sqlsrv

@Imcculley ad abstraction) I got the idea behind your patch right from the start. That´s why I was excited at first. The question is whether it is possible to make this robust to work with all the exceptions. I will keep testing this when I do further testing with the mssql installation.

@waader Thanks! I look forward to getting any feedback I can.

@alikon do we still have an issue that needs to be resolved or was it resolved elsewhere

@brianteeman it's hard for me to give a correct answer at this time, cause currently i'm not able to use a sqlsrv instance to test, i hope @waader can help us on this cause it is only a sqlsrv related issue

I plan to have a sqlserv instance for the sprint

I used this patch often when I encountered a group by problem with mssql. But because of some edge cases (the old 80:20 problem) I think there is no silver bullet to this problem algorithmically.

Then again: solving the group by problems on the part of postgresql more often than not solves the problem for mssql too. And when aliases are not used in the group by statement than the rate will go up to 100%.

The last time I checked mssql was in a pretty good shape compared to the past. Developers really should form a habit of writing group by statements conforming to the sql standards. Then the extra amount to solve the remaining edge cases will be very small.