Problem/Motivation

Discovered in #3487077: Page has Metatag integration.

When trying to uninstall Experience Builder, it crashes with "Unable to parse the column type JSON".

That's because \Drupal\sqlite\Driver\Database\sqlite\Schema::getFieldTypeMap does not contain a mapping for JSON fields. No other driver errors for this. That's because \Drupal\sqlite\Driver\Database\sqlite\Schema::introspectSchema is SQLite specific.

So when deleting the field it crashes.

Steps to reproduce

Install XB or any field using json type. Try to uninstall or delete field. It crashes

         'pgsql_type' => 'jsonb',
          'mysql_type' => 'json',
          'sqlite_type' => 'json',
          'not null' => FALSE,

Proposed resolution

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

Issue fork drupal-3487533

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

mglaman created an issue. See original summary.

mglaman’s picture

mglaman
Primary language English
Location WI, USA
CreditAttribution: mglaman at Acquia commented
Version: 11.1.x-dev » 11.x-dev

ugh picked the wrong branch

mglaman changed the visibility of the branch 3487533-cannot-delete-a to hidden.

mglaman opened merge request !10188

mglaman’s picture

mglaman
Primary language English
Location WI, USA
CreditAttribution: mglaman at Acquia commented
Status: Active » Needs review
dmitry.korhov’s picture

dmitry.korhov
Primary language English
Location Warsaw, Poland
CreditAttribution: dmitry.korhov at EPAM Systems commented
Status: Needs review » Reviewed & tested by the community

Looks straightforward, so there are no issues with merging this.

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented

For consistency should we add this to getFieldTypeMap() for the other drivers anyway?

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: wim leers at Acquia commented
Issue summary: View changes
Issue tags: +Contributed project blocker
Related issues: +#3487077: Page has Metatag integration

So this bit in the Experience Builder module revealed this bug:

  /**
   * {@inheritdoc}
   */
  public static function schema(FieldStorageDefinitionInterface $field_definition) {
    return [
      'columns' => [
        'tree' => [
          'description' => 'The component tree structure.',
          'type' => 'json',
          'pgsql_type' => 'jsonb',
          'mysql_type' => 'json',
          'sqlite_type' => 'json',
          'not null' => FALSE,
        ],
        'props' => [
          'description' => 'The prop values for each component in the component tree.',
          'type' => 'json',
          'pgsql_type' => 'jsonb',
          'mysql_type' => 'json',
          'sqlite_type' => 'json',
          'not null' => FALSE,
        ],
      ],
      'indexes' => [],
      'foreign keys' => [
        // @todo Add the "hash" part the proposal at https://www.drupal.org/project/drupal/issues/3440578
      ],
    ];
  }

— \Drupal\experience_builder\Plugin\Field\FieldType\ComponentTreeItem::schema()

But I made this use exactly the same pattern that Metatag uses, hence this bit:

* @see https://git.drupalcode.org/project/metatag/-/blob/2.0.x/src/Plugin/Field/FieldType/MetatagFieldItem.php

— \Drupal\experience_builder\Plugin\Field\FieldType\ComponentTreeItem.

So why is the Metatag module not running into the same problem? 🤔

daffie’s picture

daffie CreditAttribution: daffie at Finalist commented

I think we should add JSON storage support in #3343634: [PP-1] Add "json" as core data type to Schema and Database API. If that takes too long, we should at least at testing for the change and a comment about why the change is done.

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Status: Reviewed & tested by the community » Needs work

NW for #9. I think this is OK as a stopgap if we can get test coverage (and also answer #7). I would definitely like to see JSON support done properly in core but I'm also concerned about how long it will take given the tight timelines we have on Experience Builder.

mglaman’s picture

mglaman
Primary language English
Location WI, USA
CreditAttribution: mglaman at Acquia commented

@wim leers metatag uses text for all? https://git.drupalcode.org/project/metatag/-/blob/2.0.x/src/Plugin/Field.... I think I know what you're talking about, though. Is it the json_field? https://git.drupalcode.org/project/json_field/-/blob/8.x-1.x/src/Plugin/...

Note they use text for the sqlite_type! XB does not.

     'columns' => [
        'value' => [
          'type' => 'json',
          'pgsql_type' => 'json',
          'mysql_type' => 'json',
          'sqlite_type' => 'text',
          'not null' => FALSE,
        ],
      ],
    ];

re #9 I highly doubt #3343634: [PP-1] Add "json" as core data type to Schema and Database API is going to make enough progress anytime soon.

we should at least at testing for the change and a comment about why the change is done.

I'll see if I can make a quick enough test. Hopefully without a field type and just manipulating a table with a JSON column.

I don't have capacity to write the test.

daffie’s picture

daffie CreditAttribution: daffie at Finalist commented

Lets skip the test. We can do that in #3379255: [PP-1] Replace ban_schema() implementation with ::ensureTableExists().

mglaman’s picture

mglaman
Primary language English
Location WI, USA
CreditAttribution: mglaman at Acquia commented

Okay, so what's left? Add this line to each driver? I don't know what value the in-code comment would provide. Or would the comment be for why we only added it to SQLite and not the others?

      // @note: Only the SQLite driver has this field map to due to a fatal error caused by this driver's schema
      // @todo: Add to all drivers in XYZ
      'json:normal'     => 'JSON',

Something like this?

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: wim leers at Acquia commented

#11: oh, damn, mea culpa! 😬 I failed to document that correctly then 😮‍💨