Improved EntityNG isset() test coverage

Looks like this got already committed as part of #1826688: REST module: PATCH/update - so we need to address any issues as follow-up.

+++ b/core/lib/Drupal/Core/Entity/Field/Type/Field.php
@@ -51,6 +51,13 @@ class Field extends TypedData implements IteratorAggregate, FieldInterface {
   /**
+   * Flag to indicate if this field has been set.
+   *
+   * @var bool
+   */
+  protected $isset = FALSE;
+

Why do we need this flag? I think we should follow PHP and say isset if $value !== NULL. Let's avoid re-inventing things. Also, every additional object property raises memory usage!

+++ b/core/lib/Drupal/Core/Entity/Field/Type/Field.php
@@ -101,6 +109,14 @@ public function setValue($values) {
   /**
+   * Mark this field as not set.
+   */
+  public function unsetValue() {
+    $this->list = array();
+    $this->isset = FALSE;
+  }

Similarly, do we need separate set/get methods? Imho it should suffice to set NULL to unset it, while setting it to array() what make it empty but being set. -> That follows existing PHP semantics and thus should be what developers expect.

+++ b/core/lib/Drupal/Core/Entity/Field/Type/Field.php
@@ -285,6 +302,15 @@ public function isEmpty() {
+  public function valueIsSet() {
+    return $this->isset;
+  }

ditto.

+++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityFieldTest.php
@@ -128,6 +128,12 @@ public function testReadWrite() {
+    $entity->name = array();
+    $this->assertTrue(isset($entity->name), 'Name field is set.');
+    $this->assertFalse(isset($entity->name[0]), 'Name field item is not set.');
+    $this->assertFalse(isset($entity->name[0]->value), 'First name item value is not set.');
+    $this->assertFalse(isset($entity->name->value), 'Name value is not set.');
+

We need a test which tests this unsetting vs setting it empty.

hm, looks like the committed variant conflicts with #1869250: Various EntityNG and TypedData API improvements :/

So, we need to get it fixed to move with #1869250: Various EntityNG and TypedData API improvements. I'm working on fixing it on top of that issue right now to get tests green again. So maybe we should just incorporate it over there?

Let's fix this with #1869250: Various EntityNG and TypedData API improvements, please review the patch over there.

Re-opening and postponing on #1869250: Various EntityNG and TypedData API improvements for anything left (see http://drupal.org/node/1869250#comment-6891540)

unpostponing

We already have

    // Test removing all list items by assigning an empty array.
    $entity->name = array();
    $this->assertIdentical(count($entity->name), 0, 'Name field contains no items.');
    $this->assertIdentical($entity->name->getValue(), array(), 'Name field value is an empty array.');

    $entity->name->value = 'foo';
    $this->assertTrue($entity->name->value, 'foo', 'Name field set.');
    // Test removing all list items by setting it to NULL.
    $entity->name = NULL;
    $this->assertIdentical(count($entity->name), 0, 'Name field contains no items.');
    $this->assertNull($entity->name->getValue(), 'Name field value is NULL.');

but still this is a good addition and verifies isset() behaves correctly - so yep let's add this.

Committed/pushed to 8x., thanks!