Provide general list and map classes

Tagging for a bunch of other things.

This looks good^^, here're some little concerns.

+ // Allow getting not yet existing items as well.
+ // @todo: Maybe add a public createItem() method in addition?
+ elseif (!isset($this->list[$offset])) {
+ $this->list[$offset] = $this->createItem($offset);
+ }

I guess a protected createItem(); is ok, since this function is going to interact with $list[$index] only, and that's a protected variable. So, a public createItem(); looks useless to me^

And for public function offsetGet($offset), is there use case to return non-existing items? Maybe it will confuse the user, since offsetExists($offset) returns FALSE, while offsetGet($offset) could return something in this case. Perhaps, add Exists examination, in offsetGet()?

For

- complex data -> "map" ?

'map' could be a little confusion, as this could relate to Google map, or sort of things. How about a simple 'ComplexData':)

Looks like we need extract $entity to a generic complexdata too, and we will need tests for the two generic data type?

hash?

Hash is a way to implement a map, not a separate thing? (HashMap implements Map in Java)

Should the goal of this patch include removing the Mapping and Sequence types in Config schema and making them work with these generic implementations? Those were introduced there due to the missing implementations in Typed Data, and the initiator for this issue was to migrate them to Typed Data :)

Tests fail with exception 'Drupal\Core\Config\StorageException' with message 'Failed to write configuration file: sites/default/files/simpletest/379234/config_active/system.module.yml' in /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Core/Config/FileStorage.php:102 does not seem to be related.

#5: d8_typed_data_structures.patch queued for re-testing.

+  /**
+   * Overrides \Drupal\Core\TypedData\TypedData::getString().
+   */
+  public function getString() {
+    $strings = array();
+    if (isset($this->list)) {
+      foreach ($this->list as $item) {
+        $strings[] = $item->getString();
+      }
+      return implode(', ', array_filter($strings));
+    }
+  }

I'm curious about a few things with this:

1. That TypedData uses getString() rather than __toString(). Is this an oversight?
2. An item that returns a string of '0' would get removed due to array_filter. Should probably use a strlen callback like array_filter($strings, 'drupal_strlen')
3. The separator is hard-coded to the comma and space. Would seem like it should be possible to customize the separator?

+++ b/core/lib/Drupal/Core/TypedData/TypedDataManager.phpundefined
@@ -44,11 +45,24 @@ class TypedDataManager extends PluginManagerBase {
   /**
+   * Process definition callback for the ProcessDecorator.
+   */
+  public function processDefinition(&$definition, $plugin_id) {
+    // Provide a general list class by default.
+    if (!isset($definition['list class'])) {
+      $definition['list class'] = '\Drupal\Core\TypedData\ItemList';
+    }

I think you can define this in $this->defaults and then the default processDefinition() implementation is capable of merging that.

+++ b/core/lib/Drupal/Core/Entity/Field/Type/Field.phpundefined
@@ -27,7 +27,7 @@
-class Field extends ContextAwareTypedData implements IteratorAggregate, FieldInterface {
+class Field extends ItemList implements IteratorAggregate, FieldInterface {

+++ b/core/lib/Drupal/Core/TypedData/ItemList.phpundefined
@@ -0,0 +1,215 @@
+use ArrayIterator;
+use IteratorAggregate;
+use InvalidArgumentException;
...
+class ItemList extends ContextAwareTypedData implements IteratorAggregate, ListInterface {

We're just moving it around but while we do that, we should replace use for global classes with an inline \.

The implements IteratorAggregate can probably be removed now from Field, as it extends from ItemList. Unless you want to keep it for explicitly stating it?

There might als be some now unused use statements in Field.php

Unrelated: TypedData should really really be a DUBT or even plain unit test, the main reason it currently can't be seems to be that it relies on a create helper method that was added to WebTestBase ?!

If it where, then these long test methods could be rewritten into multiple methods so that it's easier to follow/review.

+  /**
+   * Overrides \Drupal\Core\TypedData\TypedData::getString().
+   */
+  public function getString() {
+    $strings = array();
+    if (isset($this->list)) {
+      foreach ($this->list as $item) {
+        $strings[] = $item->getString();
+      }
+      return implode(', ', array_filter($strings, 'strlen'));
+    }
+  }

As stated before I believe this needs to use drupal_strlen() in order to support UTF-8 properly.

I think this is good to go.

The naming makes also sense IMHO, these are standard programming terms, Here's what Wikipedia says about "Map":

An associative array, an abstract data type composed of a collection of keys and a collection of values, where each key is associated with one value

Setting this to major to raise awareness, this is a blocker for converting the link field type which is the last one that is not yet ready and therefore is this also blocking a critical meta issue.

I was going to commit this patch but got an error:

vortex:drupal dries$ git apply ../f.patch --index
error: core/modules/system/system.module: does not match index

The patch may require a reroll. Asking for a retest.

#26: d8_typed_data_structures.patch queued for re-testing.

Worked here, for whatever reason, so!

Committed and pushed to 8.x. Thanks!

This broke Edit.module's "edit/metadata" route.

That route now fatals with a "Unable to get a value with a non-numeric delta in a list", which is in ItemList::offsetGet(). That route results in a call to EditController::metadata(), which does something super simple: iterate over the fields parameter it receives in the request, retrieve the corresponding metadata (this all still works just fine), and then call Drupal\edit\MetadataGenerator::generate(). There is this tidbit:

$items = $entity->get($field_name);
$items = $items[$langcode];

The second line of the two now fails.

Note that this does have test coverage, at Drupal\edit\Tests\MetadataGeneratorTest. However, that works with mock objects, and it seems those mock objects should have been updated by some other changeset, but haven't been? Or… what should I do?

I don't think that's related to this issue. The API didn't change.

What entity type and field type are you using?

Node is now NG so on nodes, you now get a Field class back, which means that you no longer access it with the langcode, it automatically uses the default. Instead, you have a Field class and can do something like $node->field->value or $node->field[0]->value or, in case of the EntityBCDecorator (which still returns the old structure back on $node->field) $node->get('field')->value.

You're probably the only one or almost only one that uses ->get() so it continues to work for everyone else thanks to the BCDecorator.

Ping me in IRC and I can help to sort this out.

Closing this one again: #1942000: Node NG broke Edit module

Yes, thanks, and sorry for not doing so myself.

Opened #1949916: Make use of general, any, list & map classes for configuration schema as a followup!