Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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
Comment | File | Size | Author |
---|---|---|---|
#19 | 3232699-19.patch | 4.96 KB | alexpott |
#19 | 15-19-interdiff.txt | 781 bytes | alexpott |
#15 | 3232699-15.patch | 4.89 KB | alexpott |
#15 | 9-15-interdiff.txt | 724 bytes | alexpott |
Comments
Comment #2
alexpottHere's the relevant part of patch from the parent issue.
Not sure how and if we should test this.
Comment #3
alexpottHere'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.
Comment #4
alexpottTurns out that this piece of code has more than a few bugs :)
Comment #7
alexpottInteresting... we've found an inconsistency with mysql...
I guess we can open a separate issue about that...
Comment #8
longwaveMySQL 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
Comment #9
alexpott@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...
Comment #10
mondrakeFrom #7...
MySql seems to use the DEFAULT keyword twice, but no complains?
Comment #11
alexpottFiled the issue for MySQL... #3232804: Support MySQL default values for blob, text and json field types
Comment #12
longwave#10 another MySQL weirdness I guess, it apparently allows you to specify it any number of times and always takes the last specified value:
Comment #13
daffie CreditAttribution: daffie commentedWhy are we removing the column length info? Can we put it back.
We can add a length check for a column in SQLite.
The code is from https://stackoverflow.com/questions/12056571/limit-number-of-characters-....
Comment #14
andypostComment #15
alexpott@daffie nice spot. Didn't mean to remove that... seems like we don't have test coverage.
Comment #16
daffie CreditAttribution: daffie commentedAll the code changes look good to me.
The bug has a fix and there is testing for the fix.
For me it is RTBC.
Comment #17
andypostI bet it needs follow-up for #15 but RTBC++ the bug
Comment #18
catchThis is tricky, is there something we can link to?
Or if not can we add a concrete example like:
Comment #19
alexpottI 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.
Comment #21
catchCommitted 8ce891e and pushed to 9.3.x. Thanks!