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.

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

[6.x] Wrap MySQL default values in parentheses #29878

Merged
merged 1 commit into from Sep 6, 2019

Conversation

@browner12
Copy link
Contributor

commented Sep 5, 2019

Prior to MySQL 8.0.13, types of Blob, Text, Geometry, and JSON could not be assigned default values.

This made JSON fields cast as Collections annoying to handle when the value was null.

As of MySQL 8.0.13, we can now add default values to these fields, but a minor change needs to be made in the grammar. These fields can be assigned a default value only if the value is written as an expression, even if the expression value is a literal.

This will not work:

CREATE TABLE t2 (b BLOB DEFAULT 'abc');

but this will:

CREATE TABLE t2 (b BLOB DEFAULT ('abc'));

Originally I was going to conditionally check for the field type and only apply the parentheses for the required types, but that seemed like overkill. Only if there were performance or other implications for making every default value an expression would I think we would need to do that.

I won't presume to know how the other DB engines handle this, but there's a decent chance these changes will need to be made to them as well.

On a side note, now that we've switched to SemVer, do we need to indicate in our PRs if we think they are minor or patch commits?

wrap MySQL default values in parentheses
wrap default values in parentheses and update tests

@GrahamCampbell GrahamCampbell changed the title [6] wrap MySQL default values in parentheses [6.x] wrap MySQL default values in parentheses Sep 5, 2019

@GrahamCampbell

This comment has been minimized.

Copy link
Member

commented Sep 5, 2019

On a side note, now that we've switched to SemVer, do we need to indicate in our PRs if we think they are minor or patch commits?

No, I think the plan is just, on release day, it will be decided if patch or minor version is bumped. Usually, minor version I guess.

@GrahamCampbell

This comment has been minimized.

Copy link
Member

commented Sep 5, 2019

Next release will be v6.1.0.

@browner12

This comment has been minimized.

Copy link
Contributor Author

commented Sep 5, 2019

and patch releases will just happen Ad Hoc outside of normal release days?

@GrahamCampbell

This comment has been minimized.

Copy link
Member

commented Sep 5, 2019

I don't think so?

@GrahamCampbell

This comment has been minimized.

Copy link
Member

commented Sep 5, 2019

Patch releases will only happen if there's a critical bug to be fixed, or there happened to be no new features warranting a minor bump on release day.

@browner12

This comment has been minimized.

Copy link
Contributor Author

commented Sep 6, 2019

Patch releases will only happen if there's a critical bug to be fixed,

Yup, that's what I meant. 👍

@driesvints

This comment has been minimized.

Copy link
Member

commented Sep 6, 2019

@browner12 @GrahamCampbell we've decided to evaluate every release if there's going to be a patch or minor release to properly handle SemVer.

@GrahamCampbell

This comment has been minimized.

Copy link
Member

commented Sep 6, 2019

we've decided to evaluate every release if there's going to be a patch or minor release to properly handle SemVer.

Right, so as I thought. :P

No, I think the plan is just, on release day, it will be decided if patch or minor version is bumped. Usually, minor version I guess.

@taylorotwell taylorotwell merged commit 2f20d88 into laravel:6.x Sep 6, 2019

2 checks passed

continuous-integration/styleci/pr The analysis has passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@browner12 browner12 deleted the browner12:json-default branch Sep 6, 2019

@GrahamCampbell GrahamCampbell changed the title [6.x] wrap MySQL default values in parentheses [6.x] Wrap MySQL default values in parentheses Sep 6, 2019

@JeffreyWay

This comment has been minimized.

Copy link
Contributor

commented Sep 10, 2019

Heads up that this broke one of my migrations after updating from 6.0.1 to 6.0.2.

I had an old migration class with:

$table->integer('count')->unsigned()->default(0);

This broke because of the PR. I received the error:

Syntax error or access violation: 1064 You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near '('0')' at line 1 (SQL: alter table lessons add count int unsigned not null default ('0'))

@browner12

This comment has been minimized.

Copy link
Contributor Author

commented Sep 10, 2019

thanks for the insight Jeffrey.

what version of MySQL are you on?

maybe we'll have to be selective about what types of columns we apply the parentheses to.

@browner12

This comment has been minimized.

Copy link
Contributor Author

commented Sep 10, 2019

FYI, I've just run this exact query (alter table lessons add count int unsigned not null default ('0')) on my MySQL v8.0.17 database and it works.

@browner12

This comment has been minimized.

Copy link
Contributor Author

commented Sep 10, 2019

so the statement executed successfully, but apparently it set the default value to (_utf8mb4'0')

@dillingham

This comment has been minimized.

Copy link

commented Sep 10, 2019

experienced this on mysql 5.7.24

@browner12

This comment has been minimized.

Copy link
Contributor Author

commented Sep 10, 2019

@taylorotwell I'm thinking we should revert this for now, because it appears MySQL prior to 8.0.13 cannot handle "expressions" as their default value.

https://dev.mysql.com/doc/refman/8.0/en/data-type-defaults.html#data-types-defaults-explicit-old

With one exception, the default value specified in a DEFAULT clause must be a literal constant; it cannot be a function or an expression.

What I am going to propose is a new method on the schema grammar called ->defaultExpression() which will use the new grammar, and ->default() will retain the previous v5 compatible grammar.

@mark1282

This comment has been minimized.

Copy link

commented Sep 10, 2019

Heads up that this broke one of my migrations after updating from 6.0.1 to 6.0.2.

I had an old migration class with:

$table->integer('count')->unsigned()->default(0);

This broke because of the PR. I received the error:

Syntax error or access violation: 1064 You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near '('0')' at line 1 (SQL: alter table lessons add count int unsigned not null default ('0'))

Also experiencing this issue with defaults on integers. Just changed the MySqlGrammer back to ' return ' default '.$this->getDefaultValue($column->default);' as a temp fix.

driesvints added a commit that referenced this pull request Sep 10, 2019
taylorotwell added a commit that referenced this pull request Sep 10, 2019
browner12 added a commit to browner12/framework that referenced this pull request Sep 10, 2019
@lachezargrigorov

This comment has been minimized.

Copy link

commented Sep 10, 2019

An extra parameter could also do the job.

function default($value, $expression = false);
@browner12

This comment has been minimized.

Copy link
Contributor Author

commented Sep 11, 2019

Laravel is usually very anti true/false parameter flags, because they are not very expressive.

After digging into the source, I found out it's actually currently possible to add expressions as a default value.

I've sent a PR into the Docs to help explain how to use it:

laravel/docs#5466

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.