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

Add native JSON type support for MySQL >=5.7.8 #2266

Closed
wants to merge 5 commits into from
Closed

Add native JSON type support for MySQL >=5.7.8 #2266

wants to merge 5 commits into from

Conversation

ismailbaskin
Copy link

As of MySQL 5.7.8, MySQL supports a native JSON data type that enables efficient access to data in JSON (JavaScript Object Notation) documents. The JSON data type provides these advantages over storing JSON-format strings in a string column:

http://dev.mysql.com/doc/refman/5.7/en/json.html

*
* @author İsmail BASKIN <ismailbaskin1@gmail.com>
* @link www.doctrine-project.org
* @since 2.5
Copy link
Member

Choose a reason for hiding this comment

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

@since 2.6

Copy link
Author

Choose a reason for hiding this comment

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

@deeky666 deeky666 added this to the 2.6 milestone Jan 5, 2016
@deeky666 deeky666 self-assigned this Jan 5, 2016
@deeky666 deeky666 changed the title Support for MySQL new native JSON type which has been introduced 5.7.8 [MySQL] Add platform for version >=5.7.8 with native JSON type support Jan 22, 2016
deeky666 added a commit to deeky666/dbal that referenced this pull request Jan 22, 2016
[MySQL] Add platform for version >=5.7.8 with native JSON type support
@ghost
Copy link

ghost commented Feb 26, 2016

Any progress on this ?

@sbusch
Copy link

sbusch commented Apr 2, 2016

We need this for barryvdh/laravel-ide-helper#295

Is there anything blocking this merge? If yes I could try to help out.

@TNAJanssen
Copy link

+1

1 similar comment
@danielkeil
Copy link

+1

@Ocramius
Copy link
Member

Ocramius commented Apr 8, 2016

+1-ing this doesn't change things, folks. If @deeky666 has no time, that's how it is.

@Ocramius
Copy link
Member

Ocramius commented Apr 8, 2016

It just adds to an already infinite inbox. Github gave you like buttons exactly to avoid this: use them, please.

@Malki
Copy link

Malki commented Apr 28, 2016

Not sure but I think that registering json type is still required to avoid 'Unknown database type json requested..' exception in some cases like generating migration files.

@robhogan
Copy link

robhogan commented Oct 18, 2016

A maintainer needs to make a decision here since there's nothing more the community can do to move this along. The so-called BC break is unavoidable if we're ever to use MySQL's native JSON support for Doctrine's json type (since it's inherently more strict) so this change is either fundamentally acceptable (perhaps with a version bump and/or BC note) or both PRs should be closed and we should give up on ever using native JSON as the json default. Do correct me if I've missed something.

Personally I agree with @teohhanhui and @dunglas - no one should be surprised that mapping arbitrary text to a json column is a bad idea even if it's currently permitted, it's surely not a supported use case. The docs for both json and (deprecated) json_array already state

Some vendors have a native JSON type and Doctrine will use it if possible

and

If you know that the data to be stored always is in a valid UTF-8 encoded JSON format string, you should consider using this type

Which clearly imply 1) that you'd be wrong to rely on the type for non-JSON values (empty strings included) and 2) that we positively should be mapping to the native type for >=5.7.

IMO it's not worth blocking the project from evolving for the sake of a very weird and marginal (ab)use-case. @beberlei is the only one to mention that this "can be considered a BC break" and even he didn't sound so sure! Does anyone think it's a blocking problem?

Edit: I misunderstood the empty string problem - I see now that that's the default field value and supported by json and json_array so needs to be dealt with. Looking into it...

Edit 2: Interestingly, the schema tool doesn't pick up any change at all because Comparator::diffColumn() doesn't compare SQL types. So it looks like there's no BC because there's no C 😕 - might need fixing!

@robhogan
Copy link

Urgh - this is problematic and I think we'll need some input from @beberlei. Doctrine\DBAL\Schema\Column has no knowledge of the database column type so 1) the Doctrine\DBAL\Schema\Comparator fails to notice that column needs modifying at all and 2) MySqlPlatform::getAlterTableSQL can't do anything clever to convert empty strings to valid JSON, since it doesn't know that the original type is text.

I'd propose adding dbType to Column and inspecting that in the Comparator alongside other changed properties, but I think that would merit a separate PR and I'd appreciate a maintainer's opinion on whether that's the best approach.

@StephenOTT
Copy link

any feedback from @beberlei on this?

@fmonts
Copy link

fmonts commented Dec 22, 2016

+1. Any news?

@robhogan
Copy link

Nothing yet. I guess @beberlei isn't active on this project any more. Maybe @deeky666 or @Ocramius could have a look?

I think basically the outstanding issue is that we need to make changes to Column and Comparator to distinguish target SQL type from existing SQL type. Then we can use getAlterTableSql to deal with upgrading a text column which might contain empty strings to a JSON column that can't. That addresses the BC break (although I think the same BC break existed when postgres was given native JSON support and no one seemed to notice/care).

I'm happy to make a PR but it's a bit more work than this one and it'd be nice to think there's a chance of it being merged within six months!

@blackfyre
Copy link

blackfyre commented Dec 22, 2016

Well... this has been floating here more then a year now... it would be good to have some kind of resolution... Since the current MySQL version is 5.7.18, so slap a new version number on and go on...
Laravel even has BC in patch versions... not to mention minor versions...

@ghost
Copy link

ghost commented Dec 22, 2016

Symfony is ready too for these changes. I agree with @blackfyre, lets merge this pull request.

@StephenOTT
Copy link

So, is there someone working on merging this? Or are there issues to resolve?

@uniring
Copy link

uniring commented Feb 9, 2017

+1

@deeky666
Copy link
Member

deeky666 commented Feb 9, 2017

I will have a look into this as soon as I get time. There are currently some higher priority issues that have to be resolved first.

@deeky666
Copy link
Member

deeky666 commented Feb 9, 2017

Aye, a lot of input here :D. First decision of the day: Closing this in favour of #2455 because as said before, this is part of the first GA release. Please let's continue discussions there. Thank you all for your input an patience, we will get this baby merged soon!

@Ocramius
Copy link
Member

Ocramius commented May 9, 2017

Handled in #2653

@Ocramius Ocramius changed the title [MySQL] Add platform for version >=5.7.8 with native JSON type support Add native JSON type support for MySQL >=5.7.8 Jul 22, 2017
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.