The function testNumericExpressionSubstitution() in the file drupal/core/tests/Drupal/KernelTests/Core/Database/QueryTest.php uses the SQL statement SELECT COUNT(*) >= 3 FROM {test} and SELECT COUNT(*) >= :count FROM {test} to ensure that both return the same result, and that numeric substitution can occur in expressions. MS SQL Server chokes on the first statement. It does not like comparison operators in select expressions. Would it be possible to either flag this test for the core database drivers only, or change the test to a more universal expression like SELECT min(3, :value) AS result from the original bug report which drove the creation of this test?

CommentFileSizeAuthor
#22 drupal-3108025-testNumericExpressionSubstitution-6.patch1.02 KBdaffie
#17 3108025-17.patch3.97 KBalexpott
#17 11-17-interdiff.txt3 KBalexpott
#11 drupal-3108025-testNumericExpressionSubstitution-11.patch997 bytesBeakerboy
#9 drupal-3108025-testNumericExpressionSubstitution-9.patch1.04 KBbohart
#7 drupal-3108025-testNumericExpressionSubstitution-6.patch1.02 KBBeakerboy
#5 drupal-3108025-testNumericExpressionSubstitution-5.patch1.02 KBBeakerboy
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Beakerboy created an issue. See original summary.

Beakerboy’s picture

Issue summary: View changes
Beakerboy’s picture

Issue summary: View changes
Beakerboy’s picture

I just checked SQL Server, and it does not support inline min(var1, var2, var3) syntax. It only works by aggregating one column. Any suggestions on a more universal expression that meets the criteria for the test is appreciated. Arithmetic operations would probably be the way to go. SELECT COUNT(*) + 3 from {test} and SELECT COUNT(*) + :count from {test}.

Beakerboy’s picture

Status: Needs review » Needs work
daffie’s picture

Status: Needs review » Needs work

I think that the whole test can be removed. According to the docblock this is for a PHP bug. See: https://bugs.php.net/bug.php?id=45259.
This bug is over 10 years old. In PHP 5.2. We do not support that version any more.

bohart’s picture

Got the same issue while working with Oracle Driver.

The inputs are:

a) This test is broken on non-core database drivers.
(like mentioned MS SQL Driver and another example is Oracle Driver)

b) Basically, SQL query in the test isn't correct: SELECT COUNT(*) >= :count FROM {test}
The correct syntax is: SELECT COUNT(*) FROM {test} HAVING COUNT(*) >= :count

There is no usage of such syntax in the core (usage for >= expression in the SELECT section of the query).
And this syntax is not required for the test itself (to test numeric substitution).

c) The php bug is related to PHP 5.2 https://bugs.php.net/bug.php?id=45259
if this bug actual to currently supported PHP? (the changes related to this bug was committed back in 2015.
if no, \Drupal\Core\Database\Driver\sqlite\Statement::getStatement() can be simplified/refactored.

The possible way to go is:

Step 1) Update the test to use proper SQL syntax.
So, non-core drivers will not fail on it, but the test will check the bug as expected.
Patch attached.

Step 2) Open another issue or reopen #2428695: SQLite date handling is wrongly implemented and arguments handling needs override:
а) To unsure the PHP bug is not actual for currently supported PHP versions.
b) To refactor \Drupal\Core\Database\Driver\sqlite\Statement::getStatement() to remove rudement of this PHP bug support.
c) To remove testNumericExpressionSubstitution test case at all.

Beakerboy’s picture

I’m fine with dropping the whole test. @bohart, you may also be interested in https://www.drupal.org/project/drupal/issues/3108287

Beakerboy’s picture

Status: Needs work » Needs review
FileSize
997 bytes

completely removed test

Beakerboy’s picture

@bohart, the point of the test is to demonstrate that the database can substitute a value in a select expression, not a having clause. I'm guessing the scenario you have proposed is already well tested within the core tests.

daffie’s picture

@Beakerboy and @bohart: I am working on getting testbots on Drupal.org for our contrib database drivers (SQL Server, OracleDB and MongoDB). See: #3106299: Create testbots for the contrib database drivers for OracleDB, Microsoft SQL Server and MongoDB. I have some operational questions about PHP there. If you know the answer to some of them it would be great.

