FIX: SQL error "unknown column p.fk_soc"

[atm-florianm]

FIX error on home page

To reproduce the bug:

1. remove the right societe->client->voir from your user
2. remove any cache files for statistics (documents/user/temp/*)
3. go to home page of Dolibarr

Result:

Latest database access request error: SELECT product.ref, COUNT(product.ref) as nb, SUM(tl.total_ht) as total, AVG(tl.total_ht) as avg FROM llx_propal as p, llx_propaldet as tl, llx_product as product INNER JOIN llx_societe_commerciaux as sc ON p.fk_soc = sc.fk_soc AND sc.fk_user = 1 WHERE p.entity IN (1) AND p.rowid = tl.fk_propal AND tl.fk_product = product.rowid AND p.datep BETWEEN '2023-01-01 00:00:00' AND '2023-12-31 23:59:59' GROUP BY product.ref ORDER BY nb DESC
Return code for latest database access request error: DB_ERROR_NOSUCHFIELD
Information for latest database access request error: Unknown column 'p.fk_soc' in 'on clause'

Analysis

After researching it a bit, I found this stackoverflow topic which explains the problem: ANSI-92 joins (i.e. joins using the LEFT JOIN, INNER JOIN etc. keywords) take precedence over ANSI-89 joins (i.e. joins by comma in table list) and therefore the join on llx_societe_commerciaux occurs before the join on llx_propal but its ON clause requires llx_propal to be joined first.

Basically,

SELECT * from A, B left join C on C.xyz = A.xyz;
-- → error: unknown column 'A.xyz' in 'on clause'

is not the same as

SELECT * from (A, B) left join C on C.xyz = A.xyz;
-- → no error

The responder also recommended not to mix the two styles of joins in one request, but I found it easier to not rewrite the whole query (but we could keep this in mind for the future).

[eldy]

The syntax using the ( ) does not works on all database version.
The good solution is to have all INNER or OUTER JOIN always just after the table linked so

$sql .= " FROM ".$this->from;
if (empty($user->rights->societe->client->voir) && !$user->socid) {
			$sql .= "  INNER JOIN ".MAIN_DB_PREFIX."societe_commerciaux as sc ON p.fk_soc = sc.fk_soc AND sc.fk_user = ".((int) $user->id);
		}
$sql .=", ".$this->from_line.", ".MAIN_DB_PREFIX."product as product";

In your example:
SELECT * from A left join C on C.xyz = A.xyz, B ...
is real standard SQL that will work for all databases and versions.

[atm-florianm]

I replaced my earlier version using parentheses with a new proposal using only explicit inner joins: would this be ok? I think it makes the query more consistent by using only one style of join.

[eldy]

Yes, using inner join is also possible.

[eldy]

Quotes for php string used to forge SQL must be " because the ' is used for the SQL string inside the PHP string.
Thanks to restore like in previous code, this will solve the phpunit regression reporting trouble in sql forging syntax.

[atm-florianm]

Sorry for taking so long: initially I didn't understand your request because the only part of this SQL query that has inner strings is built on line 259 and was already using single quotes inside (SQL) and double quotes outside (PHP), so technically, there was no problem with the SQL string in the end. The reason why I had changed the quotation style in the first place was to comply with the .editorconfig rules for Dolibarr (using autoformat).

But now I think I understand that Travis has a more complex (and stringent) ruleset for Dolibarr, so we can't reliably use autoformat (even with the right .editorconfig) when sql strings are involved… or am I mistaken?

[eldy]

I use autoformat with no trouble. But if your autoformat include a feature to transform ' into ", you must disable it because we must use " for PHP string when forging a SQL request so we can use the ' for SQL string. I don't have such autoformat feature on eclipse so i do not expereince troubles.