Merge MySQL 5.7.9 (GA) semantics into MySQL57Platform #2653

deeky666 · 2017-02-09T22:42:20Z

This is adopted from #2266 and #2455 and contains additional fixes, adjustments and upgrade notice.

I made the following decisions about this whole topic:

Merge the changes into MySQL57Platform as suggested by @beberlei because the first GA version of the 5.7 series is 5.7.9
Accept possible minor BC breaks when upgrading the schema (tracked in UPGRADE.md)
Do not care about invalid values previously store in json_array type columns (they would be invalid with 2.5 anyways so what's the point?)
Did I forget anything? Please let me know.

phansys · 2017-02-09T22:45:06Z

lib/Doctrine/DBAL/Driver/AbstractMySQLDriver.php


-        if (version_compare($version, '5.7', '>=')) {
+        if ($majorVersion === '5' && $minorVersion === '7' && null === $patchVersion) {


Please, use Yoda condition.

phansys · 2017-02-09T22:46:46Z

tests/Doctrine/Tests/DBAL/Platforms/MySQL57PlatformTest.php

+
+    public function testReturnsJsonTypeDeclarationSQL()
+    {
+        $this->assertSame('JSON', $this->_platform->getJsonTypeDeclarationSQL(array()));


Support for PHP < 7 has been dropped at master. Could we use [] instead?

@phansys is that really important here? We would need to fix all of that stuff in one PR anyways.

robhogan · 2017-02-10T02:54:22Z

[Edit: Petulant complaint retracted with apologies]

Ocramius · 2017-02-10T06:25:44Z

[Edit: snarky response retracted, apologies accepted 👍 ]

deeky666 · 2017-02-10T08:53:42Z

@rh389 I did not intend to rip anyone's work off. Originally I wanted to use either of the previous PRs getting merged but both of them were lacking some things. #2266 need merge into MySQL57Platform, #2455 needed keywords, tests, documentation etc. And both of them did not have the upgrade entry. As many people like you want to get this finally going, I decided to take a "fast" approach by using what was already contributed (btw see the @author tag in MySQL57Platform) and put the missing pieces in. So much to ripping off.
As to why this took 14 months, as you might have noticed, I took a year off of OS development last year (more or less) and I apologize if that might have been frustrating to some, but we all do this in our free time and have personal lives beside Doctrine.

robhogan · 2017-02-10T10:20:28Z

Sorry @deeky666, I was out of order. It was frustrating to work on this issue without a response but that's OS sometimes, not your fault. Apologies again, thanks for you efforts and looking forward to seeing this land.

robhogan · 2017-02-10T13:44:00Z

UPGRADE.md

+MySQL 5.7 ships with native JSON type support which now is adopted by DBAL.
+If you are using MySQL 5.7 and have columns with ``json_array`` type defined in your database,
+utilizing DBAL's schema comparator will cause a schema diff, where the previously used native type
+``LONGTEXT`` will be changed to ``JSON``.


I'm not sure this is actually true - the Comparator doesn't inspect the actual column type so won't pick up the change (unless/until it has some other reason to alter that column). See #2266 (comment)

@rh389 good point. Considering that, I can remove the upgrade notice again I suppose unless there are other reasons why BC could not be maintained here. Currently I can't think of any.

Well, it will alter to a native JSON type later if the user changes their column definition some other way, which might be a bit unexpected (buggy?) but I guess technically not a BC break.

When the column is updated though it's actually quite likely to fail in practice because the LONGTEXT implementation used an empty string as a default value, which is invalid for native JSON. It's not just the case that it'll only fail if the user was doing something weird.

Not a blocker IMO - just making sure you're aware.

I see, there is nothing we can do about it if we want to move on. I'll remove the upgrade notice then.

deeky666 · 2017-03-10T11:51:36Z

@Ocramius @phansys @rh389 this thing is ready from my point of view. Anything we forgot?

phansys

Status: Needs work.

phansys · 2017-03-10T12:16:35Z

lib/Doctrine/DBAL/Platforms/Keywords/MySQL57Keywords.php

@@ -241,6 +242,7 @@ protected function getKeywords()
            'SSL',
            'STARTING',
            'STRAIGHT_JOIN',
+            'STORED',


This should be placed after 'STARTING' and before 'STRAIGHT_JOIN' in order to keep the current sorting.

phansys · 2017-03-10T12:18:40Z

tests/Doctrine/Tests/DBAL/Platforms/MySQL57PlatformTest.php

+    public function testInitializesJsonTypeMapping()
+    {
+        $this->assertTrue($this->_platform->hasDoctrineTypeMappingFor('json'));
+        $this->assertEquals('json_array', $this->_platform->getDoctrineTypeMapping('json'));


We should use assertSame() instead.

Omranic · 2017-04-12T05:13:21Z

@deeky666 @Ocramius @phansys @rh389 Is this PR landing anytime soon?! 🤔 Thanks in advance

deeky666 · 2017-04-12T13:05:26Z

@Omranic waiting for an approval and merge from one of the other core members.

mpyw · 2017-05-09T07:14:28Z

Pending for 27 days......

Ocramius · 2017-05-09T07:20:31Z

That is quite short

…

On 9 May 2017 9:14 a.m., "mpyw" ***@***.***> wrote: Pending for 27 days...... — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2653 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAJakMf7opT5a8Ss9FJeRQW4_f4lwLJgks5r4BJUgaJpZM4L8w1d> .

kimhemsoe · 2017-05-09T07:38:31Z

@deeky666 Looks good to me

mpyw · 2017-05-09T10:37:46Z

@kimhemsoe @deeky666 I'm also using the branch in my product and working fine 👍

Ocramius

LGTM 🚢

mpyw · 2017-05-09T10:57:26Z

@Ocramius Thank you ✨

le-martre · 2017-07-31T15:04:31Z

Hello guys, first of all thanks a lot for your work.
I have a small question though, I upgraded to PHP7.1 and dbal 2.6.1 especially to fix a bug that I'm currently experiencing. I'm just trying to create a json database field in one of my laravel migrations.

I still get this message though :

[Doctrine\DBAL\DBALException]                                                                 
  Unknown database type json requested, Doctrine\DBAL\Platforms\MySqlPlatform may not support   
  it.

Which makes me think that MySQL57Platform is never used or acknowledged? Any hints about where it could come from? Thanks in advance! Have a great week.

Ocramius · 2017-07-31T15:05:48Z

@MartinVandersteen please report a separate issue with a way to reproduce it.

le-martre · 2017-07-31T15:08:22Z

I don't think there is any way I could find a way to reproduce it, just asking if something has to be done at some point for Dbal to understand it has to use MySQL57 ? On what ground does it decide which one to use?

Ocramius · 2017-07-31T15:11:15Z

just asking if something has to be done at some point for Dbal to understand it has to use MySQL57

See

dbal/lib/Doctrine/DBAL/Connection.php

Line 391 in 399ab63

$this->platform = $this->_driver->createDatabasePlatformForVersion($version);

A test would:

connect to a DB
create the schema
try to introspect the schema and verify that the "json" type was correctly detected

That said, please report further findings on a new issue.

hansbogert · 2018-11-14T10:17:22Z

The Type matrix does not reflect this change. In fact it doesn't show the json type at all. (it does show the deprecated json_array)

https://www.doctrine-project.org/projects/doctrine-dbal/en/2.8/reference/types.html#mapping-matrix

deeky666 added Compatibility Improvement MySQL Platforms Type Mapping labels Feb 9, 2017

deeky666 added this to the 2.6 milestone Feb 9, 2017

deeky666 requested a review from Ocramius February 9, 2017 22:42

deeky666 mentioned this pull request Feb 9, 2017

Use native JSON type on MySQL >=5.7 #2455

Closed

phansys suggested changes Feb 9, 2017

View reviewed changes

deeky666 force-pushed the fix-mysql57-platform branch from 1412157 to 3efba67 Compare February 9, 2017 22:50

robhogan reviewed Feb 10, 2017

View reviewed changes

jjanvier mentioned this pull request Feb 24, 2017

TIP-693: introduce a native JSON field type for Doctrine jjanvier/akeneo-pim-community-dev#12

Merged

deeky666 force-pushed the fix-mysql57-platform branch from 3efba67 to 6b3ba2d Compare March 10, 2017 11:49

phansys suggested changes Mar 10, 2017

View reviewed changes

merge MySQL 5.7.9 (GA) semantics into MySQL57Platform

f4381c5

deeky666 force-pushed the fix-mysql57-platform branch from 6b3ba2d to f4381c5 Compare March 10, 2017 12:54

MarcosBL mentioned this pull request Mar 15, 2017

json type throws an error when used with select Laravel-Backpack/CRUD#545

Closed

jnevesjunior approved these changes Mar 17, 2017

View reviewed changes

Ocramius self-assigned this May 9, 2017

Ocramius approved these changes May 9, 2017

View reviewed changes

Ocramius merged commit e98d241 into doctrine:master May 9, 2017

Ocramius mentioned this pull request May 9, 2017

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

Closed

spackmat mentioned this pull request May 12, 2017

DBAL master implemented MySQL-JSON now dunglas/doctrine-json-odm#20

Closed

Tobion mentioned this pull request May 12, 2017

JSON type without encoding/decoding #2723

Closed

LukeTowers mentioned this pull request Sep 12, 2017

JSON column type not supported rainlab/builder-plugin#175

Closed

aymone mentioned this pull request Oct 16, 2017

Upgrade doctrine for JSON support dflydev/dflydev-doctrine-orm-service-provider#90

Open

morozov added the Reserved Keywords label Sep 12, 2019

github-actions bot locked as resolved and limited conversation to collaborators Aug 10, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Merge MySQL 5.7.9 (GA) semantics into MySQL57Platform #2653

Merge MySQL 5.7.9 (GA) semantics into MySQL57Platform #2653

deeky666 commented Feb 9, 2017

phansys Feb 9, 2017

deeky666 Feb 9, 2017

phansys Feb 9, 2017

deeky666 Feb 9, 2017

robhogan commented Feb 10, 2017 •

edited

Loading

Ocramius commented Feb 10, 2017 •

edited

Loading

deeky666 commented Feb 10, 2017

robhogan commented Feb 10, 2017

robhogan Feb 10, 2017

deeky666 Feb 10, 2017

robhogan Feb 10, 2017

deeky666 Feb 10, 2017

deeky666 commented Mar 10, 2017

phansys left a comment

phansys Mar 10, 2017

deeky666 Mar 10, 2017

phansys Mar 10, 2017

deeky666 Mar 10, 2017

Omranic commented Apr 12, 2017

deeky666 commented Apr 12, 2017

mpyw commented May 9, 2017

Ocramius commented May 9, 2017 via email

kimhemsoe commented May 9, 2017

mpyw commented May 9, 2017

Ocramius left a comment

mpyw commented May 9, 2017

le-martre commented Jul 31, 2017

Ocramius commented Jul 31, 2017

le-martre commented Jul 31, 2017

Ocramius commented Jul 31, 2017

hansbogert commented Nov 14, 2018 •

edited

Loading


		if (version_compare($version, '5.7', '>=')) {
		if ($majorVersion === '5' && $minorVersion === '7' && null === $patchVersion) {

Merge MySQL 5.7.9 (GA) semantics into MySQL57Platform #2653

Merge MySQL 5.7.9 (GA) semantics into MySQL57Platform #2653

Conversation

deeky666 commented Feb 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robhogan commented Feb 10, 2017 • edited Loading

Ocramius commented Feb 10, 2017 • edited Loading

deeky666 commented Feb 10, 2017

robhogan commented Feb 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deeky666 commented Mar 10, 2017

phansys left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Omranic commented Apr 12, 2017

deeky666 commented Apr 12, 2017

mpyw commented May 9, 2017

Ocramius commented May 9, 2017 via email

kimhemsoe commented May 9, 2017

mpyw commented May 9, 2017

Ocramius left a comment

Choose a reason for hiding this comment

mpyw commented May 9, 2017

le-martre commented Jul 31, 2017

Ocramius commented Jul 31, 2017

le-martre commented Jul 31, 2017

Ocramius commented Jul 31, 2017

hansbogert commented Nov 14, 2018 • edited Loading

robhogan commented Feb 10, 2017 •

edited

Loading

Ocramius commented Feb 10, 2017 •

edited

Loading

hansbogert commented Nov 14, 2018 •

edited

Loading