Overview

ComponentTreeItem has the following @todo/workaround. I can't find an Experience Builder issue for this.

        'inputs' => [
          'description' => 'The inputs for each component in the component tree.',
          'type' => 'json',
          'pgsql_type' => 'jsonb',
          'mysql_type' => 'json',
          // @todo Change back to 'json' once https://www.drupal.org/i/3487533 is resolved.
          'sqlite_type' => 'text',
          'not null' => FALSE,
        ],

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.

Proposed resolution

User interface changes

Issue fork experience_builder-3520923

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

catch created an issue. See original summary.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Related issues: +#3477428: [PP-1] Refactor (or decide not to) the XB field type to be multi-valued, to de-jsonify the tree, and to reference the field_union type of the prop values

Those @todos date back to the earliest PoCs, when we didn't have a single d.o issue yet! Yes, many parts of XB are unchanged from the early PoCs. 😅

Just to make sure: if #3477428: [PP-1] Refactor (or decide not to) the XB field type to be multi-valued, to de-jsonify the tree, and to reference the field_union type of the prop values happens, you agree we wouldn't need to do this, right?

catch’s picture

catch
he/him
Primary language English
commented

I think that issue would still result in a JSON column to store the field values.

From the current issue summary:

data_sources (json): The sourceTypes and expression portion of what's currently in the props column (prior to this proposed refactoring) for this component instance. For example:

static_values (json): The value portion of what's currently in the props column (prior to this proposed refactoring)

Or see option #3 "Field Union JSON" from the issue summary of #3440578: [PP-2] JSON-based data storage proposal for component-based page building which is what that issue spun out of (I still think there's some non-alignment between the two, but row-per-component + single json column for props/field values is consistent between the two iirc.

The reason for the JSON column that it's a single field type, and that field type needs to be able to handle the equivalent of compound field API fields, and we wouldn't want 'value_1', 'value_2', 'value_3' etc. columns (brings back memories of flexinode).

So row-per-component doesn't eliminate JSON, it only eliminates a lot of the nesting, and allows individual components to be accessed without parsing them out of the JSON tree when that might be necessary (as discussed in the alternative rendering issue). Even though what's in the JSON column would be smaller, it would still be a JSON column, so need similar treatment to what's here afaict.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Parent issue: » #3520449: [META] Production-ready data storage
Related issues: +#3487533: Cannot modify a table which uses JSON type

I think that issue would still result in a JSON column to store the field values.

With #3468272: Store the ComponentTreeStructure field property one row per component instance now in and given your analysis at #3523842-13: Spike: Explore storing a hash lookup of the ComponentInputs field property: one hash per component instance (which @effulgentsia and I +1'd), it's safe to assume by now that this will indeed remain the case.

Which means this must still happen.

Just bumped #3487533: Cannot modify a table which uses JSON type to critical.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Issue tags: +beta blocker

This would be painful to do after #3515932: Milestone 1.0.0-beta1: Enable creation of non-throwaway sites.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Active » Needs review

If #3487533: Cannot modify a table which uses JSON type doesn't happen in time, AFAICT we could still do a https://www.drupal.org/project/mysql56-style approach?

It'd reuse everything in core/modules/sqlite/src/Driver/Database/sqlite/Install as-is, except for \Drupal\sqlite\Driver\Database\sqlite\Schema::introspectSchema(), which it would decorate.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Title: [PP-1] Use JSON schema type for sqlite and remove text workaround » Use JSON schema type for sqlite and remove text workaround
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Assigned: Unassigned » larowlan
Status: Needs review » Needs work

Thoughts, @larowlan?

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Assigned: larowlan » Unassigned

Seems like we will need to go down that path yep

isholgueras made their first commit to this issue’s fork.

isholgueras’s picture

isholgueras commented
Issue summary: View changes

Remove duplicated code and tree in description for clarity because it doesn't exist, only inputs.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Title: Use JSON schema type for sqlite and remove text workaround » Use `json` schema type for SQLite and remove `text` workaround
Related issues: +#3468272: Store the ComponentTreeStructure field property one row per component instance

Ah, yes, #3468272: Store the ComponentTreeStructure field property one row per component instance completely changed the schema! 😄 Only one JSON blob left :)

isholgueras opened merge request !1110

effulgentsia’s picture

effulgentsia commented

This MR doesn't need the Drupal\Driver\Database\sqlite classes, only the Drupal\experience_builder\... classes. The former was only for Drupal 9 and maybe early minor releases of 10. https://www.drupal.org/project/mysql57 and https://www.drupal.org/project/sqlite337 can be used for reference for Drupal 11.

drush site-install doesn't work if I have the namespace

Yeah, I've run into this too with https://www.drupal.org/project/sqlite337 when installing without an already existing settings.php. What I found is I need to use the browser installer (which lets you select whether you want core's sqlite driver or a differently namespaced one) to write out settings.php the first time, and then drush site-install works if I keep that settings.php around.

Stepping back a bit though, I'm curious why XB needs to leap ahead of #3487533: Cannot modify a table which uses JSON type. Does SQLite itself actually have a json type? I thought it only had text and jsonb, and you can migrate from text to jsonb both incrementally and at any time, so why does XB need to do it before core's driver supports it?

effulgentsia’s picture

effulgentsia commented

Additional reference material: https://www.sqlite.org/datatype3.html and https://www.sqlite.org/stricttables.html.

catch’s picture

catch
he/him
Primary language English
commented

Also the current MR in #3487533: Cannot modify a table which uses JSON type is a one-liner - it looks like it either needs test coverage or possibly just a comment, so why go to all the trouble here instead of trying to get that one committed? If it can land in more or less its current state, I think it could into a patch release of core.

