Refrain from using the || operator for concatenation

If CONCAT() is part of ANSI SQL, sounds fine to me. It should be added to the handbook page.

Perhaps we should consider adding a link from the main docblock for the DB layer to the handbook page when it's more fleshed out.

As far as I know, all Oracle platforms support ||

CONCAT() doesn't exist in PostgreSQL, but you could write your own.

|| is the ANSI SQL standard and is supported by everything but MSSQL. (Shocker, I know.) http://en.wikibooks.org/wiki/SQL_Dialects_Reference/Functions_and_expres...

Hm. What about hshana's point in #4? I'd rather go with "ANSI and supported by the core DBs and some others" than "not-ANSI and supported by the core DB and some others", for different "others".

What about #3?

Since CONCAT() doesn't work in PostgreSQL, we'd need to create our own (like here)

Good to know, thanks for clearing it up.

OK, that makes sense. Thanks, DamZ. +1 on #5 then.

Do we know that any other databases we may want to support can be made to handle CONCAT() the way we do with Postgres?

Wow. just wow. Dont we all love standards...

SQL--

That's all I have to say.

Two instances of "ANSI" left in the function descriptions.
Since test bot will test RTBC patches, leaving there.

Committed to CVS HEAD. Thanks.

Just installed HEAD on PostgreSQL 8.3 and test failed:

PDOException: SQLSTATE[42883]: Undefined function: 7 ERROR: function concat(bigint, unknown) does not exist LINE 1: SELECT CONCAT($1, CONCAT(name, CONCAT($2, CONCAT(age, $3))))... ^ HINT: No function matches the given name and argument types. You might need to add explicit type casts.: SELECT CONCAT(:a1, CONCAT(name, CONCAT(:a2, CONCAT(age, :a3)))) FROM {test} WHERE age = :age; Array ( [:a1] => The age of [:a2] => is [:a3] => . [:age] => 25 )  in DrupalTestCase->run() (line 442 of /home/josh/projects/drupal7/modules/simpletest/drupal_web_test_case.php).

Here is our existing code:

if (!db_query("SELECT COUNT(*) FROM pg_proc WHERE proname = 'concat'")->fetchField()) {
   db_query('CREATE OR REPLACE FUNCTION "concat"(text, text) RETURNS text AS
      \'SELECT $1 || $2;\'
      LANGUAGE \'sql\''
   );
}

The problem is for every argument data type you can pass to CONCAT you need to specify another function. Ugly.

Hardly. You just need to add casts.

I have given up on using CONCAT() in SQL. I honestly think we need some kind of $query->concat() function that will return the proper concatenation SQL for each database driver. See #319070: Provide a wrapper for cross-db and cross-field-type concatenation (PosgreSQL).

Last I checked, Crell was against smaller functions like that, though personally, I'm for it.

How many of the situations where CONCAT() is used can't be designed around? Looking at the bug issues involving it, I get the feeling that most can.

Most of the CAST() issues are related to the use of CONACT() too, so trying to move away from its use would reduce or eliminate two of the big areas of database compatibility problems.

I think that any documentation on this issue should suggest in strong terms you need to think hard, at least twice, before deciding that CONCAT() and therefore probably CAST() are part of the solution to your problem.

Document that SQL strings cant use concat, add a concat to mapConditionOperator and live happily ever after.

I don't think mapConditionOperator can handle concat in its current form. That's not really what it's there for.

regex for drivers who don't support ||?

if (strpos($sql, '||')) {
    $sql = preg_replace('/([^,\s]+)?\s\|\|?\s([^,\s]+)/', 'CONCAT($1, $2)', $sql);
}

regex for drivers who don't support ||?

if (strpos($sql, '||')) {
    $sql = preg_replace('/([^,\s]+)?\s\|\|?\s([^,\s]+)/', 'CONCAT($1, $2)', $sql);
}

string parsing is not a solution. Damien is right -- we might need a separate concat method :(

No, I don't think REGEX are a good solution for SQL. But I knew it would stir people up! ;)

DamZ solution seems reasonable to me. 3 CONCAT functions. If it makes 'Basic SQL syntax tests" pass without an exception then I'm happy.

So to summarize then:

1) Document that || is verbotten.

2) We need a new patch that makes all 3 variants of Concat() for Postgres. Who's doing that one?

3) Do we need anything for SQLite?

Attached is PostgreSQL funcs. Have tested and they work.

MySQL CONCAT takes any number of parameters -- but then I guess we can document that plz do not use such CONCAT (good luck with people keeping that one...)

The concat(text, text) function is already created several lines before the place where this patch is applied, so we are creating/replacing the function twice for the (text, text) arguments.

Also, in HEAD the patch applied with an offset of 32 lines.

My opinion in the patch name but meh.

Agreed, they're not critical. Where is that major priority anyway? :(

Committed to HEAD all the same, though.

:)