[PHPBB3-10296] incorrect cross join in SQL Server Created: 28/Jul/11  Updated: 22/Jan/17  Resolved: 18/Nov/11

Status: Closed
Project: phpBB3
Component/s: Database Abstraction Layer (DBAL)
Affects Version/s: 3.0.9
Fix Version/s: 3.0.10-RC1

Type: Bug Priority: Blocker
Reporter: dmauri Assignee: Joas Schilling
Resolution: Fixed Votes: 0
Labels: None
Environment:

PHP 5.1.2, SQL Server 2008, Any Browser


GitHub Pull Request URL: https://github.com/nickvergessen/phpbb3/compare/phpbb:develop-olympus...nickvergessen:ticket/10296
Fix URL: https://github.com/phpbb/phpbb3/pull/468

 Description   

Cross Joins are done using an approach like this:
table_a, table_b
but this generates an errore when there is also an inner join. This query, for example, is generated when trying to send a Mass Email to a group:

SELECT u.user_email, u.username, u.username_clean, u.user_lang, u.user_jabber, u.user_notify_type FROM phpbb_users u, phpbb_user_group ug LEFT JOIN phpbb_banlist b ON (u.user_id = b.ban_userid) WHERE ug.group_id = 8 AND ug.user_pending = 0 AND u.user_id = ug.user_id AND u.user_allow_massemail = 1 AND u.user_type IN (0, 3) AND (b.ban_id IS NULL OR b.ban_exclude = 1) ORDER BY u.user_lang, u.user_notify_type

and produces the following error:

SQLSTATE: 42000 code: 4104 message: [Microsoft][SQL Server Native Client 10.0][SQL Server]The multi-part identifier "u.user_id" could not be bound. [4104]

The problem can be solved using the "CROSS JOIN" keyword, instead of the comma, at row 612 of dbal.php file:

$sql .= $this->_sql_custom_build('FROM', implode(' CROSS JOIN ', $table_array));

After this change the query will be the following

SELECT u.user_email, u.username, u.username_clean, u.user_lang, u.user_jabber, u.user_notify_type FROM phpbb_users u CROSS JOIN phpbb_user_group ug LEFT JOIN phpbb_banlist b ON (u.user_id = b.ban_userid) WHERE ug.group_id = 8 AND ug.user_pending = 0 AND u.user_id = ug.user_id AND u.user_allow_massemail = 1 AND u.user_type IN (0, 3) AND (b.ban_id IS NULL OR b.ban_exclude = 1) ORDER BY u.user_lang, u.user_notify_type

and it will be correctly executed.



 Comments   
Comment by Oleg [X] (Inactive) [ 30/Jul/11 ]

Shouldn't this also affect mysql? Mysql recommends parenthesizing joined tables as follows: select * from (a, b) inner join c. Does this work for mssql?

"In MySQL, CROSS JOIN is a syntactic equivalent to INNER JOIN (they can replace each other). In standard SQL, they are not equivalent. INNER JOIN is used with an ON clause, CROSS JOIN is used otherwise. "

http://dev.mysql.com/doc/refman/5.0/en/join.html

Comment by Noxwizard [ 30/Jul/11 ]

MySQL isn't affected because its _sql_custom_build() function parenthesizes the list. This only seems to affect MSSQL, PostgreSQL, and Oracle. A simple fix would be to swap around lines 88 and 89 of includes/acp/acp_email.php (https://github.com/phpbb/phpbb3/blob/develop-olympus/phpBB/includes/acp/acp_email.php#L88)

Comment by Oleg [X] (Inactive) [ 30/Jul/11 ]

Changing client code to work around a problem in sql generation by dbal would classify as an ugly hack rather than a fix.

Looking at _sql_custom_build for mysql, writing a version that uses cross join instead of commas would at a minimum be ugly (as it would essentially have to do search and replace in sql) and potentially fragile/impossible (if from expressions can have entire subqueries). If we can simply parenthesize the entire expression as mysql does this would seem to me like the least invasive and a very reliable approach.

Comment by Oleg [X] (Inactive) [ 30/Jul/11 ]

To the submitter, which page were you getting the error on?

Comment by dmauri [ 01/Aug/11 ]

Hi Oleg. No, putting the tables that have to be cross-joined into parenthesis doesn't work. IMHO, since mysql doesn't follow the ANSI Standard, the query should be generated with the CROSS JOIN by default, switching to the comma-separated style only if the mysql driver has been choosen. This should be fairly easy to do, since the the only section that needs to be chaged is the _sql_custom_build function.

Following this approach (but in the opposite way, just to make may installation works) I changed the code of the mssqslnative.php file in this way:

function _sql_custom_build($stage, $data)
{
switch ($stage)

{ case 'FROM': $data = str_replace(",", " CROSS JOIN ", $data); break; }

return $data;
}

and everything seems to work right now.

> To the submitter, which page were you getting the error on?

Mass Email

Comment by Joas Schilling [ 14/Nov/11 ]

Please also add a test case to the unittests for this!

Comment by Joas Schilling [ 15/Nov/11 ]

Ticket author should be asked for his correct email before merging.

Noxwizard tested Oracle and MSSQL (both fixed), I tested mysql and sqlite (both not affected) and postgres (fixed)

Generated at Sat Oct 20 12:58:24 UTC 2018 using JIRA 7.9.2#79002-sha1:3bb15b68ecd99a30eb364c4c1a393359bcad6278.