Problem/Motivation

The code 'default' => trim($row->dflt_value, "'"), implicitly casts NULLs to strings and therefore changes NULLs to empty strings which are not same when it comes to default database values. The current code causes deprecation notices on PHP 8.1 and suspect this is revealing a bug.

Steps to reproduce

Proposed resolution

Respect the NULL value and don't cast it to a string.

Remaining tasks

User interface changes

None

API changes

None

Data model changes

Better representation of reality in \Drupal\Core\Database\Driver\sqlite\Schema::introspectSchema()

Release notes snippet

CommentFileSizeAuthor
#19 3232699-19.patch4.96 KBalexpott
#19 15-19-interdiff.txt781 bytesalexpott
#15 3232699-15.patch4.89 KBalexpott
#15 9-15-interdiff.txt724 bytesalexpott
#9 3232699-9.patch4.83 KBalexpott
#9 3232699-9.test-only.patch3.21 KBalexpott
#9 4-9-interdiff.txt2.87 KBalexpott
#4 3232699-4.patch4.7 KBalexpott
#4 3232699-4.test-only.patch3.08 KBalexpott
#4 3-4-interdiff.txt4.88 KBalexpott
#3 3232699-3.patch2.69 KBalexpott
#3 2-3-interdiff.txt2.99 KBalexpott
#3 3232699-3.test-only.patch1.88 KBalexpott
#2 3232699-2.patch987 bytesalexpott
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott created an issue. See original summary.

alexpott’s picture

Issue summary: View changes
Priority: Normal » Critical
Status: Active » Needs review
Issue tags: +PHP 8.1
Parent issue: » #3220021: [meta] Ensure compatibility of Drupal 9 with PHP 8.1 (as it evolves)
FileSize
987 bytes

Here's the relevant part of patch from the parent issue.

Not sure how and if we should test this.

alexpott’s picture

Here's a test that fails on SQLite and passes on MySQL... fun times. I've change around the logic to only trim the value when it is a string.

The last submitted patch, 4: 3232699-4.test-only.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 4: 3232699-4.patch, failed testing. View results

alexpott’s picture

Interesting... we've found an inconsistency with mysql...

Drupal\Core\Database\DatabaseExceptionWrapper: SQLSTATE[42000]: Syntax error or access violation: 1101 BLOB, TEXT, GEOMETRY or JSON column 'column5' can't have a default value: CREATE TABLE "test79289586test_table" (
`column1` INT DEFAULT NULL DEFAULT NULL, 
`column2` VARCHAR(20) DEFAULT NULL DEFAULT NULL, 
`column3` INT DEFAULT 200, 
`column4` FLOAT DEFAULT 1.23, 
`column5` TEXT DEFAULT '\'s o\'clock\'', 
`column6` TEXT DEFAULT 'o\'clock', 
`column7` TEXT DEFAULT 'default value'
) ENGINE = InnoDB DEFAULT CHARACTER SET utf8mb4 COMMENT 'Test table.'; Array
(
)

I guess we can open a separate issue about that...

longwave’s picture

MySQL changed its handling of default values to allow blob, text or JSON in 8.0.13: https://dev.mysql.com/doc/refman/8.0/en/data-type-defaults.html

alexpott’s picture

@longwave #8 interesting! I think we still need to open an issue because even literal defaults for blob and therefore I think text need wrapping in brackets which we don't do atm. But this is all by the by for this issue. Here's we're concerned with sqlite and the bugs in \Drupal\Core\Database\Driver\sqlite\Schema::introspectSchema(). Patch attached fixes the test so it still fails without the fix on sqlite and now should pass on all db drivers...

mondrake’s picture

From #7...

`column1` INT DEFAULT NULL DEFAULT NULL, 
`column2` VARCHAR(20) DEFAULT NULL DEFAULT NULL, 

MySql seems to use the DEFAULT keyword twice, but no complains?

alexpott’s picture

longwave’s picture

#10 another MySQL weirdness I guess, it apparently allows you to specify it any number of times and always takes the last specified value:

MariaDB [db]> create table table1 (column1 int default null default null default null default 0 default null default 0);
Query OK, 0 rows affected (0.012 sec)

MariaDB [db]> describe table1;
+---------+---------+------+-----+---------+-------+
| Field   | Type    | Null | Key | Default | Extra |
+---------+---------+------+-----+---------+-------+
| column1 | int(11) | YES  |     | 0       |       |
+---------+---------+------+-----+---------+-------+
1 row in set (0.001 sec)
daffie’s picture

+++ b/core/lib/Drupal/Core/Database/Driver/sqlite/Schema.php
@@ -505,10 +505,23 @@ protected function introspectSchema($table) {
-        if ($length) {
-          $schema['fields'][$row->name]['length'] = $length;

Why are we removing the column length info? Can we put it back.

We can add a length check for a column in SQLite.

CREATE TABLE IF NOT EXISTS "test"
(
    "id" INTEGER PRIMARY KEY AUTOINCREMENT NOT NULL,
    "name" TEXT NOT NULL
    CHECK(
        typeof("name") = "text" AND
        length("name") <= 20
    )
);

The code is from https://stackoverflow.com/questions/12056571/limit-number-of-characters-....

andypost’s picture

alexpott’s picture

@daffie nice spot. Didn't mean to remove that... seems like we don't have test coverage.

daffie’s picture

All the code changes look good to me.
The bug has a fix and there is testing for the fix.
For me it is RTBC.

andypost’s picture

I bet it needs follow-up for #15 but RTBC++ the bug

catch’s picture

+++ b/core/lib/Drupal/Core/Database/Driver/sqlite/Schema.php
@@ -505,11 +505,28 @@ protected function introspectSchema($table) {
+        }
+        elseif (is_numeric($row->dflt_value)) {
+          // Adding 0 to a string will cause PHP to convert it to a float or
+          // an integer depending on what the string is.
+          $schema['fields'][$row->name]['default'] = $row->dflt_value + 0;
+        }

This is tricky, is there something we can link to?

Or if not can we add a concrete example like:

'1' + 0 = 1

'1.0' + 0 = 1.0
alexpott’s picture

I like the idea of adding examples to the code. There is test coverage of this and how it works so if PHP does change the behaviour a test will fail.

  • catch committed 8ce891e on 9.3.x
    Issue #3232699 by alexpott, daffie, andypost, longwave: SQLite schema...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed 8ce891e and pushed to 9.3.x. Thanks!

Status: Fixed » Closed (fixed)

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