Cannot delete a field which uses JSON type

ugh picked the wrong branch

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

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

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

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? 🤔

I think we should add JSON storage support in ✨ Add "json" as core data type Active . If that takes too long, we should at least at testing for the change and a comment about why the change is done.

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.

@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 ✨ Add "json" as core data type Active 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.

Lets skip the test. We can do that in 📌 Move hook_schema out of ban.install and create the table lazy Active .

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?

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

Increasing priority as suggested by @catch at

The core issue hasn't had an update for six months, seems like the priority could be increased from 'normal'. But also there needs to be a tracking issue for XB to update this code once it's fixed. That will probably requires a dependency on the minor release that fixes the bug, unless we determine it can be fixed in a patch release of core.

— 🐛 [PP-1] Use JSON schema type for sqlite and remove text workaround Active

I'm a bit confused by #12, ban module doesn't use a JSON field type, and it's slated for removal to contrib anyway, is 📌 Move hook_schema out of ban.install and create the table lazy Active the right issue link?

Sounds to me like @daffie is happy for this to go in without tests - so we just need a comment explaining why we're doing a stopgap and a link to the bigger issue in a @todo? Anything else?

@catch indicated in 🐛 [PP-1] Use JSON schema type for sqlite and remove text workaround Active this could also go into a bugfix release.

This isn't limited to fields, was able to add a simple test to the generic schema test that fails without this change on sqlite (only).

Ready for review

MR looks trivial.

Can a core committer please queue the test-only CI job on SQLite to prove @larowlan's ? 🙏

sqlite pipeline running at https://git.drupalcode.org/issue/drupal-3487533/-/pipelines/519476

The test only job on https://git.drupalcode.org/issue/drupal-3487533/-/jobs/5524833 isn't showing a failure. This might be an issue with the test-only job though, not sure the last time we needed to run that job on a specific database.

0c728283617a:/data$ phpunit -c core core/modules/sqlite/tests/src/Kernel/sqlite/SchemaTest.php --filter=testSchema
⏳️ Bootstrapped tests in 0 seconds.
🐘 PHP Version 8.3.11.
💧 Drupal Version 11.3-dev.
🗄️  Database engine sqlite.
PHPUnit 10.5.46 by Sebastian Bergmann and contributors.

Runtime:       PHP 8.3.11
Configuration: /data/app/core/phpunit.xml

E.....                                                              6 / 6 (100%)

Time: 00:10.267, Memory: 14.00 MB

There was 1 error:

1) Drupal\Tests\sqlite\Kernel\sqlite\SchemaTest::testSchema
Exception: Unable to parse the column type JSON

/data/app/core/modules/sqlite/src/Driver/Database/sqlite/Schema.php:534
/data/app/core/modules/sqlite/src/Driver/Database/sqlite/Schema.php:42
/data/app/core/modules/sqlite/src/Driver/Database/sqlite/Schema.php:567
/data/app/core/tests/Drupal/KernelTests/Core/Database/DriverSpecificSchemaTestBase.php:283

Output from running locally shows the expected error from the issue summary in the test

Wanted to confirm this really does fail on gitlab just in case there's an sqlite version issue or similar, so opened https://git.drupalcode.org/project/drupal/-/merge_requests/12359 with the fix reverted. Tests are still running so hasn't answered the question yet.

Here we go, just the test only job is for whatever reason not able to show it:

https://git.drupalcode.org/issue/drupal-3487533/-/jobs/5531634#L970

Late here but now the rc is out I think we should get this in asap, so will try to commit tomorrow if no-one beats me to it.

Thanks @catch

This looks good.

Is the only follow-up here ✨ Add "json" as core data type Active or do we need an additional small follow-up to add the mapping to the other core drivers? Tagging for follow-up to determine that but it might be existing JSON issues cover it enough.

Committed/pushed to 11.x and cherry-picked to 11.2.x, thanks!

catch → closed merge request !10188

Thanks @catch, yes that's the only follow up we need as far as I'm aware - removing the tag

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