isholgueras’s picture

isholgueras commented

Also the current MR in #3487533: Cannot delete a field which uses JSON type is a one-liner - it looks like it either needs test coverage or possibly just a comment, so why go to all the trouble here instead of trying to get that one committed?

I think that would be ideal.

I'll check what it needs to be done or which tests needs to be updated/created in core.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented

Should we postpone this on the basis of #16 and #17?
Getting it fixed in core feels like a much lower maintenance option.

catch’s picture

catch
he/him
Primary language English
commented

That's what I was trying to suggest when opening this issue - that the workaround could be removed in this issue once the core issue is fixed.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Title: Use `json` schema type for SQLite and remove `text` workaround » [PP-1] Use `json` schema type for SQLite and remove `text` workaround
Status: Needs work » Postponed

Thanks

isholgueras opened merge request !1121

isholgueras’s picture

isholgueras commented

I've added a second branch (3520923-json-schema-if-core-issue-landed) to be ready when #3487533: Cannot modify a table which uses JSON type lands

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

Sounds great to me — I thought that that ship had sailed, but this is obviously better 😄

wim leers closed merge request !1110

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Related issues: +#3487077: Page has Metatag integration

@effulgentsia: #3487077: Page has Metatag integration introduced this — grep that for "SQLite" to find details for why we ended up here.

@catch rightfully called out the work-around that #3487077 added with a @todo pointing to a core issue as something we should get resolved.

you can migrate from text to jsonb both incrementally and at any time, so why does XB need to do it before core's driver supports it?

While this is great for an update path, it would still mean that this issue remains at a stalemate:

  1. with the work-around, @catch's concern remains (AFAICT at least — despite this excellent remark of yours)
  2. without the work-around, XB's test suite would fail on SQLite

So: keeping at Critical for now, to just land the damn core patch 😇, given the broad consensus by core committers @catch and @larowlan here :) It "should" take mere minutes based on all recent comments!

catch’s picture

catch
he/him
Primary language English
commented
Title: [PP-1] Use `json` schema type for SQLite and remove `text` workaround » Use `json` schema type for SQLite and remove `text` workaround
Status: Postponed » Active

#3487533: Cannot modify a table which uses JSON type is in 11.x/11.2.x so will be released with 11.2.0

isholgueras’s picture

isholgueras commented

Thanks @catch!

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Title: Use `json` schema type for SQLite and remove `text` workaround » [11.1.9-and-up] Use `json` schema type for SQLite and remove `text` workaround
Status: Active » Postponed

https://www.drupal.org/project/drupal/releases/11.1.8 is from June 5.

Until 11.1.9 is out, we can't do this yet.

catch’s picture

catch
he/him
Primary language English
commented

There won't be an 11.1.9 unless there's a security release, and commits stopped on the branch once 11.1.8 was out. This will get released with 11.2.0 (next week) so experience builder should be able to add an > 11.2.0 constraint and use it from then.

catch’s picture

catch
he/him
Primary language English
commented
Title: [11.1.9-and-up] Use `json` schema type for SQLite and remove `text` workaround » [11.2.0-and-up] Use `json` schema type for SQLite and remove `text` workaround
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

#29: That works too! 👍 Thanks for commenting, much appreciated!

effulgentsia’s picture

effulgentsia commented

Any downside to doing a version check?

'sqlite_type' => version_compare(\Drupal::VERSION, '11.2', '>=') ? 'json' : 'text'

I'm not clear if any upgrade path would actually be needed or if the above would just automatically make things better once people upgrade to 11.2.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Postponed » Needs work

HAH! Yes, why the hell not?! 😅

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented

Fairly sure that will result in mismatched entity field definition warnings in status reports unless we add an update hook

catch’s picture

catch
he/him
Primary language English
commented

11.2.0 should be tagged this week, so would be easier to get an MR ready changing the type and adding the constraint IMO.

effulgentsia’s picture

effulgentsia commented

We're going to have some early XB adopters at Acquia (and maybe elsewhere, but I only know about the ones at Acquia) running XB beta1 (which is great for helping us find problems ahead of tagging a stable release) who won't be able to upgrade to Drupal 11.2 right away, so we're not quite ready yet to constrain XB to 11.2 only.

catch’s picture

catch
he/him
Primary language English
commented

Why would early adopters at Acquia be capable of running the first beta of experience builder but not a stable release of Drupal core?

As I understand it the goal for a beta for experience builder is July which is weeks after 11.2 will be out.

IMO those early (but in the case of core, late?) adopters could use composer lenient and apply a patch if they really need to run against old versions instead of adding even more unnecessary technical debt to experience builder for everyone else.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Issue tags: +core
effulgentsia’s picture

effulgentsia commented
Title: [11.2.0-and-up] Use `json` schema type for SQLite and remove `text` workaround » [PP-1] Use `json` schema type for SQLite and remove `text` workaround
Status: Needs work » Postponed
Issue tags: -beta blocker +beta target, +stable blocker

Postponing this on #3492722: Update XB to require Drupal 11.2 which is a beta blocker. Once that's in, this issue might be quite trivial, but if it's not we're not going to hold up a beta on it. Tagging it as a beta target though so that we at least attempt to get it in if it's quick.

isholgueras’s picture

isholgueras commented

It's already being done in #3492722: Update XB to require Drupal 11.2 in this commit: https://git.drupalcode.org/project/experience_builder/-/merge_requests/4...

isholgueras’s picture

isholgueras commented
Status: Postponed » Closed (duplicate)
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

Reflected in #3520449: [META] Production-ready data storage 👍

isholgueras closed merge request !1121