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

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 📌 Refactor the XB field type to be multi-valued, to de-jsonify the tree, and to reference the field_union type of the prop values Active happens, you agree we wouldn't need to do this, right?

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 ✨ JSON-based data storage proposal for component-based page building Active .

The reason for this being 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 column-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).

So even though what's in the JSON field would be smaller, it would still be a JSON field, so need similar treatment.

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

With 📌 [PP-1] Consider not storing the ComponentTreeStructure data type as a JSON blob Postponed 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 🐛 Cannot delete a field which uses JSON type Active to critical.

This would be painful to do after 🌱 Milestone 1.0.0-beta1: Start creating non-throwaway sites Active .

If 🐛 Cannot delete a field which uses JSON type Active 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.

Thoughts, @larowlan?

Seems like we will need to go down that path yep

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

Ah, yes, 📌 [PP-1] Consider not storing the ComponentTreeStructure data type as a JSON blob Postponed completely changed the schema! 😄 Only one JSON blob left :)

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 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 🐛 Cannot delete a field which uses JSON type Active . 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 anytime, so why does XB need to do it before core's driver supports it?

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

Also the current MR in 🐛 Cannot delete a field which uses JSON type Active 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.

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.

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

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.

Thanks

I've added a second branch (3520923-json-schema-if-core-issue-landed) to be ready when 🐛 Cannot delete a field which uses JSON type Active lands

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

wim leers → closed merge request !1110

@effulgentsia: 📌 Page has Metatag integration Active 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 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!

🐛 Cannot delete a field which uses JSON type Active is in 11.x/11.2.x so will be released with 11.2.0

Thanks @catch!

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.

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

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.

HAH! Yes, why the hell not?! 😅

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

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.

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.

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.

Postponing this on 🐛 PHPUnit Next Major tests failing Active 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.

It's already being done in 🐛 PHPUnit Next Major tests failing Active in this commit: https://git.drupalcode.org/project/experience_builder/-/merge_requests/4...