Closed (fixed)
Project:
Drupal core
Version:
9.3.x-dev
Component:
sqlite db driver
Priority:
Critical
Category:
Bug report
Assigned:
Unassigned
Issue tags:
Reporter:
Created:
13 Sep 2021 at 15:36 UTC
Updated:
7 Oct 2021 at 10:24 UTC
Jump to comment: Most recent, Most recent file
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 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 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!