Revisit storage of dependencies in separate table now we have separate deltas per component

larowlan → credited catch → .

Thanks, @catch and @larowlan!

🤔

I was thinking this would be less preferable, because it would require to query not 1 table when a dependency is removed (config deleted, module uninstalled …), but N tables.
With N being easily dozens of tables:

1. F (number of XB fields), which is currently up to 1 per content entity type, bundle pair
2. D (DB tables per field): 2 aka current data table + revision table
3. E (currently none, but if we end up doing one field per exposed slot too, then it could easily become 10 times as many — assuming 10 exposed slots)

I would like nicely normalized, self-contained data too, but it seems like the consequences here are quite big?

@catch Is your concern still that the the data in the current single stand-alone “dependencies” table can get out of sync?

When looking into this I realised we missed 🐛 \Drupal\experience_builder\Plugin\DataType\ComponentInputs::getPropSources needs to take \Drupal\experience_builder\Plugin\ExperienceBuilder\ComponentSource\GeneratedFieldExplicitInputUxComponentSourceBase::rawInputValueToPropSourceArray into account Active and will fix that tomorrow

I think because prop sources can have dependencies, we might not be able to do this and may need to retain the database table.

So for \Drupal\experience_builder\Plugin\Field\FieldType\ComponentTreeItem::calculateFieldItemValueDependencies we call \Drupal\experience_builder\Plugin\DataType\ComponentInputs::calculateDependencies which in turn calls \Drupal\experience_builder\PropSource\ContentAwareDependentInterface::calculateDependencies on these prop sources.

Now we know the inputs column on field items cannot contain dynamic or adapted prop sources, so we don't have to consult them for their dependencies with content entities. The static prop sources we know about from the prop field definitions on the config entity and can hence consult them (but currently do not).

But they can contain default-relative-url prop sources however at present these just depend on the component, so we already have that.

With config entities, we can call ::calculateDependencies and it will compute this (i.e what catch proposed in the issue).

So I think in theory this is possible but it probably warrants a spike to see what the performance difference/impact is.

@catch Is your concern still that the the data in the current single stand-alone “dependencies” table can get out of sync?

Multiple concerns to be honest:

1. Even if it's updated in a transaction when the entity is saved, the dependencies of configuration entities can change in the background. One example would be a field plugin moving from one module to another. No-one is going to manually update the XB dependencies table if they move a field plugin between two custom modules. Another example would be a plugin rename. Think of the groups 1/2/3 update where group_content was renamed to group_relationship for one high usage contrib module that did major surgery to entity type and plugin names.

2. The denormalization means duplication of data, which means bigger databases.

3. Any other change to how config dependencies are calculated (like something in XB itself, or even in the core config system if we found a bug with dependencies not being added correctly, which we have before) would require updating the table too.

4. Longer term, but I think it will eventually conflict with ✨ [meta] Add database driver for MongoDB to Core as experimental Active (and the contrib driver), whereas entity queries would not.

On the multiple tables:

#4.D isn't an issue, because revision tables always also include the values for the current revision - so for this kind of audit operation, only the revision tables need to be checked.

#4.E I think multiple fields on a bundle could be done with an OR condition in a single query.

If we only need to prevent uninstall of a module or deletion of a configuration entity, we can return early as soon as a single record is found without querying additional tables anyway, although it would limit the detail in the validation message.

Also it does not seem hugely likely to me that many, many different entity bundles would use the xb field, more like 2-3 usually? XB for layouts/content templates obviously a lot more than that, but that doesn't require the field.

Even if it's 50 database queries, if those are well indexed, that's 50 or 100ms, so for rare audit operations, it should be OK - but I really think we're talking about 1-10 queries on the vast majority of sites. We're talking about something that linearly scales with the number of fields, but should still be very efficient regardless of the amount of content - as long as there's an index on the columns we need to check like component (this would be something else to check regardless of this issue, for update paths etc. too)

I think if 📌 [PP-1] Consider not storing the ComponentTreeStructure data type as a JSON blob Postponed had been done earlier, we'd already be doing entity queries for this validation and 📌 [PP-1] Evaluate storing XB field type's "deps_*" columns in separate table Active would never have come up as a proposal.

@catch in #7:

1. field plugin moving from one module to another

   Never even thought of this edge case 🤯

2. denormalization means duplication of data, which means bigger databases.

   There's very little duplication here? Only the Component config entity dependency plus identifiers (entity type ID, bundle, ID, revision ID, language).

3. True. But that would be true both with a separate DB table (HEAD) and if it's extra columns in the XB field rows (this issue)?
4. 🤔 I don't understand this one. Can you elaborate?

RE: multiple tables #4.D+E → makes sense, I stand corrected 🙈

it does not seem hugely likely to me that many, many different entity bundles would use the xb field, more like 2-3 usually

AFAICT @lauriii expects/envisions there being no more EntityViewDisplay admin UI anymore, and they'd instead each get an XB ContentTemplate — although many would likely have zero exposed slots, which … means I think you're right! 😄

That on its own changes the equation a lot. 🤔 That means #4.F would never be very high.

@larowlan in #6:

cannot contain dynamic or adapted prop sources,

dynamic only really makes sense in ContentTemplates, but I think @lauriii has expressed he wants to keep this possible in the future
adapted definitely was intended to become supported, both for

