Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix datetime field with empty default #166

Merged
merged 9 commits into from
Dec 19, 2024
Merged

Fix datetime field with empty default #166

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
3c5455a
v2.1.14 (#155)
aristath Aug 14, 2024
a2661f9
Merge branch 'develop'
aristath Sep 4, 2024
e43782e
Add test that reproduces issue with invalid empty date default
wojtekn Dec 11, 2024
056bb18
Remove trailing space
wojtekn Dec 11, 2024
674bb6f
Add more cases to the test
wojtekn Dec 11, 2024
7835a5e
Ensure that for empty default date fields there is no default
wojtekn Dec 11, 2024
7f43961
Fix lint issues
wojtekn Dec 11, 2024
e81a812
Fix one more lint issue
wojtekn Dec 11, 2024
86b43fd
Fxi support for valid date field default values
wojtekn Dec 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 41 additions & 0 deletions tests/WP_SQLite_Translator_Tests.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -290,6 +290,47 @@ public function testShowCreateTable1() {
);
}

public function testShowCreateTableWithEmptyDatetimeDefault() {
$this->assertQuery(
"CREATE TABLE _tmp_table (
ID BIGINT PRIMARY KEY AUTO_INCREMENT,
timestamp1 datetime NOT NULL,
timestamp2 date NOT NULL,
timestamp3 time NOT NULL,
timestamp4 timestamp NOT NULL,
timestamp5 year NOT NULL,
notempty1 datetime DEFAULT '1999-12-12 12:12:12',
notempty2 date DEFAULT '1999-12-12',
notempty3 time DEFAULT '12:12:12',
notempty4 year DEFAULT '2024',
notempty5 timestamp DEFAULT '1734539165',
);"
);

$this->assertQuery(
'SHOW CREATE TABLE _tmp_table;'
);
$results = $this->engine->get_query_results();

$this->assertEquals(
"CREATE TABLE `_tmp_table` (
`ID` bigint AUTO_INCREMENT,
`timestamp1` datetime NOT NULL,
`timestamp2` date NOT NULL,
`timestamp3` time NOT NULL,
`timestamp4` timestamp NOT NULL,
`timestamp5` year NOT NULL,
`notempty1` datetime DEFAULT '1999-12-12 12:12:12',
`notempty2` date DEFAULT '1999-12-12',
`notempty3` time DEFAULT '12:12:12',
`notempty4` year DEFAULT '2024',
`notempty5` timestamp DEFAULT '1734539165',
PRIMARY KEY (`ID`)
);",
$results[0]->{'Create Table'}
);
}

public function testShowCreateTableQuoted() {
$this->assertQuery(
"CREATE TABLE _tmp_table (
Expand Down
32 changes: 30 additions & 2 deletions wp-includes/sqlite/class-wp-sqlite-translator.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3693,16 +3693,17 @@ protected function get_column_definitions( $table_name, $columns ) {
$auto_increment_column = $this->get_autoincrement_column( $table_name );
$column_definitions = array();
foreach ( $columns as $column ) {
$mysql_type = $this->get_cached_mysql_data_type( $table_name, $column->name );
$is_auto_incr = $auto_increment_column && strtolower( $auto_increment_column ) === strtolower( $column->name );
$definition = array();
$definition[] = '`' . $column->name . '`';
$definition[] = $this->get_cached_mysql_data_type( $table_name, $column->name ) ?? $column->name;
$definition[] = $mysql_type ?? $column->name;

if ( '1' === $column->notnull ) {
$definition[] = 'NOT NULL';
}

if ( null !== $column->dflt_value && '' !== $column->dflt_value && ! $is_auto_incr ) {
if ( $this->column_has_default( $column, $mysql_type ) && ! $is_auto_incr ) {
$definition[] = 'DEFAULT ' . $column->dflt_value;
}

Expand Down Expand Up @@ -3858,6 +3859,33 @@ function ( $row ) use ( $name_map ) {
);
}

/**
* Checks if column should define the default.
*
* @param stdClass $column The table column
* @param string $mysql_type The MySQL data type
*
* @return boolean If column should have a default definition.
*/
private function column_has_default( $column, $mysql_type ) {
if ( null === $column->dflt_value ) {
return false;
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wojtekn This seems to remove all defaults for date/time fields, including valid defaults, such as col YEAR DEFAULT '2024'. Could we add a test case with a valid default for each of these data types, and ensure that it is preserved?

My guess is that the problem could be that the original condition, despite looking fine ('' !== $column->dflt_value), might be missing the fact that the default can include a quoted string, so it should be rather something like '' !== trim( $column->dflt_value, "'" ).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking further, what's tricky, though, is that the '' value as a default is valid for string fields. So I guess we need to trim the quotes only for the case of non-string or date/time fields?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. I untangled the conditions to handle all cases and make it more readable in 86b43fd


if ( '' === $column->dflt_value ) {
return false;
}

if (
in_array( strtolower( $mysql_type ), array( 'datetime', 'date', 'time', 'timestamp', 'year' ), true ) &&
"''" === $column->dflt_value
) {
return false;
}

return true;
}

/**
* Consumes data types from the query.
*
Expand Down
Loading