Problem/Motivation

FieldItemList::filterEmptyItems() removes Items and renumbers deltas.
But each Item is supposed to know its own delta (its "name" - i.e the way my parent calls me), and we do not update that.

// Put two values, first one being empty.
$node->field_text = array('', 'bar');
$node->field_text->count();            // -> 2
$node->field_text[1]->value;          // -> 'bar'
$node->field_text[1]->getName();  // -> 1
// Filter the first value.
$node->field_text->filterEmptyValue();
$node->field_text->count();            // -> 1
$node->field_text[0]->value;          // -> 'bar'
$node->field_text[0]->getName();  // -> oops, still 1...

Proposed resolution

Patch adds a generic ItemList::filter($callback) method, which takes care of reassigning the correct delta in each Item.
FieldItemList::filterEmptyItems() now relies on that.

CommentFileSizeAuthor
#17 interdiff.txt911 bytesyched
#17 2365585-ItemList_filter-17.patch7.36 KByched
#12 interdiff.txt3.16 KByched
#12 2365585-ItemList_filter-12.patch7.9 KByched
#11 2365585-ItemList_filter-11.patch4.74 KByched
#7 interdiff.txt512 bytesyched
#7 2365585-ItemList_filter-7.patch4.74 KByched
#4 2365585-ItemList_filter-4.patch4.74 KByched
#4 2365585-ItemList_filter-4-test-only.patch1.71 KByched
#3 2365585-ItemList_filter-3.patch3.04 KByched
#1 2365585-ItemList_filter-1.patch3.04 KByched

Comments

yched’s picture

yched commented
Status: Active » Needs review
Issue tags: +needs test
StatusFileSize
new 2365585-ItemList_filter-1.patch3.04 KB

Patch adds a generic ItemList::filter($callback) method, which takes care of reassigning the correct delta in each Item.
FieldItemList::filterEmptyItems() now relies on that.

Working on a test, but setting to NR for bot & feedback.

Status: Needs review » Needs work

The last submitted patch, 1: 2365585-ItemList_filter-1.patch, failed testing.

yched’s picture

yched commented
Status: Needs work » Needs review
StatusFileSize
new 2365585-ItemList_filter-3.patch3.04 KB

Silly last minute incomplete var rename.

yched’s picture

yched commented
Issue tags: -needs test
StatusFileSize
new 2365585-ItemList_filter-4-test-only.patch1.71 KB
new 2365585-ItemList_filter-4.patch4.74 KB

With test.
Interdiff = the test-only patch.

The last submitted patch, 4: 2365585-ItemList_filter-4-test-only.patch, failed testing.

amateescu’s picture

amateescu commented 
+++ b/core/lib/Drupal/Core/TypedData/ListInterface.php
@@ -80,4 +80,16 @@ public function set($index, $item);
+   * @return this

$this, not this :P

Otherwise the patch looks good to me but we should probably ask @fago or @Berdir to take a look as well.

yched’s picture

yched commented
StatusFileSize
new 2365585-ItemList_filter-7.patch4.74 KB
new interdiff.txt512 bytes

Heh, can't seem to get those right :-p

yched’s picture

yched commented
Issue summary: View changes
fago’s picture

fago
Primary language German
Location Vienna
commented
Issue tags: +Needs tests 
+++ b/core/lib/Drupal/Core/TypedData/ListInterface.php
@@ -80,4 +80,16 @@ public function set($index, $item);
+  public function filter($callback);

I think this is a good API addition, but as it's a separate method now I think it needs some dedicated test coverage in the typed tests as well.

Also, this extends ListInterface - which should be fine as I do not expect anyone to implement it on its own right now. Still, not sure whether this should be mentioned in some change record?

fago’s picture

fago
Primary language German
Location Vienna
commented
Priority: Normal » Major

Having an $item->getName() lying to the caller seems to be rather fundamentally bad thing to me, thus bumping to major.

yched’s picture

yched commented
StatusFileSize
new 2365585-ItemList_filter-11.patch4.74 KB

Reroll for now.

yched’s picture

yched commented
Issue tags: -Needs tests
StatusFileSize
new 2365585-ItemList_filter-12.patch7.9 KB
new interdiff.txt3.16 KB

Tests added.

yched’s picture

yched commented

Draft change notice added at https://www.drupal.org/node/2380287

RTBC anyone ? :-)

yched’s picture

yched commented

Bump - @fago's remarks have been adressed, this just waiting for an RTBC.

amateescu’s picture

amateescu commented
Status: Needs review » Reviewed & tested by the community

Let me help you with that :)

fago’s picture

fago
Primary language German
Location Vienna
commented
Status: Reviewed & tested by the community » Needs work

Thanks, added test coverage looks good. The change record draft reads good as well, I improved its formatting a bit though.

+++ b/core/modules/system/src/Tests/TypedData/TypedDataTest.php
@@ -381,20 +382,55 @@ public function testTypedDataLists() {
-    }
-    catch (\Exception $e) {
+    } catch (\Exception $e) {
...
-    }
...
+    } catch (\Exception $e) {

Not sure how that sneaked in, but that's a coding style regression? Thus, setting back to NW for that.

yched’s picture

yched commented
Status: Needs work » Reviewed & tested by the community
StatusFileSize
new 2365585-ItemList_filter-17.patch7.36 KB
new interdiff.txt911 bytes

Oops, indeed.

Just a code style fix, so back to RTBC.

yched’s picture

yched commented

@fago: thanks for the adjustements on the change record. I edited it to re-add the notion of "subsequent keys starting at 0".

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

This issue addresses a major bug and is allowed per https://www.drupal.org/core/beta-changes. Committed e55a223 and pushed to 8.0.x. Thanks!

  • alexpott committed e55a223 on 8.0.x 
    Issue #2365585 by yched: FieldItemList::filterEmptyItems() renumbers...
fago’s picture

fago
Primary language German
Location Vienna
commented

@yched: Thanks, agreed - it makes sense to clarify that.
@alex: Thanks, I published the change record then.

Status: Fixed » Closed (fixed)

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