In Drupal\Core\Database\Driver\sqlite\Statement::getStatement we have the following:

  protected function getStatement($query, &$args = []) {
    if (count($args)) {
      // Check if $args is a simple numeric array.
      if (range(0, count($args) - 1) === array_keys($args)) {

under PHP 7.2, if for any reason the $args is NULL or not an array, this will lead to notices and/or test failures, like

1) Drupal\KernelTests\Core\Database\DatabaseExceptionWrapperTest::testPreparedStatement
Thrown exception is not a PDOException:
count(): Parameter must be an array or an object that implements Countable
/home/travis/drupal8/vendor/symfony/phpunit-bridge/DeprecationErrorHandler.php:262
/home/travis/drupal8/core/lib/Drupal/Core/Database/Driver/sqlite/Statement.php:30
/home/travis/drupal8/core/lib/Drupal/Core/Database/StatementPrefetch.php:158
/home/travis/drupal8/core/lib/Drupal/Core/Database/Driver/sqlite/Statement.php:90
/home/travis/drupal8/core/lib/Drupal/Core/Database/Connection.php:612
/home/travis/drupal8/core/tests/Drupal/KernelTests/Core/Database/DatabaseExceptionWrapperTest.php:28
/home/travis/drupal8/core/tests/Drupal/KernelTests/Core/Database/DatabaseExceptionWrapperTest.php:39

Note: There is no SQLite testing on DrupalCI with PHP 7.2, yet - it's likely that this will appear once there'll be. The error above is result from a TravisCI test.

CommentFileSizeAuthor
#7 2932777-7.patch1.26 KBborisson_
#2 2932777-2.patch752 bytesmondrake
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mondrake created an issue. See original summary.

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake as a volunteer commented
Status: Active » Needs review
FileSize
2932777-2.patch752 bytes

I think it's just safer to replace the count() with a more appropriate check that $args is a non-empty array.

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake as a volunteer commented
Priority: Normal » Major
Issue summary: View changes
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented

Why would args not be countable? Ie when is this not going to be an array? And is the simpler thing just to type hint to an array?

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ as a volunteer commented

I think typehinting to an array makes more sense.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ as a volunteer commented
FileSize
2932777-7.patch1.26 KB

The array typehint is needed, because further down in the code we're doing array operations, so typehinting to \Countable is not sufficient. Implemented that.

No interdiff as this is another approach.

daffie’s picture

daffie CreditAttribution: daffie commented
Status: Needs review » Reviewed & tested by the community

The problem with adding a type hint to the parameter is that if goes wrong a type error exception will be thrown.
At the moment only PDOException or DatabaseExceptionWrapper exceptions are allowed.
Any other exception is considered wrong. See the patch from comment #6 with the SQLite testbot result.
The correct patch for me is the one from comment #2. That one is for me RTBC.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Status: Reviewed & tested by the community » Needs work

I think we can do something way better here :) patch incoming.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Status: Needs work » Reviewed & tested by the community

I thought we might be able to get rid of the SQLite statement fudge but we can't - it seems https://bugs.php.net/bug.php?id=45259 is still a bug. :(

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed 68cd2e5528 to 8.6.x and 93b2f9e806 to 8.5.x. Thanks!

Just to make it clear #2 was the patch committed.

diff --git a/core/lib/Drupal/Core/Database/StatementInterface.php b/core/lib/Drupal/Core/Database/StatementInterface.php
index a97043af62..4f4248df04 100644
--- a/core/lib/Drupal/Core/Database/StatementInterface.php
+++ b/core/lib/Drupal/Core/Database/StatementInterface.php
@@ -44,7 +44,7 @@
    *
    * @param $args
    *   An array of values with as many elements as there are bound parameters in
-   *   the SQL statement being executed.
+   *   the SQL statement being executed. This can be NULL.
    * @param $options
    *   An array of options for this query.
    *
diff --git a/core/lib/Drupal/Core/Database/StatementPrefetch.php b/core/lib/Drupal/Core/Database/StatementPrefetch.php
index 4e940ea372..de60d73bf9 100644
--- a/core/lib/Drupal/Core/Database/StatementPrefetch.php
+++ b/core/lib/Drupal/Core/Database/StatementPrefetch.php
@@ -214,8 +214,8 @@ protected function throwPDOException() {
    *
    * @param $query
    *   The query.
-   * @param array $args
-   *   An array of arguments.
+   * @param array|null $args
+   *   An array of arguments. This can be NULL.
    * @return \PDOStatement
    *   A PDOStatement object.
    */

Added some docs to the base class and interface so other implementations can know - and also any other efforts to typehint.

  • alexpott committed 68cd2e5 on 8.6.x 
    Issue #2932777 by mondrake, borisson_, alexpott, daffie: Risky count()...

  • alexpott committed 93b2f9e on 8.5.x 
    Issue #2932777 by mondrake, borisson_, alexpott, daffie: Risky count()...

Status: Fixed » Closed (fixed)

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