Replicate SQL LEAST() in SQLite

Hi @julienjoye: Thank you for your patch!

The patch looks good. To get this patch committed there needs to be automated testing. We want to be sure that the new code does what it suppost to do and we want to make sure that somebody later does not accidently removes the added code. My suggestion would be to add what we call a KernelTest. See:https://www.drupal.org/docs/8/testing/types-of-tests-in-drupal-8. Add a new test class called SelectLeastTest in the directory "core/tests/Drupal/KernelTests/Core/Database/". In that class add a test with a provider method. See: https://phpunit.readthedocs.io/en/9.0/writing-tests-for-phpunit.html#dat... and search our code for the key "dataProvider". I will help you and review your patch.

Preapred test case only.

@Kumar Kundan: Thank you for working on this issue.

+++ b/core/tests/Drupal/KernelTests/Core/Database/SelectLeastTest.php
@@ -0,0 +1,36 @@
+  public function testSelectLeast($arg1, $arg2, $arg3, $arg4, $arg5, $expected) {
+    $this->assertSame($expected, min($arg1, $arg2, $arg3, $arg4, $arg5));
+  }

The test in not testing the SQL LEAST operator. Can we change the test to:

  /**
   * Tests the SQL LEAST operator.
   *
   * @dataProvider selectLeastProvider
   */
  public function testSelectLeast($values, $expected) {
    $least = Database::getConnection()->query("SELECT LEAST(:values[])", [':values[]' => $values])->fetchField();
    $this->assertEquals($expected, $least);
  }

To make this test work the test need to be a Kernel test.
I changed the row of arguments into an array. In this way we can test more combinations.

Thanks @daffie. I started working on this approach.

I am working for Patch Improvment of SQLite database.

Patch looks a lot better.

1. +++ b/core/lib/Drupal/Core/Database/Driver/sqlite/Connection.php
   @@ -239,6 +240,15 @@ public static function sqlFunctionGreatest() {
   +    $values = array_filter(func_get_args(), 'strlen');
   

   Could we add a comment to this line. Something like: "Remove all NULL, FALSE and empty strings values but leaves 0 (zero) values."

2. +++ b/core/tests/Drupal/KernelTests/Core/Database/SelectLeastTest.php
   @@ -0,0 +1,35 @@
   +      [['A', 'B', 'C', 'NULL'], 'A'],
   +      [['A', 'B', 'C', 'NULL'], 'A'],
   +      [[1, 2, 3, -1, 'NULL', 'FALSE', 0], -1],
   +      [[1, 2, 3, -1, 'NULL', 'FALSE', 0], -1]
   

   These line a double. This is not necessary.

3. +++ b/core/tests/Drupal/KernelTests/Core/Database/SelectLeastTest.php
   @@ -0,0 +1,35 @@
   + * Tests THE SQL LEAST operator.
   

   Could we change the word "THE" to lowercase.

4. The testbot fails for the PostgreSQL database. This is because PostgreSQL order the value NULL differently.

5. +++ b/core/tests/Drupal/KernelTests/Core/Database/SelectLeastTest.php
   @@ -0,0 +1,35 @@
   +      [[1, 2, 3, -1, 'NULL', 'FALSE', 0], -1],
   

   This line fails for PostgreSQL, because it interpreted the values as strings and not as intergers. I think it is best just to remove lines with negative numbers.

Here I have tried to address comment #14, please review.

+++ b/core/tests/Drupal/KernelTests/Core/Database/SelectLeastTest.php
@@ -0,0 +1,33 @@
+      [['A', 'B', 'C', 'NULL'], 'A'],
+      [['A', 'B', 'C', 'NULL'], 'A'],

These are double. We do not need to test the same thing twice.

Here I have addressed comment #17

Changing status to needs review as patch #18 passed the tests.

Patch looks good. Almost RTBC.

1. +++ b/core/tests/Drupal/KernelTests/Core/Database/SelectLeastTest.php
   @@ -0,0 +1,32 @@
   +      [['A', 'B', 'C', 'NULL', 'F', 0], 'A'],
   

   Can we remove the number zero fro the values array. The function LEAST is made for comparing items of the SAME kind.

2. There are also a number of coding standard violation that need to be fixed. See: https://www.drupal.org/pift-ci-job/1775696

#23.2 still needs to be addressed.

Here I have addressed #23.2.

All code changes look good to me.
There is testing added for the added functionality.
For me it is RTBC.

+++ b/core/tests/Drupal/KernelTests/Core/Database/SelectLeastTest.php
@@ -0,0 +1,32 @@
+    $least = $this->connection->query("SELECT LEAST(:values[])", [':values[]' => $values])->fetchField();

One question, instead of using raw SQL query, is it possible using $this->connection->select() here?

One question, instead of using raw SQL query, is it possible using $this->connection->select() here?

That is something that can be done. In combination with ->addExpression('LEAST(1,2,3,4,5)', 'num'). Only I do not see how that will improve the testing coverage.

@daffie, thanks, just asking.

Committed b5343b9 and pushed to 9.1.x. Thanks!

SQL Server does not support the LEAST operator, so there are now many core testing failures under sqlsrv.