Posted by klausi on December 17, 2012 at 9:13am
8 followers
| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | entity system |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
Issue Summary
Problem: EntityNG always returns FALSE if you check a field/property with isset() which has been deliberately set to an empty array before.
Use case: REST module has a PATCH operation to allow partial updates. It receives data that has set some fields/properties to be empty, indicating that the field should be deleted. Currently it is not possible to determine which property has been set to empty after an entity has been deserialized. See #1826688: REST module: PATCH/update.
Test case:
<?php
$entity = entity_create('entity_test', array('name' => 'klausi', 'user_id' => 1));
$isset = isset($entity->field_test_text);
dpm(intval($isset)); // FALSE as expected
$entity->field_test_text->value = 'foo';
$isset = isset($entity->field_test_text);
dpm(intval($isset)); // TRUE as expected
$entity->field_test_text = array();
$isset = isset($entity->field_test_text);
dpm(intval($isset)); // FALSE? WTF?
?>Solution: make the magic __isset() method work like the usual PHP isset().
Comments
#1
Here is the first patch.
I'm using a boolean flag to indicate whether a Field has been set or not. IMO that is the only clean way to determine if the field has been set or not.
I also added the methods unsetValue() and valueIsSet(), not sure in which interface they should end up. I'm also open for better naming suggestion, since we cannot use isSet() (reserved PHP function name).
#2
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.
#3
So this was accidentally committed with #1826688: REST module: PATCH/update. Lets develop a followup here if you disagree with the approach.
Otherwise I'm going to close this issue in the next weeks.
#4
Cross post with fago. I'm going to look into this.
I think I remembered the semantics of isset() not correctly, because if you set a variable to NULL isset() will indeed return FALSE. Sorry for that.
#5
Hopefully addressed all remarks from #2, please review.
#6
The last submitted patch, entity-isset-1868274-5.patch, failed testing.
#7
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?
#8
Let's fix this with #1869250: Various EntityNG and TypedData API improvements, please review the patch over there.
#9
Ok, so I'm calling this issue fixed since the required functionality got in and the improvements are developed in #1869250: Various EntityNG and TypedData API improvements.
#10
Re-opening and postponing on #1869250: Various EntityNG and TypedData API improvements for anything left (see http://drupal.org/node/1869250#comment-6891540)
#11
unpostponing
#12
So let's see if the remaining test case is already covered. Go testbot!
#13
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.
#14
Committed/pushed to 8x., thanks!
#15
Automatically closed -- issue fixed for 2 weeks with no activity.