SQLite Greatest implementation is wrong for parameters with the value NULL

Testing shows this to be functionally equivalent and it is much easier to read.

I would prefer a standard if over the ternary if to make it even more readable but either way works.
I'm marking rtbc but will happily review again if you change it.

I'd prefer the if as well - wouldn't this be enough?

<?php
if (!in_array(NULL, $args, TRUE)) {
return max($args);
}
}

Don't you mean NULL rather than $null?

What about GREATEST() with no arguments? Should this return NULL or should it break?

#10:

mysql> SELECT GREATEST();
ERROR 1582 (42000): Incorrect parameter count in the call to native function 'GREATEST'
mysql> SELECT GREATEST(1);
ERROR 1582 (42000): Incorrect parameter count in the call to native function 'GREATEST'

So, in MySQL at least it requires two arguments.
But should we try to replicate this in PHP?

What we should not do is silently fail. Either we return NULL or we throw an exception.

EDIT:
Oracle docs suggest that only one argument is required:
http://docs.oracle.com/html/A95915_01/sqfunc.htm#i1005813

Other question: How reliable is max() for strings or datetime?
For my taste we should postpone this question to a follow-up, where we can also add tests:
#2262425: Does PHP max() correctly replicate SQL GREATEST() for non-numeric values?

Another thing to do here and now: Add a nice docblock.

Btw, the existing implementation is not just misleading but wrong and buggy, as it can lead to count(UNDEFINED).
Maybe previous PHP versions were silent about this, but it is definitely wrong.
I am changing this to a bug report.

(Originally i came from here: #2259947: Minor bug fixes in database system)

This patch still applies. Are there any db-driver-specific tests? Could this be tested? I don't think the testbot currently runs on sqlite, but it eventually will (hopefully) run on all supported db drivers.

Moving to the right component.

The patch looks good to me.

The documentation from MySQL states tht there be a minimum of two parameters for the GREATEST function. It also states that if one parameter is NULL that the function will return NULL. This patch does both.

The patch still applies, so for me it is RTBC

It would be nice to have some tests for this.

With new tests for this we have to watch out for any problems with a PostgreSQL database.

Tests added.

Minor change to the added tests.

Rerolled the patch from comment #22, because ConnectionTest has been moved.

After some strange problems with the testbot it is now happy. So back to needs review.

The NULL is interesting, I actually tried a quick fix in #2810325: Undefined variable $args in sqlFunctionGreatest() and got another exception as a result of that.

This happened for me in a kernel test that saved a user, so it tried to write to the sequence table.

Changing the title at @Berdirs request.

Yes, tried with this patch applied now and this is the error that I get now:

General error: 20 datatype mismatch: UPDATE {sequences} SET value = GREATEST(value, :existing_id) + 1

So I guess it wants to do NULL + 1. Still not sure why one of the values is NULL in my case.

Patch or MR doesn't apply anymore
The last patch or MR doesn't apply to the target branch, please reroll the code so that it can be reviewed by the automated testbot.

Would like to know if still valid bug though after 6 years

I started triage on #1425794: SQLite exception "primary key must be unique" when sequences table erroneously contains more than one row and found my way to this issue. The problematic code is still in sqlFunctionGreatest().

I decided to update the patch and add tests.

Let's try this. Still having trouble with PHPStan locally so I can't test this.

IS seems to match the proposed patch
#48 has a fail patch to show the tests are covering the change
All green across the different databases.

Think this is good.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.