Closed (fixed)
Project:
Experience Builder
Component:
Code
Priority:
Critical
Category:
Task
Assigned:
Unassigned
Issue tags:
Reporter:
Created:
30 May 2024 at 13:06 UTC
Updated:
21 Jun 2024 at 19:04 UTC
Jump to comment: Most recent
Problem/Motivation
If a field used in a experience builder field there is nothing stopping the module that provides that field from being uninstalled
Steps to reproduce
- Use a field in experience builder field
- uninstall the module that provide the field
- view the entity that used the field
- Should get a 'Unhandled expression type.' exception
Proposed resolution
we'll need to implement something like `\Drupal\field\FieldUninstallValidator` instead. And thanks to the expressions, we easily can:
- new `\Drupal\experience_builder\FieldUninstallValidator` service
- assume the module being uninstalled provides field type `foobar`
- iterates over all XB field instances, for each field instance:
- determine the DB table of the _current_ data (e.g. `node__field_xb_demo`) as well as the _revision_ data (e.g. `node_revision__field_xb_demo`)
- determine the prop expression prefix that should not occur for this module to be safely uninstallable: `new FieldTypePropExpression('foobar', '')` (yes, use the empty string as the field prop name — not semantically beautiful, but pragmatic at this stage)
- determine which entities use an instance of the `foobar` field type inside`XB`: `SELECT entity_id, JSON_SEARCH(field_xb_demo_props, 'all', '`:information_source:`foobar␟%') FROM node__field_xb_demo`
- For all XB field instances we will also need to check the default values to ensure the field type `foobar` is not used there either
The result is _one query per entity type_ to find where XB uses instances of this field type to assign static props values.
Remaining tasks
User interface changes
API changes
Data model changes
Issue fork experience_builder-3450957
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:
changes
Comments
Comment #2
tedbowComment #3
tedbowComment #4
tedbowAdd a item to the summary that we need to account for default values of XB fields
Comment #5
wim leersCrediting myself for that proposed solution. I wrote that at https://git.drupalcode.org/project/experience_builder/-/merge_requests/2....
Comment #6
wim leersComment #7
tedbowComment #8
wim leersWonderful to have you working on this, @tedbow! You'll be the first one to start using https://mariadb.com/docs/skysql-dbaas/ref/xpand/functions/JSON_EXTRACT/ and other JSON functions in the database 😄
Comment #9
tedbow@Wim Leers well that should be interesting!
I have rough version of finding use of a field if it is in the default value of a field. Next I will work on the actual querying of the column values
Comment #11
tedbow@Wim Leers #8, AFAICT Sqlite does not support json_search? are we going support all core DB supported databases right now? Or I am I wrong about json_search() support?
Comment #12
tedbowOk was able to find matches using
json_extract()but the wildcard key matching works in MariaDB but not SqliteDidn't mean to bump it down from critical before
Comment #13
wim leers#12: Why is that? https://sqlite.org/json1.html#jex says this should work? I'll get SQLite testing going…
Comment #14
wim leersFYI: working to preemptively unblock this issue by ensuring XB is being tested against new enough SQLite version: #3452134: PHPUnit SQLite CI job (+ MariaDB + PostgreSQL).
Comment #15
wim leersComment #16
wim leers#3452134: PHPUnit SQLite CI job (+ MariaDB + PostgreSQL) landed. That should allow us to prove or disprove #12, and once this is merged, we'll have the confidence that it won't break.
Comment #17
wim leersBTW, for conditionally skipping tests depending on the active DB (and hence depending on CI job), see
\Drupal\KernelTests\Core\Database\DriverSpecificKernelTestBase::setUp()for inspiration.Comment #18
catchAdding a reference to #3440578: [PP-2] JSON-based data storage proposal for component-based page building so it's easier to find in both directions.
Comment #19
tedbowComment #20
catchI'm a bit confused by this. Which I could possibly answer by reading the code base more, but also feels a bit conceptual so being lazy instead.
My understanding of the current approach is:
1. There is a 'xb json field' which holds all the data relevant to XB.
2. This 'xb json field' holds components and props etc. which are based on 'field types'.
3. However, the different field types that are used in the 'xb json field' don't have any kind of representation as field.storage.* field.field.* config entities assigned to the entity bundle, they are are instead dynamically used based on the components available.
If #3 is correct, then there is no such thing as 'deleting a field' with this data model, because there is no field (in the current entity bundle + fields sense) to delete.
If #3 is wrong, and we have to create field.storage.* and field.field.* config entities for each bundle that's using components in xb that need those field types, then the very existence of those fields should prevent uninstall (because they will impact the components available to use), not whether they have content.
If whatever provides the components/props has an explicit dependency on the field types, then it also feels like we could use that. i.e. an image component with an image widget/formatter using the image field type being available on a bundle should prevent uninstall of the image module. To uninstall image module, you'd need to remove that component, and then removing the component would depend on whether there's any content using it.
Comment #21
larowlan@catch it's not fields, it's references in the tree to values in other fields
Eg a hero title component might reference a hero image field and title field in a node. At present if the hero image field is deleted it fatals
The intent is that some components store arbitrary data but some are backed by structured fields. The intention is this will allow editing the field values and the 'layout' in one place
Comment #22
larowlanIn terms of the why, a hero image is a good example - you could imagine this would be used in the full page view but also in a card view mode for a node
Having some structured data at the entity level allows for reuse
Comment #23
catchIf the 'hero title component' is enabled for an entity bundle, that would also break if the field is deleted, so shouldn't we be preventing deletion of the field while the component is enabled, AND also prevent deletion of the component if it's used in any content?
edit:
Also where/how are these fields created, .e.g. https://git.drupalcode.org/project/experience_builder/-/blob/0.x/src/Sdc... is dynamically working with field types to bypass the field actually needing to exist - it's stuff that gave me the impression there are no 'real' fields outside the XB one, but sounds like you're saying there would be actual field and field storages for every field that is associated with any prop? But if that's the case, why not just rely on those to prevent uninstall?
Comment #24
tedbow@catch @larowlan sorry we really to document better how this works
for now I have created #3452848: [PP-1] Test coverage to prove configurable fields cannot be deleted from content entity types if they are used in XB Content Type Template because the "props" in XB field could dynamically reference an actual field of a entity being displayed. In which case we would want to prevent the field on the bundle from being deleted
This current issue deals with a static field where it basically just uses the "shape" of a field type and stores the values in the props too(what we are calling "static"). In this case the values for the field props would be stored in the "props" column of the XB field
Comment #26
wim leersMerged in upstream changes + resolved conflicts after #3452497: [MR Only] Edit any component prop, powered by a new FieldForComponentSuggester service, which will power the JS UI landed.
Comment #27
wim leersComment #29
wim leers