Fix datetime field with empty default #166
Conversation
Co-authored-by: Jeroen P <[email protected]> Co-authored-by: Rostislav Wolný <[email protected]> Co-authored-by: Brandon Payton <[email protected]> Co-authored-by: Alex Kirk <[email protected]> Co-authored-by: Jan Jakeš <[email protected]> Co-authored-by: Bero <[email protected]> Co-authored-by: Antony Agrios <[email protected]>
wojtekn
force-pushed
the
fix/datetime-field-with-empty-default
branch
from
a18ec4e to
7835a5e
Compare
|
I initially had a question but I figured it so I removed it (in case you got a notification 😅 ). The fix looks good to me. I looked through The MySQL data types in the backups from Studio and Jetpack and the fix seems to cover the cases that we ran into. 👍 Out of curiosity, I see that the fix is covered with unit tests so there are no manual testing steps needed. If I were to test this manually with Studio, do you know what steps I would need to take? 🙇 |
The easiest way would be:
|
|
Nice thanks! |
| private function column_has_default( $column, $mysql_type ) { | ||
| if ( null !== $column->dflt_value && '' !== $column->dflt_value && ! in_array( strtolower( $mysql_type ), array( 'datetime', 'date', 'time', 'timestamp', 'year' ), true ) ) { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
@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, "'" ).
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Good catch. I untangled the conditions to handle all cases and make it more readable in 86b43fd
Fixes #165
I propose to fix an issue where dumping the datetime field that has empty value results with producing incorrect MySQL.