Implement the new entity field API for the link field type

couldn't finish this patch today, but here's my intermediate results.
added an Unknown type for the serialized link attributes.

basically this is missing a LinkItemTest

Sounds to me like 'Serialized array of attributes for the link.' defines this quite clearly as a (serialized) array. So why Unknown?

Tagging to lure @fago's in here :)

Sounds to me like 'Serialized array of attributes for the link.' defines this quite clearly as a (serialized) array. So why Unknown?

Well, it's unknown what is in there, right? I think having a 'unknown' type is useful for any situation where you prefer to not keep describing your data structure, e.g. think of deeply nested CMI-structures.

However, we could still be more specific, e.g. introduce an attributes type and class which properly implements the ComplexDataInterface at run-time. Still, the property-definitions returned without data would have to be empty.

@fago, I'm trying to build an Attribute typed data, which lives in link module lib directory. As you suggested, a class implementing ComplexDataInterface could be a good idea. Here's what I've figured out, an outline of what the class could look like. Future contrib modules could extend this Attribute class.

// Attribute is complex.
$attribute instanceof ComplexDataInterface;

// property is a list of items.
$attribute->get('class') instanceof ListInterface;

// Item is typed data, representing a value.
$attribute->get('class')->offsetGet(0) instanceof TypedDataInterface;

// Item value is a primitive strings.
is_string($attribute->get('class')->offsetGet(0)->getValue());

My concern is with the child interfaces in the nested cass structure. For instance,

// property is a list of items.
$attribute->get('class') instanceof ListInterface;

Should another new class implement ListInterface to represent $attribute->get('class')? E.g.,

class AttributeProperty implements ListInterface {
}

Because right now, I'm creating a class to represent $attribute, but the following property, item seem to don't have such a proper class.

/**
 * The attribute data type.
 *
 * This data type is used when describe link attributs, e.g., class, style.
 * Each attribute is represented as a property obejct, which is instance of
 * ListInterface. Each item inside attribute is instance of TypedDataInterface.
 * The item value is a primitive string.
 */
class Attribute extends TypedData implements ComplexDataInterface {

  /**
   * All attributes.
   *
   * @var array
   */
  protected $attributes = array();

  // some code.
}

I guess I can't say each property lives only with interface. The property should live with actual class?
If the answer is yes, then this issue seems need more work. I'm not sure I'm on the right way. Any ideas? Thanks:)

Also, I'm thinking the document inside following is confusing.

interface ComplexDataInterface extends Traversable  {

  /**
   * Gets a property object.
   *
   * @param $property_name
   *   The name of the property to get; e.g., 'title' or 'name'.
   *
   * @throws \InvalidArgumentException
   *   If an invalid property name is given.
   *
   * @return \Drupal\Core\TypedData\TypedDataInterface
   *   The property object.
   */
  public function get($property_name);

* @return \Drupal\Core\TypedData\TypedDataInterface
* The property object.

At least for entity property, it's not the case. It returns ListInterface, like

// Properties are not complex, they’re only a list of items.
$entity->get('image') instanceof ListInterface;

At least for entity property, it's not the case. It returns ListInterface, like

No it is the case. The list also implements the TypedDataInterface.

Yep, we'd another another class to represent the list classes. For that we'd need a generic list class, such that we can properly represent a list of strings. Thus, I think we should better take care of #1913328: Provide general list and map classes before this as we can use the generic map class also as base for the attributes class (or just that).

Marking as postponed on #1913328: Provide general list and map classes.

That went in, working on this.

Ok, here is a mostly working patch. Contains working tests, experienced some strange issues there however. will review my own patch in the next comment to point them out.

+++ b/core/modules/link/lib/Drupal/link/Tests/LinkItemTest.phpundefined
@@ -0,0 +1,104 @@
+    // ->attributes returns an array that can not be changed. By design?
+    // set('class', $class) does not work becaue 'class' is not a defined
+    // property yet.
+    $entity->field_test->get('attributes')->setValue(array('class' => $class));
...
+    $entity->field_test->get('attributes')->setValue(array('class' => $new_class));
+    // @todo: Does not work, does not update the value.
+    //$entity->field_test->get('attributes')->set('class', $new_class);

So, the problem is that $field->attributes returns the getValue() representation of the attributes, which is an array that you can't change. So you can only read it from that but not write anything into it.

Second, set('something', $bla) doesn't work on not-yet-existing properies, because it does a get() internally. Also, it currently doesn't seem to work at all, see the second snippet, which does not give me an array but also does not change the value.

+ //$entity->field_test->get('attributes')->set('class', $new_class);

Yes, this works only if there is already a 'class' key in the map. I must say having a map without any metadata is a bit weird and I was not really sure how to implement the class best. So right now it works only from a pre-defined array as good as it can and tries to produce the metadata from there. As you noted, that does not work for creating new items. I agree that it should work that way, so I suppose it should just eat what it gets and live with that.

That said, let's fix the Map class such that set() just creates a new value.

tested manually, the patch for #11 functions correctly

Updated patch that fixes set().

Setting $this->values has two effects, one that I understand and another one that I don't.
a) It makes sure that getPropertyDefinitions() returns it, then get() works.
b) It fixes updates. First I only did that conditionally but then I noticed that the set() on existing values that already have a defined property class in $this->properties. Not sure why this is necessary?

ok, here is a patch for fixing the Map class and adding test-coverage for that. It's not complete yet as the change has still one fail in the map class.

Updated #16 with a correction for the one test-fail.

lets see if this does it.

Generally this looks good to me, however a re-roll is necessary because FieldItemUnitTestBase has become FieldUnitTestBase. And I found some nit-picky stuff too.

+++ b/core/modules/link/lib/Drupal/link/Tests/LinkItemTest.php
@@ -0,0 +1,99 @@
+use Drupal\field\Tests\FieldItemUnitTestBase;
+
+/**
+ * Tests the new entity API for the test field type.
+ */

This should be FieldUnitTestBase now.

+++ b/core/modules/link/lib/Drupal/link/Tests/LinkItemTest.php
@@ -0,0 +1,99 @@
+    // Create an field field and instance for validation.

Create a link field... maybe?

+++ b/core/modules/link/lib/Drupal/link/Tests/LinkItemTest.php
@@ -0,0 +1,99 @@
+  /**
+   * Tests using entity fields of the field field type.

Same here link field I guess.

+++ b/core/modules/link/lib/Drupal/link/Tests/LinkItemTest.php
@@ -0,0 +1,99 @@
+    // Verify entity creation.

How about Create entity.? The verification happens below and has its own comment already.

+++ b/core/modules/link/lib/Drupal/link/Tests/LinkItemTest.php
@@ -0,0 +1,99 @@
+
+    // Verify changing the field value.

Sounds a bit odd, shouldn't it be something like Verify the field value is changed.?

+++ b/core/modules/link/lib/Drupal/link/Type/LinkItem.php
@@ -0,0 +1,47 @@
+/**
+ * @file
+ * Definition of Drupal\link\Type\LinkItem.

Should be Contains \Drupal\link\Type\LinkItem., as far as I know.

+++ b/core/modules/link/lib/Drupal/link/Type/LinkItem.php
@@ -0,0 +1,47 @@
+/**
+ * Defines the 'email_field' entity field item.

'link_field'?

Thanks for the review, re-rolled and fixed those comments I think.

I can't find anything else that needs to be done here -> RTBC

Yep, I agree this is ready.

Committed/pushed to 8.x, thanks!

I think we're down to one of these now.

Congrats! Keep up the great work and thanks.