This came up during #1905230: Improve the typed data API usage of configuration schema and some other places, so let's do that.

We also need a generic item lists for handling multiple-valued contexts of #1743686: Condition Plugin System, or generic map to handle link field attributes.

Suggested names:
- list -> list. However the classname "List" is reserved, so I have used ItemList for now.
- complex data -> "map" ?

Attached is a first patch which starts extracting code from the Field class and moves it to an item list class.

Files: 
CommentFileSizeAuthor
#26 d8_typed_data_structures.patch30.5 KBfago
PASSED: [[SimpleTest]]: [MySQL] 52,604 pass(es). View
#26 d8_typed_data_structures.interdiff.txt1.28 KBfago
#24 d8_typed_data_structures.patch30.37 KBfago
PASSED: [[SimpleTest]]: [MySQL] 52,213 pass(es). View
#24 d8_typed_data_structures.interdiff.txt1.11 KBfago
#23 d8_typed_data_structures.patch30.41 KBfago
PASSED: [[SimpleTest]]: [MySQL] 52,044 pass(es). View
#23 d8_typed_data_structures.interdiff.txt1.05 KBfago
#22 d8_typed_data_structures.patch30.45 KBfago
PASSED: [[SimpleTest]]: [MySQL] 52,049 pass(es). View
#22 d8_typed_data_structures.interdiff.txt3.54 KBfago
#20 d8_typed_data_structures.interdiff.txt1.8 KBfago
#20 d8_typed_data_structures.patch29.9 KBfago
FAILED: [[SimpleTest]]: [MySQL] 52,083 pass(es), 0 fail(s), and 1 exception(s). View
#19 d8_typed_data_structures.patch28.73 KBfago
FAILED: [[SimpleTest]]: [MySQL] 47,860 pass(es), 112 fail(s), and 87 exception(s). View
#18 d8_typed_data_structures.patch30.12 KBfago
FAILED: [[SimpleTest]]: [MySQL] 52,100 pass(es), 1 fail(s), and 0 exception(s). View
#18 d8_typed_data_structures.interdiff.txt1.21 KBfago
#16 d8_typed_data_structures.patch28.7 KBfago
PASSED: [[SimpleTest]]: [MySQL] 52,292 pass(es). View
#15 d8_typed_data_structures.patch29.44 KBfago
FAILED: [[SimpleTest]]: [MySQL] 46,211 pass(es), 632 fail(s), and 211 exception(s). View
#5 d8_typed_data_structures.patch22.01 KBfago
FAILED: [[SimpleTest]]: [MySQL] 51,283 pass(es), 1 fail(s), and 0 exception(s). View
d8_typed_data_structures.patch14 KBfago
PASSED: [[SimpleTest]]: [MySQL] 50,321 pass(es). View

Comments

Gábor Hojtsy’s picture

Status: Needs work » Needs review
Issue tags: +Configuration system, +D8MI, +language-config

Tagging for a bunch of other things.

smiletrl’s picture

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?

fago’s picture

So, a public createItem(); looks useless to me^

Well, without that you cannot create list item and then know it's delta what kind off sucks.

And for public function offsetGet($offset), is there use case to return non-existing items?

The regular use case for this is trying to access the first field item via $node->field->value what runs through $node->field[0]->value. Also, I think it makes the API easier to use as you do not have to check for unset objects before moving on - as it is the case for retrieved properties as well. Indeed our __isset() implementation there just checks for non-NULL values, what we should do here as well to be consistent.

fago’s picture

So you'd suggest type "complex" and class name ComplexData ?

I'm not sure - "map" seems to be a common term for that, e.g. http://en.wikipedia.org/wiki/Map_%28disambiguation%29. Whereas complex vs simple types is usually more a categorization of data types only.

fago’s picture

FileSize
22.01 KB
FAILED: [[SimpleTest]]: [MySQL] 51,283 pass(es), 1 fail(s), and 0 exception(s). View

ok, worked a bit more on this and added "any" and "map" data types as well. All untested and needs tests cases.

chx’s picture

hash?

Berdir’s picture

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

Status: Needs review » Needs work

The last submitted patch, d8_typed_data_structures.patch, failed testing.

Gábor Hojtsy’s picture

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 :)

Gábor Hojtsy’s picture

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.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
Issue tags: -Configuration system, -D8MI, -language-config, -typed data

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

Status: Needs review » Needs work
Issue tags: +Configuration system, +D8MI, +language-config, +typed data

The last submitted patch, d8_typed_data_structures.patch, failed testing.

Dave Reid’s picture