• adapting a static prop source (for example a type: integer plus UnixTimestampToDateAdapter to result in a type: string, format: date — see that class in HEAD),
• adapting 2 static prop sources (for example a type: string, $ref: "json-schema-definitions://experience_builder.module/image" plus something like 'type' => 'string', '$ref' => 'json-schema-definitions://experience_builder.module/config-entity-id' to represent an image style config entity, which are then combined in ImageAndStyleAdapter — see that class in HEAD)

  I think because prop sources can have dependencies, we might not be able to do this and may need to retain the database table.

  I don't understand why it would not be technically possible? I expressed in #4 it'd be , but I do think we'd be able. The stored prop sources must be able to be reconstructed, so we should be able to call the methods to compute the dependencies again as you described.

  What am I missing? 🤔

  @larowlan in #5: Nice catch! 😅

Per the above discussion, and having a call with @catch: this is already tagged , but now I'm going to bump this to as well.

Because if we were to do this late(r), this would require a painful update path. So, as @catch wrote at the end of #7: this is kind of going back to 📌 [PP-1] Evaluate storing XB field type's "deps_*" columns in separate table Active , and undoing 📌 [PP-1] Evaluate storing XB field type's "deps_*" columns in separate table Active 😅, but this time there'll be more existing test coverage to make that refactor likely painless 😊 (And let's make sure to add the right DB indexes too.)

Opened a draft MR to show what this might look like.

I think it works fine for 'find which content uses this component' which is the changes to ComponentAudit class (tests are passing here).

I think it falls over for field type uninstall validator with #6

The things that are ugly are:

• config entity queries that know the shape of settings (prop_field_definitions) inside component sources that extend from \Drupal\experience_builder\Plugin\ExperienceBuilder\ComponentSource\GeneratedFieldExplicitInputUxComponentSourceBase, the changes to ::calculateDependencies on that class and its child. And the changes to ComponentFieldItem in that test.

But I think the issue there might be the way we're implementing that uninstall validator - specifically the requirement to have it tell you which fields are being used.

The uninstall validator interface runs off module name (not field name/type) so we can simplify a lot of that code to just look for components that have dependencies on that module _of any type_ not necessarily from a field type POV

If we can accept that simplification and just let it prevent uninstalling the module (which is the root reason) rather than saying which fields are in use - and just say

$reasons[] = new TranslatableMarkup(
          'Is required for component type %component, that is in use in the content of the following entities: %entity_type id=%entity_id revision=%revision_id',
          [
           '%component' => $component_id,
            '%entity_type' => $entity_type,
            '%revision_id' => $revision_id,
            '%entity_id' => $entity_id,
          ],
        );

Rather than the current message

$reasons[] = new TranslatableMarkup(
          'Provides a field type, %used_field, that is in use in the content of the following entities: %entity_type id=%entity_id revision=%revision_id',
          [
            '%used_field' => $field_definition['id'],
            '%entity_type' => $entity_type,
            '%revision_id' => $revision_id,
            '%entity_id' => $entity_id,
          ],
        );

I think that ugliness will go away.

Thoughts?

Edit I also think that lets us get rid of the concept of collecting dependencies of type plugin which core doesn't support. So that would also be a win in my opinion. Unless we want to keep that to allow for the edge case of a plugin going missing, but core doesn't have any handling for that and it could easily happen, I think that scenario we can use the render-safe-container to prevent breakage.

Thanks! Will review 😊

The plugin dependency information is for more than just field types IIRC. It’s also for anything else plugin-based, such as SDCs.

IIRC it is indeed about being able to inform the Drupal user precisely what the problem is: not just “module X” is gone (which might be meaningless), but “plugin X is gone” (which more precisely describes the impact).

But I’ll need to check by reviewing + reading the code.

I definitely agree it’s less than ideal that XB pushes beyond what core offers here 😔 I remember feeling strongly that this is the right call for the end user, but perhaps … enough has changed, or I was wrong! 😇

Have a lovely weekend!

But I think the issue there might be the way we're implementing that uninstall validator - specifically the requirement to have it tell you which fields are being used.

The uninstall validator interface runs off module name (not field name/type) so we can simplify a lot of that code to just look for components that have dependencies on that module _of any type_ not necessarily from a field type POV

This makes sense to me, if you know exactly what you're doing and you really want to uninstall that module, then you need to either find content using the component and delete it (the field delta, or the entire entity if it's test content or similar). Or you'd need to audit the constituent parts (components?) of the component and update it somehow to remove the dependency so it can be uninstalled, which is always going to be a non-trivial developer task.

But the more likely outcome here is 'whoops, better not uninstall that module then, forgot I was using it for this, thanks for telling me'.

IIRC it is indeed about being able to inform the Drupal user precisely what the problem is: not just “module X” is gone (which might be meaningless), but “plugin X is gone” (which more precisely describes the impact).

I'm not sure about this.

The uninstall validator is more about 'you can't uninstall module x yet because your content depends on it', and regardless of what the module is providing, that should prevent 'x is gone' - because it never gets to the point where it's gone.

At the moment it looks like XB is allowing plugins to go missing via Component::onDependencyRemoval (which we briefly discussed yesterday) when modules are uninstalled. I just opened 🐛 Component::onDependencyRemoval() should be an uninstall validator Active for that.

If modules are always prevented from being uninstalled when they provide a component dependency, then the only case a plugin can go missing is when a developer just deletes it. But the whole problem with that is there is no actual event to listen to to run any of this code, it's there one second, then completely missing the next. So to inform the user about that is more of just an error message when they're viewing content, and the plugin ID is there but the plugin itself is not any more, which I think is what 💬 ComponentNotFoundException: Unable to find component Active is for.

But in that case the missing plugin ID must be in the config somewhere, so separate content audit page or similar could do something like go through the config entities, look for plugin IDs that don't correspond to a plugin, and then give you a list of broken components - and then you can use the queries here to find where those components are used.