Problem/Motivation

Working on Display Builder, #3562989: Implements RevisionLogInterface for Instance entity, we noticed some quirks in the soruce field model:

  • FieldItemInterface::propertyDefinitions() is missing, we need to define source_id, source, third_party_settings and node_id
  • SourceValueItem is extending Drupal\Core\Field\Plugin\Field\FieldType\MapItem which doesn't seem to be the good parent, because it imply there is a single value property bagging the source_id, source, third_party_settings and node_id properties
  • source_id must be required

Proposed resolution

Fix those.

The ui_patterns_field module is still experimental, so we it seems we can break the storage here. However, we are committed to storage stability in Display Builder which is heavily using this field. So, lets not break anything and this is the time to also move ui_patterns_field out of the experimental status.

Add some tests?

Issue fork ui_patterns-3584856

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

pdureau created an issue. See original summary.

pdureau’s picture

pdureau commented
Assigned: Unassigned » pdureau

I will give a try.

Would it also be the opportunity to add an index in order to improve performance? Unfortunately, the best candidate, node_id is not mandatory at UI Patterns level.

pdureau’s picture

pdureau commented
Issue summary: View changes
pdureau’s picture

pdureau commented
Assigned: pdureau » Unassigned
Status: Active » Needs review

Let's talk :)

pdureau opened merge request !489

pdureau’s picture

pdureau commented
Assigned: Unassigned » christian.wiedemann
pdureau’s picture

pdureau commented

Is any the most suited data type for storing an associative array?

    $definitions['source'] = DataDefinition::create('any');
    $definitions['third_party_settings'] = DataDefinition::create('any');

Can we set a better type without breaking anything?

pdureau’s picture

pdureau commented
Issue summary: View changes

Let's talk about tests and module status change before merging anything.

pdureau’s picture

pdureau commented
Assigned: christian.wiedemann » pdureau
Status: Needs review » Needs work

1 remaining tasks: Check if moving from any to map is doable by also changing the config schema (now, we have 2 issues: 1. phpunit fails, 2. we may break storage stability)

No need for more tests.

About the status change of the module,, UI Patterns Field will not be experimental anymore. But it will be done in #3548884: [trans] Make SourceValueItem field type translatable with synchronized and asynchron translations

pdureau’s picture

pdureau commented
Assigned: pdureau » christian.wiedemann
Status: Needs work » Needs review

1 remaining tasks: Check if moving from any to map is doable by also changing the config schema (now, we have 2 issues: 1. phpunit fails, 2. we may break storage stability)

It doesn't break the storage: I have built content using SourceValueItem from the 2.0.x and switch to the MR branch, everything still work the same.

phpunit fails were a bit more weird. It was triggered when moving from any to map but were related to $definitions['source_id'] = DataDefinition::create('string')->setRequired(TRUE);:

Drupal\Core\Entity\EntityStorageException: SQLSTATE[23000]: Integrity constraint violation: 1048 Column 'field_ui_patterns_source_1_source_id' cannot be null: INSERT INTO "test93736819node__field_ui_patterns_source_1" ("entity_id", "revision_id", "bundle", "delta", "langcode", "field_ui_patterns_source_1_node_id", "field_ui_patterns_source_1_source_id", "field_ui_patterns_source_1_source", "field_ui_patterns_source_1_third_party_settings") VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1, :db_insert_placeholder_2, :db_insert_placeholder_3, :db_insert_placeholder_4, :db_insert_placeholder_5, :db_insert_placeholder_6, :db_insert_placeholder_7, :db_insert_placeholder_8); Array
(
    [:db_insert_placeholder_0] => 1
    [:db_insert_placeholder_1] => 1
    [:db_insert_placeholder_2] => page
    [:db_insert_placeholder_3] => 0
    [:db_insert_placeholder_4] => en
    [:db_insert_placeholder_5] => 
    [:db_insert_placeholder_6] => 
    [:db_insert_placeholder_7] => a:0:{}
    [:db_insert_placeholder_8] => a:0:{}
)

I removed the setRequired(TRUE). Maybe you will have a better idea.