Split out from #2168241-9: Type hints for optional methods in StatementInterface (D8) / DatabaseStatementInterface (D7), here are some bug fixes for dbtng, without any noise from docblock improvements.

  • Drupal\Core\Database\Driver\pgsql\Connection::createDatabase():
    Replace $this->exec() with $this->connection->exec().
  • Drupal\Core\Database\Driver\sqlite\Connection::sqlFunctionGreatest():
    unset($args[$i]), instead of unset($args).
  • Drupal\Core\Database\Driver\sqlite\Connection::createDatabase():
    SplFileInfo needs to be prefixed with '\' for global namespace.
  • Drupal\Core\Database\StatementPrefetch::current():
    Replace array_unshift() with array_shift().
CommentFileSizeAuthor
#10 D8-2259947-10-dbtng-bugs.patch1.54 KBdonquixote
#7 D8-2259947-7-dbtng-bugs.patch3.12 KBdonquixote
#1 D8-2259947-1-dbtng-bugs.patch2.55 KBdonquixote

Comments

donquixote’s picture

donquixote commented
Status: Active » Needs review
StatusFileSize
new D8-2259947-1-dbtng-bugs.patch2.55 KB
donquixote’s picture

donquixote commented
Issue summary: View changes
donquixote’s picture

donquixote commented
Issue summary: View changes
Crell’s picture

Crell commented

This needs manual testing on SQLite and Postgres before it can be RTBCed.

dcrocks’s picture

dcrocks commented

Tried against Sqlite on current D8 clone. Install seems to be ok, but ran into unrelated issue so haven't tested further.

damien tournoud’s picture

damien tournoud commented

Drupal\Core\Database\Driver\sqlite\Connection::sqlFunctionGreatest():
unset($args[$i]), instead of unset($args).

-1. GREATEST needs to return NULL if any argument is NULL.

