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
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

andypost’s picture

Status: Active » Needs review
FileSize
2.43 KB

Also better to inherit jsonb

erik.erskine’s picture

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

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

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

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

Assigned: Unassigned » kevinquillen
kevinquillen’s picture

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

FileSize
3.45 KB

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

andypost’s picture

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

Patch has been applied and pushed to dev for review

kevinquillen’s picture

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

andypost’s picture

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

Status: Fixed » Closed (fixed)

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