The test is for testing a PHP bug (https://bugs.php.net/bug.php?id=45259).
The bug was in PHP version 5.2. We are not supporting that version at the moment.
The test is testing a SQL statement that is not correct.
The statement is using a group expression without a group or having in the statement.
The test is no longer necessary and therefore can be removed.
The patch looks good to me. For me it is RTBC.

daffie’s picture

mondrake’s picture

Hi all, good to see there is movement about getting contrib drivers tested over Drupal core testsuites!

Adding a related issue here, #3098426: EntityQueryTest::testToString fails with non-core db drivers. There is some discussion there about how to make dbdriver-dependent testing a bit more structured. Actually I believe a new issue should be spinned off. Would love to get ideas.

Beakerboy’s picture

@daffie, I cannot speak to whether or not the test SQL is ‘correct’. However, I know Boolean expressions are not supported as a return type on SQL Server, so if Drupal wants to ensure that parameter substitution in SELECT expressions is possible, than alternate syntax is needed. Obviously the bug was discovered because someone IS parameterizing select expresssions somewhere (maybe not in Drupal). I proposed the numerical expression in patch #6 in case Drupal dev team wanted to continue ensuring parameterization is possible. This seems like a reasonable requirement, and I would be surprised if #6 was unable to pass on other databases. If #6 DOES work on other systems, I would prefer using that over #11.

alexpott’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
3 KB
3.97 KB

We need to do more here. The same bug is referenced in \Drupal\Core\Database\Driver\sqlite\Statement::getStatement() - maybe we can make the sqlite driver not have so many customisations.

In the patch attached I remove some of the customisations but I wonder if we can completely remove \Drupal\Core\Database\Driver\sqlite\Statement::getStatement().

daffie’s picture

@alexpott: A lot of your test failures are related to #2031261: Make SQLite faster by combining multiple inserts and updates in a single query. In Drupal 9 the minimum requirement of SQLite will have the variable DSQLITE_MAX_VARIABLE_NUMBER set to a minimum of 250000. Hopefully that will fix your testbot failures. Do you want to wait for Drupal 9.1 to work on removing code from SQLite Statement class or do you want to create a followup for that?

Beakerboy’s picture

Title: Funtion testNumericExpressionSubstitution uses non-standard SQL » Function testNumericExpressionSubstitution uses non-standard SQL
Beakerboy’s picture

@daffie,
Would you like to re-add RTBC and indicate which patch you prefer? I feel the SQLite issues should have its own discussion, and the proposed solution breaks more than it fixes. Again, my vote is for patch 6 (comment #7) since it just changes the test instead of removing it completely. If someone can show that there are no instances in core (or major contrib modules) where SQL expression substitution occurs, then I would say it's OK to remove it.

alexpott’s picture

Status: Needs review » Needs work

I agree with @Beakerboy here - if we go for #7 then we don't remove the numeric expression test coverage and we can work on removing this test as part of #2031261: Make SQLite faster by combining multiple inserts and updates in a single query

Maybe someone want to re-upload #7 so it is the latest patch on the issue and then we can rtbc it.

Beakerboy’s picture

I don't see why we would ever want to remove this test completely. Keeping it will ensure that future and existing database drivers will, and will continue to work correctly, and have the feature sets that module developers expect. Tests suites are about stating expected behavior, and proving that expectations are met. If "Parameter substitution in expressions" is an expected feature, we need a test for it, even if the bug that prompted the original creation of the test is moot.

alexpott’s picture

@Beakerboy good point. But also note we do have things like $select->addExpression(':bundle', 'bundle', [':bundle' => $this->entityType->id()]); in core. So if parameter substitution in expressions doesn't work then SqlContentEntityStorageSchema is going to be v broken.

Beakerboy’s picture

It's good to see it tested in different layers, but a database dev will likely start with the database tests when learning the system before diving into entities. I would argue that the Entity system is a separate module, and is testing ITS functionality against the requirements, limitations, and obligations of the database layer. We shouldn't rely on Entities to prove that the database works. The database tests should do that. This is all speaking philosophically....I really don't care (much) how y'all implement it.

daffie’s picture

We shouldn't rely on Entities to prove that the database works.

Unfortionaly this is how it works in Drupal.

daffie’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC.

  • catch committed 084e627 on 9.0.x
    Issue #3108025 by Beakerboy, alexpott, daffie, bohart:  Function...

  • catch committed 5c15188 on 8.9.x
    Issue #3108025 by Beakerboy, alexpott, daffie, bohart:  Function...

  • catch committed 918b509 on 8.8.x
    Issue #3108025 by Beakerboy, alexpott, daffie, bohart:  Function...
catch’s picture

Version: 8.9.x-dev » 8.8.x-dev
Status: Reviewed & tested by the community » Fixed

Committed 084e627 and pushed to 9.0.x, 8.9.x, and 8.8.x. Thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.