--- a/core/lib/Drupal/Core/Database/StatementPrefetch.php
+++ b/core/lib/Drupal/Core/Database/StatementPrefetch.php
@@ -298,7 +298,7 @@ public function current() {
         case \PDO::FETCH_OBJ:
           return (object) $this->currentRow;
         case \PDO::FETCH_CLASS | \PDO::FETCH_CLASSTYPE:
-          $class_name = array_unshift($this->currentRow);
+          $class_name = array_shift($this->currentRow);
           // Deliberate no break.
         case \PDO::FETCH_CLASS:
           if (!isset($class_name)) {

This is not actually supported, and I doubt it works. We can support it cleanly now that we have PHP 5.4, but that requires more changes than just that. We have an existing issue for that somewhere.

donquixote’s picture

donquixote commented
StatusFileSize
new D8-2259947-7-dbtng-bugs.patch3.12 KB

To give some background: These were issues I ran into while fixing docblock issues and generally everything that makes my IDE complain.
The existing code in these places looked obviously wrong, but yeah, probably the fix is not always as easy as thought.

The new patch still contains all 4 attempted fixes, but I am ok to move some of this to separate issues.

@Damien (#6) / GREATEST(*, NULL)

GREATEST needs to return NULL if any argument is NULL.

Good to know, I was not aware. I tried to find a resource to confirm the fact, and this is the closest I got: http://bugs.mysql.com/bug.php?id=15610, but yes it confirms you are right.

Either way, I think the existing code is confusing, and it causes a count(UNDEFINED) which the IDE correctly complains about, and is likely to give us a PHP notice (I did not actually verify that).
It might incidentally do what we want, but it looks to me as if the original authors had something else in mind.

I'd say we should simply return NULL directly from the foreach() loop, and add a comment to explain why we do this. And while we are at it, clean up the rest of the method. See the new patch.

Another question is how reliable PHP's max() actually is in replicating SQL GREATEST() for non-integer values.
Open a separate issue?

@Damien (#6) / StatementPrefetch::current()

This is not actually supported, and I doubt it works.

The problem with the existing code is simply that array_unshift() requires a second parameter, and it does not return anything.
The easiest explanation is that the author originally meant to do array_shift() instead.

I am ok to deal with this in another issue if someone can find it.

But what do you mean by "not actually supported"? The array_shift()? Or something in the switch/case?

EDIT: Seems that yoda is not very popular around here (to my regret).
Going to fix this in the next round.

damien tournoud’s picture

damien tournoud commented

Another question is how reliable PHP's max() actually is in replicating SQL GREATEST() for non-integer values.
Open a separate issue?

This is a complex subject. We never could implement collations properly in SQLite because the necessary hooks were not available in PHP. I implemented them, and support is now available in PHP 5.4.

Let's open a separate issue for that, the rest of your fix is fine.

The problem with the existing code is simply that array_unshift() requires a second parameter, and it does not return anything. The easiest explanation is that the author originally meant to do array_shift() instead.

But what do you mean by "not actually supported"? The array_shift()? Or something in the switch/case?

@donquixote: Sorry I was cryptic. I probably wrote this, and you are right the intent was obviously array_shift(). But the whole PDO::FETCH_CLASS / PDO::FETCH_CLASSTYPE never worked anyway, because to implement it properly we need to create an instance of a class without calling its constructor, which was impossible in userland PHP before 5.4.

Because the rest of the database layer doesn't really support this option either as far as I remember, we never bothered.

Now it is possible to support and test this properly. But let's do that in a separate issue.

donquixote’s picture

donquixote commented

I implemented them

'spect!

This is a complex subject.

I guess now that the hook is there, the remaining task of correctly replicating SQL's GREATEST() using max() or other PHP constructs is not that hard anymore. This needs some testing and evaluation, and we would need to check whether the behavior of GREATEST() is even consistent between MySQL, Oracle etc. This can be done as a follow-up if someone has too much time.

I'd say, we add a @todo and then commit the fix as-is. This might not fix the complete problem, but it is a clear improvement and it introduces no regression. Whatever was wrong with max() vs GREATEST(), it won't be any more or less wrong after the patch.

I probably wrote this, and you are right the intent was obviously array_shift(). But the whole PDO::FETCH_CLASS / PDO::FETCH_CLASSTYPE never worked anyway

Again, what about we apply this simple "fix" and add a @todo? This way the IDE will be happy (*), and the harder problem can be dealt with in a follow-up.

(*) IDE will be really happy once we finish all the other cleanup issues for dbtng.

But the whole PDO::FETCH_CLASS / PDO::FETCH_CLASSTYPE never worked anyway, because to implement it properly we need to create an instance of a class without calling its constructor

Or only use it with classes that don't have a constructor? Either way, let's do this in a follow-up.

donquixote’s picture

donquixote commented
Related issues: +#1814762: SQLite Greatest implementation is wrong for parameters with the value NULL, +#2262425: Does PHP max() correctly replicate SQL GREATEST() for non-numeric values?, +#1476782: DatabaseStatementPrefetch::current PHP function array_unshift() are used incorrectly
StatusFileSize
new D8-2259947-10-dbtng-bugs.patch1.54 KB

Follow-ups:
#2262425: Does PHP max() correctly replicate SQL GREATEST() for non-numeric values?

Existing (should have looked before):
#1814762: SQLite Greatest implementation is wrong for parameters with the value NULL
#1476782: DatabaseStatementPrefetch::current PHP function array_unshift() are used incorrectly

Looks like we should rather not touch these two problems and let the other issues handle it.

damien tournoud’s picture

damien tournoud commented
Status: Needs review » Reviewed & tested by the community
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed d8cdb35 and pushed to 8.x. Thanks!

  • Commit d8cdb35 on 8.x by alexpott: 
    Issue #2259947 by donquixote: Fixed Minor bug fixes in database system.

Status: Fixed » Closed (fixed)

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