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

Added JSON type to MySQL and PostgreSQL schema #15560

Merged
merged 2 commits into from
Feb 6, 2018

Conversation

zhukovra
Copy link
Contributor

@zhukovra zhukovra commented Jan 25, 2018

Q A
Is bugfix? no
New feature? yes
Breaks BC? no
Tests pass? yes
Fixed issues #13019, #15247

This is fixed and tests enriched PR #13783.
Correct realization of PR #15266.

Status: working on tests stabilization under new versions of DBs.

@zhukovra zhukovra changed the title Added JSON type to MySQL and PostgreSQL schema WIP Added JSON type to MySQL and PostgreSQL schema Jan 25, 2018
@zhukovra zhukovra closed this Jan 26, 2018
@zhukovra zhukovra deleted the develop-json branch January 26, 2018 08:09
@zhukovra zhukovra reopened this Jan 26, 2018
@samdark
Copy link
Member

samdark commented Jan 26, 2018

@SilverFire I think you'd like to take a look at it.

@zhukovra
Copy link
Contributor Author

@samdark FYI I want to separate that PR with excluding "Fixed default values loaded from schema for PostgreSQL" commit because I fix it for test passing on PostgreSQL 9.6.

Another important question for me is why Travis tests works on outdated and rarely used versions of databases servers? Where can I read about that decision policy?

@samdark
Copy link
Member

samdark commented Jan 26, 2018

Travis works with whatever is provided by default. We can merge adjusted travis.yml if needed.

@samdark
Copy link
Member

samdark commented Feb 4, 2018

@zhukovra would you please merge with master? Also... what's left for the pull request to be finished?

@zhukovra
Copy link
Contributor Author

zhukovra commented Feb 5, 2018

@samdark I'll try finish PR today or tomorrow.

@SilverFire
Copy link
Member

Please pay attention to changes introduced by #15348

@zhukovra
Copy link
Contributor Author

zhukovra commented Feb 5, 2018

I've found a bug with JSON column type in MySQL that will not be fixed in PHP < 5.6. Failed test is here.

What do I need to do with it? Should I use version_compare() for making that test pass?

@SilverFire
Copy link
Member

Yes, throw exception and leave a TODO comment to drop this check in Yii 2.1

@zhukovra zhukovra changed the title WIP Added JSON type to MySQL and PostgreSQL schema Added JSON type to MySQL and PostgreSQL schema Feb 5, 2018
@zhukovra
Copy link
Contributor Author

zhukovra commented Feb 5, 2018

@SilverFire @samdark please review.

Copy link
Member

@samdark samdark left a comment

Choose a reason for hiding this comment

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

Generally code is fine. There are minor issues to fix and questions to answer. Also a line for changelog is needed.

.travis.yml Outdated
- mysql-5.7-trusty
packages:
- libmysqlclient-dev
- libmysqlclient20
Copy link
Member

Choose a reason for hiding this comment

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

Are both libraries necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope. Copypaste from outdated Travis instruction. Fixed.

.travis.yml Outdated
packages:
- libmysqlclient-dev
- libmysqlclient20
- mysql-community-client
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need a client?

.travis.yml Outdated
- mysql-client-core-5.6
- mysql-client-5.6
- libmysqlclient-dev
- libmysqlclient20
Copy link
Member

Choose a reason for hiding this comment

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

See above.

.travis.yml Outdated
- mysql-client-core-5.6
- mysql-client-5.6
- libmysqlclient-dev
- libmysqlclient20
Copy link
Member

Choose a reason for hiding this comment

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

See above.

@@ -272,4 +272,25 @@ public function money($precision = null, $scale = null)

return $this->getDb()->getSchema()->createColumnSchemaBuilder(Schema::TYPE_MONEY, $length);
}

/**
* Creates a json column.
Copy link
Member

Choose a reason for hiding this comment

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

json -> JSON

*/
public function json()
{
/**
Copy link
Member

Choose a reason for hiding this comment

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

Since it's not a phpdoc block, use /* with single *.

/**
* TODO Remove in Yii 2.1
*
* Disabled due bug in MySQL extension
Copy link
Member

Choose a reason for hiding this comment

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

due to bug

@@ -249,7 +249,7 @@ public function testGetPDOType()

public function getExpectedColumns()
{
return [
$columns = [
Copy link
Member

Choose a reason for hiding this comment

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

returning it right away was fine.

]);
];

/**
Copy link
Member

Choose a reason for hiding this comment

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

Use /*

@samdark samdark added this to the 2.0.14 milestone Feb 5, 2018
@samdark
Copy link
Member

samdark commented Feb 5, 2018

As I understand, this PR fixes everything that #15266 was going to fix, right?

@zhukovra
Copy link
Contributor Author

zhukovra commented Feb 6, 2018

@samdark I think that bugs #15266 was fixed by @SilverFire. PR updated. Thank you for review.

Copy link
Member

@SilverFire SilverFire left a comment

Choose a reason for hiding this comment

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

Looks good to me

@samdark
Copy link
Member

samdark commented Feb 6, 2018

A line for changelog is needed.

@@ -4,6 +4,7 @@ Yii Framework 2 Change Log
2.0.14 under development
------------------------

- Enh #13019: Support JSON in SchemaBuilderTrait (zhukovra, undefinedor)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@samdark but it is already there

@samdark samdark merged commit 40b0383 into yiisoft:master Feb 6, 2018
@samdark
Copy link
Member

samdark commented Feb 6, 2018

Merged. Thank you!

@titanproger
Copy link

Hello every one!
I have a question about new json support on PostgreSQL.
Is it way to create a json field by migration now?
I mean not a "jsonb" but "json" type.
example

$this->createTable('table_name', [
            "id" => $this->bigPrimaryKey(),
            "body" => "json",   // warning "json" generates jsonb type
        ]);

First my think - is create own QueryBuilder width $typeMap = [
....
"json_raw" => 'json'
]
and use json_raw abstraction type.

@finnan444
Copy link

finnan444 commented Apr 3, 2018

@titanproger Hi, i also encountered this issue, may be you created a new issue?
BTW my Postgres creates in migration not json, but jsonb by default
IMHO, this flag should be added to \yii\db\SchemaBuilderTrait::json() as parameter

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants