MapItem is broken for configurable fields

This was introduced in #2021779: Decouple shortcuts from menu links but got shuffled around in #2235457: Use link field for shortcut entity - I guess it's kind of useful, although it doesn't have the best name either. Removing sounds like an API change, you never know if someone already uses it or not of course.

Hm, really we cannot remove it? maybe we can just mark it as Deprecated, since it is unused in Core? It seems highly unlikely that someone is using it though...

I pinged @catch in IRC:

(06:37:03 AM)catch: jhodgdon: dead code can be removed.
(06:37:21 AM) jhodgdon: catch: can I quote you on that? :)
(06:38:21 AM) catch: jhodgdon: hah. Yes I think that's fine for now.
(06:38:31 AM) catch: jhodgdon: if we keep it, we not only have to document it but also test it.
(06:38:40 AM) catch: jhodgdon: if it turns out someone is using it, we could put it back.
(06:38:46 AM) catch: jhodgdon: but I'd remove + change record just in case.

So on that basis, tentatively marking the patch in #2 RTBC. We need a change record, which I will write shortly.

This could be added back by contrib, if contrib wants to store arbitrary associative arrays as field data on entities.

+1 for removing dead code.

Fix issue title and summary slightly

Change record is also up.
https://www.drupal.org/node/2577605

Some recommendations for the use case of storing array maps:

1. Think about using config entity to store the array map instead of storing as a serialized field.
2. Think about using an alternate storage method from contributed modules such as JSON/JSONB Field.
3. Re-implement the field type as a contributed module.

added notes about how dead this is to summary.

Not documentation.

Oh yes.

How on earth could that fail

Seems that catch signed off on this pending a change record, which does exist, so...

Committed and pushed to 8.0.x. Thanks!

published the change record. Hooray!

Hm. I was using this in https://www.drupal.org/project/tmgmt. Not sure if just core not using something qualifies as dead code that can be removed? And there is no required to have formatters and widgets to use a field type, you can just use it as storage.

Same for http://drupal.org/project/mailmute

Both projects are completely broken right now.

The core class wasn't used, documented, or tested... the thought was that it could be taken over by contrib if needed. If we put it back in core (or don't remove it), it needs both tests and documentation. It had neither.

I'm aware of that. I can also live with having to add that to TMGMT. It just seems like a strange decision at this point of the cycle, but we're apparently back to making all kinds of changes again :)

Also, from your own comment.

(06:38:40 AM) catch: jhodgdon: if it turns out someone is using it, we could put it back.

So, basically, I just wanted say, that there is at least one project using it. Now it's catch's call ;)

It's not like this is the only piece of code in D8 that doesn't have test coverage.

Damn I was using this in 2 modules :(

Made a short list of contrib modules using `map` as base fields who are inconvenienced by this change:

• Commerce: LineItem, Order, PaymentInfo, PaymentTransaction entities.
• TMGMT: RemoteMapping entity
• Ubercart: CartItem, Order, OrderProduct entities
• RNG: RuleComponent entity
• Courier: MessageQueueItem entity

I'm with #24, its an odd time to remove code. Seems like tests should be added instead of removal. It looks like the field is pretty useful to contrib.

Reverted this, then. Though that means we ought to document and test it. ;)

Updated the title and IS for changed scoped of the issue also unpublished the change record.

Thanks. Testing agreed, but not exactly sure what you want to document? This is an API, it doesn't need user documentation. It's the equivalent to 'serialized' => TRUE in D7.

As noted in the original issue report, the class header needs a documentation header update.

And this isn't an "API" exactly -- it's a field type.

Since we are now keeping MapItem I feel pretty strongly that #2229181: MapItem::propertyDefinitions() claims it has a static value property, which is wrong should be rolled back. Interacting with a map field on the API level might work currently, but I recently experimented with adding a widget for it and that quickly reveals that that issue actually introduced a bug. Every database column in a field's schema must correspond to a property and MapItem violates that now. We even document this behavior explicitly, because we were already bitten by it when we started with all the schema auto-generation fun. See FieldItemInterface::schema():

  /**
   * ...
   *
   * @return array
   *   An empty array if there is no schema, or an associative array with the
   *   following key/value pairs:
   *   - columns: An array of Schema API column specifications, keyed by column
   *     name. The columns need to be a subset of the properties defined in
   *     propertyDefinitions().
   * ...
   */

Since re-introducing that property would need some test coverage anyway I think this would be a good issue to effectively "revert" #2229181: MapItem::propertyDefinitions() claims it has a static value property, which is wrong. An actual revert is not of much value in this case IMO (because it was such a small commit) and due to the IMO needed test coverage is not feasible anyway. Also we might consider keeping the toArray() override (but not sure about that).

Not re-titling yet to get some feedback.

Btw, here's the relevant commit, so people don't have to go and look through that issue: http://cgit.drupalcode.org/drupal/commit/?id=78594ab

When writing a test for this, it might be interesting to see what happens exactly. I just started using this field and it seems the result of getValue() is wrapped in an additional array somehow. (Which I suppose it undesired, but fixing it would result in an API change for the modules in #26.)

getValue() of what exactly? Like with any field type, a field is always a list of field items, so $entity->get('field')->getValue() always returns a list of values.

@Berdir, in a custom entity it returns my set value wrapped in an additional array.

Relevant entity definition:

  /**
   * {@inheritdoc}
   */
  public function getMapItems() {
    return $this->get('map_field')->getValue();
  }

  /**
   * {@inheritdoc}
   */
  public function setMapItems(array $items) {
    $this->set('map_field', $items);
    return $this;
  }

  /**
   * {@inheritdoc}
   */
  public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
    $fields = parent::baseFieldDefinitions($entity_type);

    $fields['map_field'] = BaseFieldDefinition::create('map')
      ->setLabel(t('Map field'))
      ->setDescription(t('Map field'))
      ->setQueryable(FALSE)
      ->setRevisionable(TRUE);

    return $fields;
  }

Using this getter and setter does the following:

$array = [
  'map' => [
    'value1',
  ],
];

$entity = MyEntity::create([]);
$entity->setMapItems($array);
$entity->save();
dpm($entity->getMapItems());

Returns:

Array
(
    [0] => Array
        (
            [map] => Array
                (
                    [0] => value1
                )

        )

)

Moved #37 into #2887105: MapItem violates FieldItemInterface::schema()'s contract - breaks widgets

So like only the documentation part is left ryt? Or are more changes needed to be made?

This not being solved has big consequences until this day: #2895532-124: @DataType=map cannot be normalized, affects @FieldType=link, @FieldType=map.

It'd have been much better for Drupal 8 if this would've stayed reverted IMHO. The few modules that did use this could've just copy/pasted the existing implementation and made it their own.

It'd have been much better for Drupal 8 if this would've stayed reverted IMHO. The few modules that did use this could've just copy/pasted the existing implementation and made it their own.

No they couldn't really, not the way it works in core, and the remaining bugs in it are also in core.

As we discussed over there, the changes you made are IMHO a workaround due to those bugs.

Answering your comment from over there:

AFAICT there are two possible explanations/solutions:

1. The API is incomplete/a subset of what it needs to be, because the current required matching between propertyDefinitions() + schema() is getting in the way: it prevents us from expressing what we need to express.
2. The API is fine, but both the docs and \Drupal\Core\Entity\Sql\SqlContentEntityStorageSchema::getDedicatedTableSchema()are incorrect.

I could see it being solved either way (or perhaps in other ways I don't yet see!), but IDK which is the correct approach — that requires a more complete understanding of Entity/Field/Typed Data API than I have! Looking forward to your proposal! :)

in my opinion, this has a clear answer: 1.

Because as you said yourself, it works fine with base fields, it's just configurable fields where it doesn't. And that kind of inconsistency in the entity storage is a bug. Base fields and configurable fields (and multi-cardinality base fields) should work transparently. Interestingly, we have a bug about an inconsistency in the other direction in the exact same place. unserialize on load only works for configurable fields: #2788637: Values in shared table for SQL content entity storage do not get unserialized.

This adds the same special cases that we have for shared table fields to the logic for dedicated tables (including the same @todo's to the issue that is supposed to improve it and give field types more control) and adds a MapItemTest that tests basic storing and loading of map fields.

Cool! Thanks for the explanation and the patch, @Berdir!

This will definitely need review from Entity/Field API maintainers. I can't review the functional changes, but the test definitely looks good, so removing Needs tests. I do think it'd be good to store a nested array in one of the test examples though. The patch is also referencing #2232427: Allow field types to control how properties are mapped to and from storage, so adding that as a related issue. Your comment is mentioning #2788637: Values in shared table for SQL content entity storage do not get unserialized., so adding that also as a related issue.

Agreed, makes sense to store a more complex data structure. Doesn't matter for the current implementation (we're basically just testing serialize()), but who knows what we'll do in the future with this stuff.

And yes, we have too many issues :)

Added that.

👍 No more remarks from me!

The patch looks great to me as well, I just found a few nitpicks:

1. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php
   @@ -1315,8 +1326,17 @@ protected function saveToDedicatedTables(ContentEntityInterface $entity, $update
   +            // properties from and assume that they will be stored serialized.
   

   "properties from ...?" -> the suspense is killing me :D

2. +++ b/core/modules/field/tests/src/Kernel/MapItemTest.php
   @@ -0,0 +1,69 @@
   +  protected function setUp() {
   

   Missing {@inheritdoc}.

3. +++ b/core/modules/field/tests/src/Kernel/MapItemTest.php
   @@ -0,0 +1,69 @@
   +   * Tests using entity fields of the field field type.
   

   ... of the map field type? :)

What, Wim, the champion of nitpicks missed some? How is that possible ;)

Thanks for the review. I really didn't like the "assume" part in my comment there actually, so I changed the logic a bit so that we explicitly check that it is indeed serialized.

That looks even better. I would RTBC this but I think @tstoeckler wants to take a look as well, so holding off for a bit.

Took a look at this and some of the related issues. It's quite hard to decipher not only what is going on but also what everyone is thinking due to all the different issues plus the ones that we're committed/reverted/etc. I'm a bit short on time so this will be more of a ramble than I'd like.

As a sort of "ceterum censeo"-style disclaimer (which basically applies to anything I say about the Entity API): Since the Entity API is weird, full of quirks, bugs and worse things with or without this patch going in, my dissent should not hold up a commit if there's significant consensus otherwise. I'll try to express my reasoning as best I can, but I'm not sure when I will be able to get back to this due to time constraints, so at the end of the day I will be able to continue with life and the pursuit of happiness regardless what happens here.

OK, here we go.
So the problem with MapItem was that it was thought out to be able to have a dynamic set of properties which all get serialized into a single value database column. But there is not actually a value property, but the various values get set on the field item directly, as though they were actual properties (or, arguably, they are actual properties). While all that sounds nice, this is a clear violation of our API as I already pointed out in #2887105: MapItem violates FieldItemInterface::schema()'s contract - breaks widgets. The API clearly states that the schema columns must be a subset of the property names. The fact that this leads to a fatal in ::getDedicatedTableSchema() is just a symptom of that bug.

This issue now aims to resolve this problem in the opposite direction of #2887105: MapItem violates FieldItemInterface::schema()'s contract - breaks widgets: by removing this restriction from the API. That is why I think the hunk in ::getDedicatedTableSchema() is problematic. We don't really know where else in core and contrib we are relying on this currently valid assumption which we are now making invalid. On a conceptual level I also disagree with removing this restriction. We have gone to great lengths to making our Entity API as not-SQL-specific as possible FieldItemInterface::schema() is one of the biggest (if unfortunately not last) remnants of the SQL-coupling. But because the schema data must be a subset of the property data currently you can see the schema as an SQL-specific extension of the property metadata - which is actually semantically precisely what it is. The fact that we introduced schema at the field (i.e. composite) level instead of the property (i.e. primitive) level was a mistake, but with this change we are further engraving that choice instead of making it less pronounced.

The other half of the issue is that the non-existance (or transparency) of the value (non-)property actually doesn't work at the moment. The test added here shows the intended usage of a map field. What is not clear to me, however, is why it is of such ample importance for map fields to work in that specific way? I.e. while $entity->field_test->value['foo'] certainly doesn't look as elegant as $entity->field_test->foo is it really reason enough to question and change/remove APIs? Is there anything that is actually broken as in "it can't be used" (not "it can't be used as expected", because as seen above the expectation of MapItem is very questionable itself).

The current patch while "disguising" as generic code really bakes the assumption of MapItem very deeply into the storage layer. I.e. the checking !$storage_definition->getMainPropertyName() && count($storage_definition->getColumns()) == 1 && !empty($attributes['serialize']) is only ever-so-barely more elegant than checking whether we are dealing with a field of type map. And I would really strongly advise against baking such specific logic into our storage layer.

So yeah, prettty strongly -1 against this. Also marking Needs issue summary update that really is not at all representative of the latest patches.

Thanks for your feedback.

I think I understand your concern and agree at least partially with it :)

Completely deprecating schema() in favor of per-property schema is an interesting thought, would require a lot more settings though on those primitive types that would again be SQL-centric. Luckily, it is an schema array, not SQL, so it's not that complicated to convert and in the end doesn't make a big difference if the definition is an array or an object.

However, IMHO, the situation is like this:

* The field type map exists
* It works as this test uses it when used as a base (shared table) field
* It is completely broken when used as a configurable (dedicated table, so also a multi-value base field technically) field.

In my opinion, we are not making a *change* here and we're not introducing anything new. We just add support for for what we already support for shared table fields to dedicated table fields. Changing how MapItem works is not an option in 8.x, that would break all entity types using it as a base field (TMGMT, Commerce, ...).

The only alternative to this patch is IMHO deprecating MapItem entirely and throwing an exception when used as a configuable field. I thought also about doing this as a bugfix and then discussing the deprecation in #2887105: MapItem violates FieldItemInterface::schema()'s contract - breaks widgets. But I'm only doing this issue so I can use it in #2895532: @DataType=map cannot be normalized, affects @FieldType=link, @FieldType=map and that's weird if we're adding new usages knowing that we might deprecate it soon. The other issue is not just about the field type level but also properties like options, and as long as we have MapItem, we also want to support it in serialization I think? Otherwise we would need to explicitly exclude it.

Thoughts?

Ahh OK, wasn't aware that that syntax works for base fields, I thought it was just ::getDedicatedTableSchema() breaking configurable fields completely. That does change things, but will have to check the code out that does that for base fields before commenting any further.

Thanks for the quick feedback.

BTW: Conceptually I do think ::schema() should die on field items but a) that's a super super long-term goal and b) even in the long-term I'm not confident we can pull it off with BC. So let's not discuss that here. ;-)

Yes, the logic of those checks is basically the same as we already have for base fields. That is why contrib modules are using map fields already right now.

See \Drupal\Core\Entity\Sql\SqlContentEntityStorage::mapToStorageRecord() for the converting from the field to the raw values for storing, loading currently works automatically/implicitly because by default there is only one property and then it's just assigned to the field value which then takes care of converting it through the FieldItem/FieldItemList setValue() magic. See \Drupal\Core\Entity\Sql\SqlContentEntityStorage::loadFromSharedTables(). What we are missing *there* in contrast to this patch is support for automatic unserialize(), which is why we have fallbacks to unserialize() in setValue() of certain field types: #2788637: Values in shared table for SQL content entity storage do not get unserialized..

I'm available in slack/IRC if you have a few minutes to discuss.

It works as this test uses it when used as a base (shared table) field

Do we have test coverage for that? If not, can we add it? Also, this issue is currently categorized as a task. I think we should either:

• keep it that way and only commit test coverage for what currently works (the shared table case) and open a separate bug report issue for the dedicated table case.
• or open a child issue for the shared table case test coverage, and then re-categorize this issue as a bug report.

By the way, I mostly agree with #62, and think that it's an unfortunate bug that what is currently documented as the requirement of schema() (that it only contain columns that are a subset of defined properties) is not being enforced with an exception. But given that the map field type exists and works (without exceptions/errors) when used as a single-item base field, I don't have an opinion yet on the best way forward.

OK, looked at mapToStorageRecord and the patch and played around with it for a bit.

Some thoughts:

1. You're right that mapToStorageRecord contains similar code. That was introduced all the way back when we started auto-generating entity schemas so there's a good chance I am the one to blame for that. In any case, I agree that if we do that for base fields we must do the same for configurable fields.
2. After having thought about this for a while, I'm fine with keeping the hunk in SqlContentEntityStorageSchema as long as we add a @todo pointing to #2887105: MapItem violates FieldItemInterface::schema()'s contract - breaks widgets. That way that part is expicitly scoped as a bug fix and not a changing/loosening of the API. We can then discuss over there if we do actually want to change the API or not.
3. To make sure this does work with base fields I ran Commerce's OrderItemTest (and also added an interim save-load step there). It does in fact work, but I agree with #66 that we should add a test for that in core.
4. When playing around with this in the aforementioned test I found that this patch actually introduces an inconsistency between base and configurable fields. For base fields, the serialized value string ends up in ContentEntityBase::$values after loading. Then, when a map field item is actually accessed, it will be unserialized by MapItem::setValue(). That's how it should work for configurable fields, as well. What may (or may not) be related to this is that for base fields we only do this handling in mapToStorageRecord() (and not in mapFromStorageRecord() but this patch introduces hunks in both saveToDedicatedTables() and loadFromDedicatedTables()
5. One minor note after looking at how mapToStrageRecord implements the similar logic: I think the patch could avoid the repeated checks of the 'serialize' attribute and the repeated (un)serialize() calls by using some local variables or something.

Marking needs work for issue summary update and the tests requested in #66.

#60: What, Wim, the champion of nitpicks missed some? How is that possible ;) 😳😔

#62+#63+#64+#65+ = 🙏👌 and 😳😯😮😵

To get this going again, I'm just applying @tstoeckler's requested changes in #68 to @Berdir's latest patch in #60:

1. Ok.
2. ✔️ Done.
3. ✔️ I wanted to write the test, but this turned out to already exist! #68.3 and #66 are asking for it, because #63 claims it already works for @FieldType=map base fields on entity types. "it" here is the ability to do $entity->field_test->foo instead of $entity->field_test->value['foo'].
   \Drupal\Tests\link\Kernel\LinkItemTest::testLinkItem() already tests this: it has $this->assertEqual($entity->field_test->options['attributes']['class'], $class);, not $this->assertEqual($entity->field_test->options['value']['attributes']['class'], $class);.
   Oh, but that's @DataType=map, not @FieldType=map. We need an entity type with BaseFieldDefinition::create('map'). That does not yet exist in core. So, created a test entity type (modeled after \Drupal\entity_test\Entity\EntityTestComputedField) plus test coverage (by applying the assertions already in the patch to a @FieldType=map base field as well).
4. That sounds like a 100% internal detail with no consequences for any user of the Entity API? I made no changes for this.
5. ✔️ Done — or at least, I did half of what you asked for, the other half would've made the code much harder to understand IMHO.

+++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php
@@ -1165,8 +1165,19 @@ protected function loadFromDedicatedTables(array &$values, $load_from_revision)
+              $column_is_serialized = !empty($attributes['serialize']);
+              // @todo Give field types more control over this behavior in
+              //   https://www.drupal.org/node/2232427.

I obviously should've put that $column_is … line below the comment 😅

But that's an easy thing to fix during commit, plus it's not actually important.

This blocks #2895532: @DataType=map cannot be normalized, affects @FieldType=link, @FieldType=map, which is major. Bumping this to major.

That issue now works around it and doesn't depend on it anymore, so not sure about it being major.

Will leave this to @tstoeckler as he was the one with concerns about this.

This doesn't hard-block the other issue, but the other issue is still reusing a key portion of the test coverage added by this patch. So if this lands, the other patch becomes simpler and hence less contentious.

So I could argue either way — I'm fine with both Major and Normal.

The code itself looks good to me, but

That sounds like a 100% internal detail with no consequences for any user of the Entity API? I made no changes for this.

is not very convincing to me. First of all we would need to be extra super sure that this does actually not have any consequences before this can go in. But even on pure DX grounds I would oppose this going in with this inconsistency. The Entity Field API is incredibly complex and it is very common to simply look at the values in memory to see what is going on, so I'm not at all convinces that we can get away with inconsistencies between base and configurable fields, even if it doesn't turn out to affect the API.

Also this is still tagged "Needs Documentation" and "Needs issue summary update" so I don't think it can be RTBC on that ground either.

Isn't 4. what #2788637: Values in shared table for SQL content entity storage do not get unserialized. is about? That's an existing, known issue and that field types have to do that is a bug and please please help me fix that there :)

Hmmm... I guess that's true, yes. Alright, fine, then let's get this in. Still needs work for the documentation and IS updates, though.

#2895532: @DataType=map cannot be normalized, affects @FieldType=link, @FieldType=map landed! Rebased this. Because

+++ b/core/modules/system/tests/modules/entity_test/src/Entity/EntityTestMapField.php
@@ -0,0 +1,37 @@
+class EntityTestMapField extends EntityTest {

Already exists now!

> Add tests and documentation for MapItem FieldType

The current patch is doing more than adding tests and documentation! Looks like this issue needs rescoping.

@joachim++

Re-titled and updated the issue summary. Removing the "Needs documentation update" tag and created #3117327: Add documentation for MapItem for that so this can be scoped only to the concrete bug fix.

Sent for a re-test, will RTBC if that still passes as per #77 / #79 the patch itself was fine and already reviewed and discussed at length.

Git was able to rebase. I ran MapItemTest locally and it still passed, so setting to RTBC and crossing my fingers...

Great!

Checked that MapItemTest was run: https://dispatcher.drupalci.org/job/drupal_patches/35539/testReport/Fiel...

The patch is failing on 9.1.x with:

Field.Drupal\Tests\field\Kernel\MapItemTest
✗	
Drupal\Tests\field\Kernel\MapItemTest
fail: [Other] Line 0 of sites/default/files/simpletest/phpunit-369.xml:
PHPunit Test failed to complete; Error: PHPUnit 8.5.3 by Sebastian Bergmann and contributors.

Testing Drupal\Tests\field\Kernel\MapItemTest
..                                                                  2 / 2 (100%)

Time: 3.25 seconds, Memory: 4.00 MB

OK (2 tests, 20 assertions)

Remaining self deprecation notices (2)

  2x: Declaring ::setUp without a void return typehint in Drupal\Tests\field\Kernel\MapItemTest is deprecated in drupal:9.0.0. Typehinting will be required before drupal:10.0.0. See https://www.drupal.org/node/3114724
    2x in DrupalListener::startTest from Drupal\Tests\Listeners

So looks like we need to create a 9.x version of the patch. Thanks!

Back to RTBC as the fix is stright forward.

As much as I'd like to get this committed, @alexpott filled a "counter-bug" for #3130869: Single value serialized base fields are broken so I suspect this change would break his use case for configurable fields. We might want to hold back from committing this until we somehow support both use cases for both shared and dedicated storage, might want to combine that into a single issue to ensure that.

I might be wrong, but as far as I can tell, this patch only changes (broken) behavior for fields without a main property, while the problem reported in #3130869: Single value serialized base fields are broken is with a field that does have a main property. So while it's certainly related and it might end up changing the same lines touched here again, I think it should be safe to commit this and then tackle that separately.

Yep I agree with @Berdir we need to be careful here or we'll break a configurable field which has a field type like:

  /**
   * {@inheritdoc}
   */
  public static function propertyDefinitions(FieldStorageDefinitionInterface $field_definition) {
    $properties['map'] = MapDataDefinition::create()
      ->setLabel(t('Map'));

    return $properties;
  }

  /**
   * {@inheritdoc}
   */
  public static function schema(FieldStorageDefinitionInterface $field_definition) {
    $columns = [
      'map' => [
        'type' => 'blob',
        'size' => 'normal',
        'serialize' => TRUE,
      ],
    ];

    $schema = [
      'columns' => $columns,
      // Add indexes here if necessary.
    ];

    return $schema;
  }

  /**
   * {@inheritdoc}
   */
  public static function mainPropertyName() {
    return 'map';
  }

the above field works as a configurable field BUT not as a base field :) - fun times.

@tstoeckler yeah well I think we need to prove that the changes here don't break that. I think that the mainPropertyName method is not actually necessary for such field to work as a configurable field.

I think that the mainPropertyName method is not actually necessary for such field to work as a configurable field.

Well per this issue without the main property name the field will be considered equivalent to MapItem, i.e. it will be unserialized not into the "map" property, but onto the item itself.

Re-rolling for 8.9.x

Rerolled for 9.5

Missing :void

This is the same as #101 but with a tests-only patch.

This could use the issue summary update to explain the workaround here, what exactly is split into the spin-off issue etc. Code looks OK but it's hard to review what it's actually doing without the wider context.