+  /**
+   * 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?
fago’s picture

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 :)

Yes, but this issue is needed for some other stuff as well, e.g. #1839080: Implement the new entity field API for the link field type or for the conditions API to support lists of strings. So I think we should only bake this clean-up into this patch if it's do-able rather quick, but postpone to a follow-up if not.

That TypedData uses getString() rather than __toString(). Is this an oversight?

Well, using it when casting to strings was not the purpose of the method, but having an easy, explicit way to get a string representation of the data. I can see using __toString() has some advantages anyway though. This needs a change in the interface and some classes, so it deserves its own issue.

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')

Good suggestion, thanks!

The separator is hard-coded to the comma and space. Would seem like it should be possible to customize the separator?

Well, this is just a simple way to get a string representation - it does not fit for generating customizable string tokens. I'd be happy to discuss how we could leverage typed data for token generation best though. Maybe, getString() could be extended to support multiple token formats - but I'm not sure formatting should be part of the core typed data API, I'd rather have a separate system relying on typed data for that? Maybe, we could just rely on the token API for the formatting part per data type, while the chaining part could be handled by the typed data API? Let's discuss this more over at #1869582: Leverage the Typed Data API for token replacements

fago’s picture

Status: Needs work » Needs review
FileSize
29.44 KB
FAILED: [[SimpleTest]]: [MySQL] 46,211 pass(es), 632 fail(s), and 211 exception(s). View

ok, I took a stab on that and completed the data types + added tests covering them (independently from existing entity field stuff).

I've not look into using this for config-schema stuff yet, else this should be complete. Please review!

fago’s picture

FileSize
28.7 KB
PASSED: [[SimpleTest]]: [MySQL] 52,292 pass(es). View

Ops, I realized I forgot to take care of handling the special case of non-list values for entity field values again. Updated patch.

Berdir’s picture

+++ 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.

fago’s picture

FileSize
1.21 KB
30.12 KB
FAILED: [[SimpleTest]]: [MySQL] 52,100 pass(es), 1 fail(s), and 0 exception(s). View

Indeed. Typed definitions get cached so no need for micro-optimizing here - fixed that.

I also tried to see whether we can use the new classes for the config schema, but I've quickly seen that's not a simple undertacking but requires considerable more effort. Thus opened #1928868: Typed config incorrectly implements Typed Data interfaces for that.

fago’s picture

FileSize
28.73 KB
FAILED: [[SimpleTest]]: [MySQL] 47,860 pass(es), 112 fail(s), and 87 exception(s). View

Oh, above patch also had the start of #1928868: Typed config incorrectly implements Typed Data interfaces baked in - re-uploading the patch without that. The interdiff is correct.

fago’s picture

FileSize
29.9 KB
FAILED: [[SimpleTest]]: [MySQL] 52,083 pass(es), 0 fail(s), and 1 exception(s). View
1.8 KB

Noted the dedicated tests for the 'any' type were missing, added them.

Berdir’s picture

+++ 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.

fago’s picture

FileSize
3.54 KB
30.45 KB
PASSED: [[SimpleTest]]: [MySQL] 52,049 pass(es). View

Thanks for the review. Attached patch takes care of the use statements and fixes the code to use \Exception style.

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 ?!

Totally, yep. I guess that instead of that helper we should have a TypedDataUnitTestBase or so. But let's clean that up in another issue.

fago’s picture

FileSize
1.05 KB
30.41 KB
PASSED: [[SimpleTest]]: [MySQL] 52,044 pass(es). View

berdir pointed out on IRC that all use statements of global classes should not be used, not only exceptions. Thus re-rolled the patch accordingly.

fago’s picture

FileSize
1.11 KB
30.37 KB
PASSED: [[SimpleTest]]: [MySQL] 52,213 pass(es). View

But then, the same applies to maps - thus updated the patch to take care of that also.

Dave Reid’s picture

+  /**
+   * 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.

fago’s picture

FileSize
1.28 KB
30.5 KB
PASSED: [[SimpleTest]]: [MySQL] 52,604 pass(es). View

As discussed on IRC, changed it to use drupal_strlen() as we've Drupal-specific code below Core and thus should follow best-practices.

Updated patch attached.

Berdir’s picture

Priority: Normal » Major
Status: Needs review » Reviewed & tested by the community

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.

Dries’s picture

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.

Dries’s picture

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

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Worked here, for whatever reason, so!

Committed and pushed to 8.x. Thanks!

Wim Leers’s picture

Status: Fixed » Active

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?

Berdir’s picture

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.

Berdir’s picture

Status: Active » Fixed

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

Wim Leers’s picture

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

Gábor Hojtsy’s picture

Automatically closed -- issue fixed for 2 weeks with no activity.