Field should use "native" prefix so no need in core patches

CommentFileSizeAuthor
#8 2480201-8.patch3.45 KBerik.erskine
#1 2480201-1.patch2.43 KBandypost

Comments

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Active » Needs review
StatusFileSize
new 2480201-1.patch2.43 KB

Also better to inherit jsonb

erik.erskine’s picture

erik.erskine commented
Status: Needs review » Needs work 
  1. +++ b/src/Plugin/Field/FieldType/JsonItem.php
@@ -33,6 +33,8 @@ class JsonItem extends FieldItemBase {
           'type' => 'json',
+          'pgsql_type' => 'json',
+          'mysql_type' => 'json',

    json isn't a valid generic type according to the schema API, nor a valid MySQL type. However I don't see a reason why this module can't store the serialised string as text on non PostgreSQL databases. 

  2. +++ b/src/Plugin/Field/FieldType/JsonbItem.php
@@ -32,7 +31,9 @@ class JsonbItem extends FieldItemBase {
+          'type' => 'text',
+          'pgsql_type' => 'jsonb',
+          'mysql_type' => 'json',

    As above, the MySQL type isn't valid here. We can probably remove the MySQL specific line and default to the generic text type.

kevinquillen’s picture

kevinquillen commented

MySQL JSON has implemented a JSON type as of MySQL JSON 5.7.7. It reads like their implementation is the combination of both JSON/JSONB from Postgres.

It is only a labs release right now. When will it be merged into the main MySQL branch, I am not sure.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Needs work » Needs review

Also MariaDB 10.0.16 supports that

So probably we need to add check for db version in hook_requirements and readme update too

kevinquillen’s picture

kevinquillen commented

Will do.

I suspect we will need a way to implement JSON specific query altering as well? Or should we leave that to developers just wanting to store the data? Might be quite useful to work in Views support at the least.

kevinquillen’s picture

kevinquillen commented
Assigned: Unassigned » kevinquillen
kevinquillen’s picture

kevinquillen commented

I haven't forgotten about this. I am trying to get MySQL version selection into my VM build so I am not limited to just Postgres.

erik.erskine’s picture

erik.erskine commented
StatusFileSize
new 2480201-8.patch3.45 KB

Rerolled #1, because FormatterBase::viewElements function has changed.

andypost’s picture

andypost
he/him
Primary language Russian
commented

Mysql should do a version check, but could be expensive

  • kevinquillen committed e568730 on 8.x-1.x authored by ingaro 
    Issue #2480201 by andypost, ingaro: Clean schema implementation for...
kevinquillen’s picture

kevinquillen commented

Patch has been applied and pushed to dev for review

kevinquillen’s picture

kevinquillen commented

andypost & erik, are we good here in 8.1.x?

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Needs review » Fixed

I'm OK with current code, and still waiting for beta1
Suppose db version check needs separate issue, just to display warning if unsupported version used with suggestion to upgrade to minimal version to get speedup

andypost’s picture

andypost
he/him
Primary language Russian
commented

Filed #2793899: Add hook_requirements() about minimum suggested version
Looking for beta release

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.