Come together with the global Drupal community in Rotterdam, 28 Sept – 1 Oct 2026. Sessions, contribution, connection, and Early Bird savings until 8 June.Helper issue: #2353411: Helper issue for #2164601 - stop auto-creating FieldItems
Sandbox branch: 2164601-list_auto_create in http://cgit.drupalcode.org/sandbox-yched-1736366
Problem/Motivation
Just trying to access an arbitrary delta in a FieldItemList automatically creates an "empty" FieldItem object (with all NULL properties) and adds it to the list at that delta.
This allows nice DX:
- for accessing values without bothering whether the field is empty or not:
$entity->field[0]->value;
(you'll get NULL if the field is empty, which is what you want) - for easy assignment of field values on-the-fly:
$entity->field[0]->value = 'foo'; $entity->field[1]->value = 'bar'; $entity->field[2]->value = 'baz';(works because accessing the delta creates an item ready for you to populate).
Yet this is very wrong for several reasons:
- Looking at the object modifies the object :-p
- This produces inconsistent/confusing behavior:
isset($node->body[0]); // TRUE - OK isset($node->body[42]); // FALSE - OK $node->body[42]; // TextWithSummaryItem - What ? You just said it didn't exist ?Offset doesn't exist, but I can still get it and it's a real, non NULL object.
- The "ArrayAccess" metaphor doesn't really fly: the DX above is nice, but is not what you'd expect from a real array. $entity->field[2]->value would fatal out if $entity->field was an actual array with no entry at index 2.
- In terms of data strutures, a FieldItemList (or more generally an ItemList) is supposed to be a sequence of Items with subsequent numeric indexes starting at 0. That's what the data model is, and that's what consumer code that "does stuff with ItemLists" expect. But the code above just created a "whole" in the deltas, and iterating on the list will jump from 0 to 42.
Even though we present an array-like syntax for DX, the API should enforce the consistency of the underlying data structure, and make sure a List stays a List no matter what you try to do with it. - This makes it *very* easy to write code that inadvertently adds new "empty" Items at runtime, de-facto forcing any code that "does stuff with ItemLists" to account for the possibility of hitting one of those, which they typically do not expect.
We have sprinkled ->filterEmptyItems() calls in various places in core to remove those Items before running the ItemList through some specific, identified steps (e.g formatters, entity save...). Still I found myself helping a participant to the Amsterdam Sprint, and saw she had to manually safeguard herforeach ($entity->field as $item) {with a manualif ($item->isEmpty()) {break;}. Good thing she bumped into it early; worst case, your code works, but breaks later when you enable another contrib module that tries to read field values from the entity before it reaches you.
In other terms, big DX fail.
Proposed resolution
We discussed that issue with @fago, @Berdir, @plach, @amateescu at the post-conf sprint in Amsterdam.
We came up with the following proposal:
- No more creating ItemLists with an Item with NULL properties - that's being done in #1988492: Avoid having empty field items by default
- No more auto-creating Item objects on accessing List[$non_existent_delta] - this is what this issue is about.
More specifically:
When a (non computed) field has no value:
- ItemList::$list is an empty array()
- foreach ($entity->field as $item) produces 0 iterations
- $entity->field->first() returns NULL
- $entity->field->property (ItemList::__get()) returns NULL
- $entity->field->property = 'foo' (ItemList::__set()) creates an Item at delta 0 and populates the property
Exception for computed fields: those are never really "empty", Items will get auto-created, and will appear when reading values or iterating $entity->field. That's actually the only specific meaning of "computed field" (as opposed to "a regular field with computred properties").
More generally:
- $entity->field[unknown delta] (ItemList::offsetGet()) returns NULL
- $entity->field[unknown delta]->property / ->method() thus fails with a fatal
In other words: we safeguard $entity->field->property for nice DX (e.g for mono-valued or required fields), but if you try to do stuff on an explicit delta, you need to ensure it exists with an isset(), just like you would do with a real array.
New items can be added:
- For the first item at delta 0, with the $entity->field->property = 'foo' syntax mentioned above,
- For any delta, by a new ItemList::append() method (with support for the [] syntax):
$entity->field->append(array('property' => 'foo', 'property_2' => 'bar'));
$entity->field[] = array('property' => 'foo', 'property_2' => 'bar');
// Short syntax to only assign the 1st property:
$entity->field[] = 'foo';
There is still a possibility of explicitly creating an "empty" Item by doing $entity->field->append(NULL) and doing nothing with the item after that. Widgets need this for the "add new value" button, and some other code need it to access non-static methods that live on the Item class (e.g Field UI and Item::fieldSettingsForm()).
But then it's your explicit choice, and your fault if those $items leak out to other runtime code.
API changes
- $entity->field[$delta] / $entity->field->get($delta) returns NULL instead of a fresh, empty Item when delta doesn't exist.
- $entity->field->first() returns NULL instead of a fresh, empty Item when the field is empty.
In practice:
- If you want to do stuff with an item (read a value, call a method), you need to ensure it exists first (isset($entity->field[$delta]), as you would with a regular array.
- If you want to add *new* items:
$entity->field[2]->value = 'foo'; doesn't work anymore (fatal error if the delta doesn't already exist)
Instead you do one of:
$entity->field->appendItem(array('value' => 'foo'));
$entity->field[] = array('value' => 'foo');
$entity->field[] = 'foo';
- For DX, the short-hand syntax for the 1st item at delta 0 still works (which is the most common use case):
$entity->field->value; returns the value at delta 0 if the field is not empty, NULL otherwise
$entity->field->value = 'foo'; creates an Item at delta 0 if the field was empty, and populates the value.
| Comment | File | Size | Author |
|---|---|---|---|
| #102 | interdiff.txt | 3.73 KB | yched |
| #102 | 2164601-list_auto_create-102.patch | 51.32 KB | yched |
| #100 | interdiff.txt | 717 bytes | yched |
| #100 | 2164601-list_auto_create-100.patch | 51.84 KB | yched |
| #89 | 2164601-list_auto_create-89.patch | 51.75 KB | amateescu |
Comments
Comment #1
yched commentedComment #2
plopescHello
It looks like it was done by design. This is the
offsetGet()implementation inItemListclass:offsetGet()is overloaded in order to create new items at certain index.Should we remove this overload and provide the public
createItem()method as suggested in the @todo??Regards.
Comment #3
fagoYeah, it's an object but containing only NULL/empty values, so if you ask it whether it's empty it is.
The thinking behind that was that it doesn't make any semantically difference and this eases creating/setting new items, in particular if you think of the single item case.
However, I guess there are cases where there is a difference between empty and non-existing values, e.g. when you present widgets for some items. I guess this works correctly right now, as the iterator only returns existing items, but I see that this is potentially confusing.
As an alternative I could see us doing a public createItem() method that appends it at the end of the list. But then, I think we still want to keep the current behaviour at least for the first item, so you can safely do $node->title->value? That said, the behaviour would still exist but be limited to the first item, is that less confusing?
Comment #4
yched commentedYes, an explicit createItem() or appendItem() seems preferable to a magic "just ask for it and it suddenly exists".
(Also because the current behavior (->offsetGet(42)) generates non consecutive deltas, which is not something that is supposed to happen, and thus potentially requires renumbering before we do anything important)
So yeah, +1 on appendItem() + "no auto-creation for deltas > 0";
But I'm not sure about delta 0 either...
IMO this is very much related to #1988492: Avoid having empty field items by default and the random "Item objects with NULL values" that pop up at various times in our code flow and makes our code covered with safeguard filterEmptyValues() calls - which I still think is a real problem.
Our conceptual model relies on the ItemList being a mere glorified numeric array of items. In that sense,
- An Item object with NULL values is not the same as no object.
- An (Item)List with that has an Item with NULL values is not the same as an empty List.
During the lifecycle of an entity, our ItemLists keep going from one flavor of "empty" to the other, depending on the sequence of "accessing values" (which creates NULL-values Item objects) and filterEmptyValues() calls that get added here and there to clean them up before running code that needs "empty" to mean "empty".
So for a same field that is semantically "empty", a foreach ($items as $delta => $item) will sometimes produce one iteration (delta 0 / $item with NULL values) and sometimes none. That's ... not great :-)
Note that in D7, _field_filter_items() / hook_field_is_empty() was only there to process widget submitted form values (e.g discard textfields left empty) before promoting them to the $entity->field_foo model structures. That was 2 calls (of an '_' prefixed funtion...), one in form validate, one in form submit. Once the values were in the $entity model, empty was array(), and no further filtering was performed.
In current HEAD, there are 13 calls to FieldItemList::filterEmptyValues().
I think we should get back to that. There should be one flavor of "empty" IMO, and that's ItemList::$list == array().
Proposal in the next comment :-)
Comment #5
yched commentedSo - I understand that it's handy to have $node->field->value not fatal out if the field is empty.
I'm not fully sure making it "just work even if empty" makes for consistent / sustainable DX - that's not a behavior that you would realistically expect from someting that is conceptually an array of values. If your data model is "the field might be empty", then doing isset() checks is only consistent with the data model. But let's say we try to preserve that for now.
We do have a "prototype" of an Item object with NULL values available somewhere, right ? What if :
- an empty ItemList is just $this->list == array()
Iterating on it makes 0 iterations, offsetGet(any $delta) returns NULL / possibly even triggers a notice ?
- the magic getters make
$entity->field_foo->valuereturn property values from the prototype *without* copying the prototype into $this->list - the list stays empty.- the magic setters make
$entity->field_foo->value = $valuetrigger the "clone proptotype as a new item in the previously empty list" behavior.Would that work ?
Comment #6
yched commentedProbably a bit rough still, but let's see what fails.
Comment #8
yched commentedShould fix a couple fails.
Problem is of course that not auto-creating items breaks syntax like:
Mulling on that...
Comment #11
yched commentedRestarting this - will try to summarize the discussion we had at the Amsterdam post-conf sprint today.
For now, just rerolling the current patch.
Comment #13
yched commentedKeep auto-create for the
$entity->field->value = 'foo'syntaxComment #15
yched commented[recap of the solution discussed in Amsterdam, moved to the issue summary]
Comment #16
yched commentedIt's not completely clear when this Item gets created though (straight in ItemList::__construct(), as we currently do for all fields ?)
Also, not clear how many items should be auto-created. Why couldn't there be computed fields with multiple values ?
Comment #17
yched commentedSide note: the only "computed field" we seem to have in core atm is Shortcut 'path'...
Comment #18
yched commentedOK, here's a patch that should install.
Ugly bit: in current HEAD, FieldItemList::applyDefaultValue() does:
so that the field type can provide a "default default value" in its Item::applyDefaultValue() method in case the field definition provides no default value.
That only worked because we always created one Item, which we need to stop doing. For now, I replaced that with the following:
But this can only be a temporary stop gap. We need to get rid of that practice of field types using FieldItem::applyDefaultValues(), creating an item to figure out whether there should be an item is absurd. Field types that need this have other ways:
- use the 'default_value' annotation for the field type added by #2318605: uuid, created, changed, language default value implementations need to be updated
- implement FieldItemList::processDefaultValue(),
- override FieldItemList::applyDefaultValue() itself.
Comment #19
yched commentedNR for testbot.
Also, from the plan in #15:
Not sure we'll be able to do that. In addition to the ugly workaround mentioned in #18, widgets rely on that for the empty rows added by the "add new" button.
Comment #21
yched commented- Opened helper issue : #2353411: Helper issue for #2164601 - stop auto-creating FieldItems
- pushed the patch to 2164601-list_auto_create in http://cgit.drupalcode.org/sandbox-yched-1736366
- Expanded the issue summary with a description of the problems caused by the current situation, and a copy of #15 for the solution that was agreed in Amsterdam.
Comment #22
yched commentedAlso, clearer title.
Comment #23
yched commentedComment #24
yched commentedAdded an "API changes" section
Comment #25
yched commentedDown to 30-ish fails in the helper issue, some of them hairy, will report soonish.
Meanwhile, found the following while working on those fails:
#2354177: Shortcut::getRouteParams() should be named getRouteParameters() for consistency
#2354199: Calling ContentEntity::updateOriginalValues() should not be the task of the SqlContentEntityStorage
Comment #26
yched commentedUpdate to the code samples in the issue summary : we'll also be supporting
$entity->field[] = 'value'
(same polymorphism as supported by TypedData's Item::setValue())
Comment #27
yched commentedShould be down to just NodeTranslationUITest failing. More on that one in a later comment.
Summary of the interdiff:
That's:
- Field UI calling Item::field(Storage)settingsForm(),
- The current practice for "allowed values" (Item implements OptionsProviderInterface), which should get deprecated in #2002138: Use an adapter for supporting typed data on ContentEntities
- The current practice of Item::applyDefaultValue(), which we really should get rid of. Separate discussion though.
Comment #29
yched commentedFix NodeTranslationUITest.
In short: "comment as field" relies on the fact that the "comment status" field always returns a value - if no stored value, then the "default value" needs to be returned (which is not the behavior of Field API's "default value", which only applies at entity creation time).
This is currently done by CommentItem overriding FieldItemBase::__get(), which doesn't work now that "no stored value" means there is no Item object created.
Patch does it by CommentFieldItemList overriding ItemList::get().
Comment #30
yched commentedAnd the patch
Comment #31
yched commentedOk, green.
Patch still has a couple @todos for docs / pending questions (will summarize those in a later comment), but should otherwise be ready for reviews.
Comment #32
berdirWhy remove the $items check? it is defined as a n optional argument that can be NULL...
Comment #33
swentel commentednitpick - redundant space
Comment #34
yched commentedWhile working on deltas renumbering, it occured to me that filterEmptyItems() is currently inconsistent : #2365585: FieldItemList::filterEmptyItems() renumbers deltas but does not update the Items
The next patches here will most probably include that fix, but it's already a bug in HEAD so we might as well fix it separately.
Comment #35
yched commentedUpdated patch:
- Fixes @berdir's and @swentel's remarks in #32 / #33
- Ensure subsequent deltas : renumber the subsequent deltas on unset($list[N])
Adds a $list->drop(N) method as a non-array-syntax equivalent.
- Ensure subsequent deltas : only support 0-based subsequent deltas in $list->setValue($array)
$list->setValue(array(1 => 'foo', 3 => 'bar')) raises an exception.
That revealed a couple places that currently set field values on non 0-based consecutive deltas:
- $user->removeRole()
- The custom form code for $term->parent / $forum->parent (the form values need to be massaged *once*, before building the entity - thus done in validate()).
- Ensure delta consistency : updates $item->getName() (which is supposed to be the item delta) when any renumbering happens (in unset() case above, or using filterEmptyItems()). This includes #2365585: FieldItemList::filterEmptyItems() renumbers deltas but does not update the Items, which would be best to get in first.
- Adds some missing docs, adds the newly added methods to ItemListInterface (and thus also adds implementations in Sequence.php) :
appendItem(), drop(), clearItems()
- Opened #2356623: Remove usages of FieldItem::applyDefaultValue() to stop using FieldItem::applyDefaultValue(), and referenced it in the existing @todo
- Dropped the protected ItemList::nextIndex(), not really useful.
Comment #36
yched commentedTo be discussed :
- Instead of an exception on $list->setValue(array with non 0-based deltas), do we want to just silently renumber the passed-in values (array_values()) ?
Might be easier DX ?
- At some point I considered forbidding NULL items altogether, by:
dropping the item on $list->set(N, NULL) / $list[N] = NULL
filtering out NULL values in $list->setValue($array)
I abandoned that, because you can always assign NULL on the item directly : $list[N]->setValue(NULL).
Also, while items carring the NULL walue are not wanted for FieldItemLists, I cannot say for sure we don't want to support that in ItemLists generally.
In the helper issue, @amateescu suggested that we could have FieldItem::setValue(NULL) notify the FieldItemList parent, which in turn could remove the element from the list. I still think it would make some unintuitive/surprising behavior - e.g. :
I guess we could forbid *FieldItem* to receive the NULL values :
- have FieldItem::set(NULL) raises an exception
- have FieldItemList::setValue($array) filters out NULL values
Not fully decided yet.
- FWIW, I stumbled across SplDoublyLinkedList, which is the SPL implementation of a "List" data structure. I'm actually thinking it would make sense to switch the internal ItemList::$list to a SplDoublyLinkedList instead of an array, since it already does exactly what we want in terms of index management / 0-based sequential deltas / renumber on drop - but in C code. And well, we want a List, this is a List...
Still mulling on that, but opinions welcome.
Comment #37
yched commentedDid that. Patch reverts the fixes in TermForm, ForumForm, User.
+ Opened #2368807: Remove special support for NULL values in FieldItemList as a side issue
Comment #39
yched commentedarray_walk() takes its array by ref, array_walk(func()) = no good.
Comment #41
yched commentedShakes fist at duplicate code.
Comment #42
yched commentedReroll after #2369639: EntityFieldTest helper methods makes debugging tests impossible
Comment #43
alexpottAccording to @dawehner this is how nature works.
Comment #44
fagoDid a first review - trying to stay out of quantum theory for now, but maybe this can make newton happy again ;-)
That code reads a bit weird as it doesn't add it on a special delta. Maybe it could check teh count first and append atems as long until the target count is reached?
Afterwards, it could decide to just loop over items?
I think "remove()" would be a better method name. Also, append has the item suffix, this one doesn't. Whatever we end up using should be consistent - I'd tend to have an item suffix as it makes it more clear what you are dealing with.
Comment uses different wording that method name.
$this
Not sure that's needed, as you could just set it to an empty array?
It clears the list, not the items imho.
That's a bit hairy, but I could see things like the comment new property being a computed item, while the list is not computed but has one fixed item. Maybe, this could be better handled in a separate field item list class for single, computed fields?
As the API did not differentiate on having the $item being created or not, that followed that. I.e., it does not matter whether there is a NULL property or not.
But this patch is going to change this, so should be this method. -> It should tell you whether the object exists imho.
Interesting work a rount, but a bit weird as it would not work with queries. That#s another issue though.
Is that change necessary? It shouldn't be imho?
Would it make sense to add an auto-append flag to first(), which auto-appends an item if none is there?
We've got some code which follows recent "best practice" of doing an explicit:
"$author_name = $items->first()->value;"
Given that patch, that would change form being bad / unreliable :-/
While I agree with this being a good change, it's a rather big API change now.
A thought: Is it possible to migitate it by allowing write operations like this to create the object? Does it work to
- let the magic getter of the list return a new empty item, pass it the parent but do not reference it in the list
- use on change in the item, to notify the parent of the write
- detect the write on the orphaned child item and so adopt it as a new child?
That way we'd avoid the API change and could continue supporting the explicit usage of
$entity->field->first()->value?
Comment #45
xjmComment #46
yched commentedReroll after #2365585: FieldItemList::filterEmptyItems() renumbers deltas but does not update the Items got in.
Will process @fago's feedback from #44 after that.
Comment #48
yched commentedFixed the EntityReferenceFormatterTest that were added in #2370703: ER's "autocreate" feature is mostly broken (and untested), and that used the "$items[$new_delta] = ..." syntax.
This revealed an existing bug, though : on EntityRefItem,
-
$item->entity = $existing_entitypopulates $item->target_id with $existing_entity->id()- but
$item->setValue(array('entity' => $existing_entity))does not, which triggers lots of interesting fails.Opened #2386559: ERItem::setValue(array('entity' => $entity) produces broken Items for that - meanwhile, patch here uses a workaround syntax that works.
Comment #49
yched commentedReroll
Comment #50
yched commentedRe: @fago's review in #44 :
1. Yeah, bugged me a bit too, but didn't really find a better shape for that code.
"check the count first and append items as long until the target count" : well, that would be
Is that really better than doing it in the existing loop ?
2. Agreed, renamed ItemList::drop($index) to removeItem($index)
3. 4. 5. Well, in the spirit of enriching the API to manipulate the lists (append, drop, filter...) I though it could be nice to add a method to enpty the list rather than just relying on setValue(array()), which is a TypedData-ism, but is less immediately clear in terms of "operation on a list data structure".
But well, turns out SplDoublyLinkedList, which is the data structure we could inspire from, has no method for emptying the list either. So, well... removed it :-)
6. $this->definition->isComputed() || $this->definition->getItemDefinition()->isComputed()
Yeah, the weird thing is that *I* was the one that added that code in patch #8 above - but I can't remember why :-)
I'll try to remove it in a next patch and see what happens :-)
7. You mean this ?
8. True, doesn't work with queries - but that's also the case with the current workaround in CommentItem::__get()
Query-wise, you currently need to differentiate "entities where the comment status is X" and "entities where the comment status not unspecified" (or do an "= X OR is NULL") anyway. Patch doesn't change that, it only slighlty cahnges the way "missing comment status" is filled in at runtime.
But that makes me think that the behavior added here in CommentFieldItemList::get() should let us remove the similar behavior in CommentItem::__get(). Will try that in a next patch.
9. Right, not really needed I think - it's just that $entity->get('field_datetime')->set(0, $value) / $entity->set('field_datetime', $value) feel very outdated :-) Reverted.
Will report on 10. & 11.
Comment #51
yched commentedAs mentioned in #50 :
- Removes CommentItem::__get(), should be taken care of by CommentFieldItemList::get() now.
- Tries to simplify the code supporting computed fields, because "computed fields" are still a very blurry concept at this point.
Comment #53
fagoad 7.: yes exactly, this and interdiffs look good.
Comment #55
yched commentedFix CommentItemList : if get() ensures there's always at least one Item, then offsetExists() needs to tell the same story.
That's interesting, because as per discussion with @fago here at the Ghent sprint, even though the comment fields are not computed fields strictly speaking (we do store the values, we just need to kick in and make sure there's a value even when there is none currently stored), CommentItemList in that latest patch is a good example of how a "computed field" should work :
- override ListClass::get() to make sure the ItemList contains the correct number of Items
- override ListClass::offsetExists() accordingly
It could be doable to provide helpers in a base ComputedFieldItemListBase class to provide "guidelines" for implementors. To be explored in a separate issue that clarifies what exactly are "computed fields" and how they should behave.
Comment #56
yched commentedComment #57
fagoYep, do we already have an issue for clarifying computed fields usage?
ouch. isEmpty() checks will trigger that check down the tree, so having it to introduce here sucks. Why not just get the value of the whole list and then get the first item?
As discussed, we'll still need a way to make this things easier. For BC, first() should probably even default to it - let's dicuss?
Missing trailing point.
This should be in __set() only as it's not something what makes sense for typed data general, but for the entity field api only to support:
$entity->field = $entity2->field;
Why is that 0 gone?
Comment #58
pfrenssenRead through the patch, have not actually tried it out.
Much better :)
We're looping twice over $values now. Wouldn't it be better to only loop once?
Can be rewritten in one line:
But this is perhaps less readable.
Looping twice over $values.
Parentheses around $this->definition->isComputed() are not needed.
Throw an exception if the item is not found in the list?
You can use
for ($i = $from_index, $i < count($this->list), $i++)instead of theforeach()to avoid looping over unneeded indexes.This does not need to be added.
I have trouble understanding what is going on here, but if we have no
$field_datathen we're not doing anything with$items. Is it then still necessary to retrieve them and set their values?<nitpick>Capitalize 'instead'</nitpick>
Comment #59
yched commentedre @fago #57:
Not yet, created #2392845: Add a trait to standardize handling of computed item lists
1. Fixed
2. Discussed live with @fago at the Ghent sprint, we agreed $items->first($autocreate_if_empty) is not needed in the end. Very little code actually has a use for "get the first item, add one if not exists". One of the places that do (BFD/FSConfig::getOptionsProvider()) actually should *not* leave the created item in the entity, patch fixes that.
3. Fixed
4. Existing code. @fago thinks it is incorrect, but that's for a separate issue
5. Because FieldItem::set($non_existing_delta, $value) now raises an exception. That's actually consistent with what the phpdoc for ListInterface::set() currently says : "*Replaces* the item at the specified position in this list".
re @pfrenssen #58:
1. :-)
2. Discussed live with @fago & @pfrenssen : we decided to drop the exception on non-numeric keys here, for perf reasons.
3. Yeah, matter of taste, but less readable IMO :-)
4. Same as 2. Note that #2381777: Unify setValue() implementations in ItemList & FieldItemList will remove the code duplication.
5. Fixed
6. Discussed with @fago & @pfrenssen, changed to : removeItem($index) throws an exception when the index is not in the list.
7. True - also, added the missing phpdoc
8. Fixed
9. Explained live at the Ghent sprint :-)
10. Fixed
Comment #60
fagoThanks, patch looks already pretty solid.
I still think that's an unnecessary API break at this stage - imo it should be totally valid to do: $node->title->set(0, array('value' => 'foo'));
I think that should just be removed. The default of a field has to be in the field definition.
We should apply defaults when appending an item though, such that a possible default to an item property would be applied.
This is one example of previous valid usage which we should not break unnecessarily.
So this leads to an immediate re-key of the list. Does this break code like:
foreach ($list as $i => $item) {
if ($item is blue) {
unset($item[$i]);
}
}
Does the second unset still unset the right item?
Comment #61
pfrenssen1. I agree. We shouldn't babysit missing default values.
2. Seeing it like this it indeed looks like bad DX.
3. I would assume that this would indeed break. We can either opt to not rekey, or to provide a method like $this->removeItems($offsets) to remove multiple items and then rekey at the end.
Comment #63
yched commentedReroll + fix test :
TypedDataTest needs to be adjusted accordingly
re @fago #60:
OK, why not. Changed ItemList::set($delta, $value) to allow adding a new item if $delta is the next available position in the list.
This still maintains the consistency of sequential, while allowing you to populate your field on the fly :
$node->field[0] = 'foo';
$node->field[1] = 'bar';
$node->field[2] = 'baz';
As I pointed in Ghent, that means changing ListInterface::set() phpdoc, because it currently states it's only about "replacing" an item, not creating new ones at unpopulated indexes.
1. I think we had that discussion in Ghent. As the code comment you mention says, agreed that it would be way better to remove that and go with "The default of a field has to be in the field definition" if we could, but we can't at the moment since there are currently field types in core that provide their own business logic in their own override of applyDefaultValue(). See also #18 above.
I opened #2356623: Remove usages of FieldItem::applyDefaultValue() for that, and the code you quote has a @todo on that issue - but that's way outside the scope of this patch.
I don't think I agree, but that seems like a separate discussion anyway. Current HEAD doesn't apply defaults on a new Item that gets "magically autocreated" into a List, right ? The patch here replaces "new Items are magically autocreated" with "new Items need to be explicitely appended", but doesn't (and shouldn't) change whether defaults need to be applied to those new Items.
2. Er - yes, that BC break is exactly the title of this issue :-)
It is very precisely the intent of this patch, as was disscussed and agreed in Amsterdam 50 comments ago ?
For the case of the specific code instance pointed here : the "add more" button means widgets *add* new items to the current ItemList, and let you edit them.
HEAD pretends that an ItemList is an endless pit of Items - "just ask for more, I have as many as you want".
With patch, the widget takes an explicit action of adding a new item, which completely makes sense IMO.
3. Yep, such code would break, you cannot work that way on a data structure that is about subsequently indexed elements - this exact same code would break on a SplDoublyLinkedList (which also support an ArrayAccess $list[$i] syntax).
This is why we provide the ItemList::filter() method.
Comment #64
jibranWe can remove this @todo it is commit.
Comment #65
yched commentedReroll,
+ adressed #64 : in fact that whole hunk can be removed now.
Comment #67
yched commentedSilly me, spoke too soon.
Scratch #65, that is the correct change for #64.
Interdiff is with #63.
Comment #68
yched commentedReroll + bump ?
Comment #69
fagoRight, thanks :)
So the old code should continue to work now, right? That's what I meant with valid usages that the previous patch made invalid - but great to see this addressed now.
Now unnecessary changes, but the new code is better readable anyway.
I think we should have some test-coverage for appending items with the right offset works. Settings to needs work for that, else patch looks ready to go to me.
Also, this needs a change record draft and update issues summary for committers.
Comment #70
yched commented@fago : thanks :-)
Reroll for now, then I'll address #69
Comment #71
yched commentedBack to NW for #69
Comment #72
larowlanI can guide someone through #69 during critical office hours (in 4 hours)
Comment #73
jibranWorking on this during critical office hours.
Comment #74
larowlanComment #75
jibranThank you @larowlan for all the help.
For #69
Comment #78
jibranFixed the test fails. Also created drafted change notice https://www.drupal.org/node/2404453
Comment #79
jibranIt is ready for review again.
Comment #80
pfrenssenNeeds reroll now that #2404021: entity_reference formatters should be in Core is in.
Comment #81
pfrenssenI rebased against HEAD, but this magically caused this hunk to appear in the patch:
diff --git a/core/modules/field/src/Tests/EntityReference/EntityReferenceFormatterTest.php b/core/modules/field/src/Tests/EntityReference/EntityReferenceFormatterTest.php index 45f7bdc..6f38dac 100644 --- a/core/modules/field/src/Tests/EntityReference/EntityReferenceFormatterTest.php +++ b/core/modules/field/src/Tests/EntityReference/EntityReferenceFormatterTest.php @@ -263,15 +263,17 @@ protected function buildRenderArray(array $referenced_entities, $formatter, $for // Create the entity that will have the entity reference field. $referencing_entity = entity_create($this->entityType, array('name' => $this->randomMachineName())); - $delta = 0; + // Assign the referenced entities, and place the 'access' flag so that the + // formatter does not need to check field access. + $items = $referencing_entity->get($this->fieldName); foreach ($referenced_entities as $referenced_entity) { - $referencing_entity->{$this->fieldName}[$delta]->entity = $referenced_entity; - $referencing_entity->{$this->fieldName}[$delta++]->access = TRUE; + $items[] = [ + 'entity' => $referenced_entity, + 'access' => TRUE, + ]; } // Build the renderable array for the entity reference field. - $items = $referencing_entity->get($this->fieldName); - return $items->view(array('type' => $formatter, 'settings' => $formatter_options)); }We're being kicked out of the sprint room now, I'll have another look tomorrow.
Comment #82
manningpete commentedI could apply the patch, so no reroll is needed.
Comment #83
pfrenssenThanks for having a look! Indeed I was mistaken, that hunk did not appear out of nowhere but was just moved around in the reroll, causing it to be highlighted in my difftool when I compared the "before" and "after" branches. All is well.
Comment #84
yched commentedThanks folks !
Assigning to @fago for RTBC ?
Comment #85
yched commentedReroll after #2235457: Use link field for shortcut entity. No need to touch anything in shortcut.module anymore.
Comment #86
yched commentedAs a side note, we need to get this in before we can work on #2392845: Add a trait to standardize handling of computed item lists
Comment #87
amateescu commentedRerolled for #2406749: Use a link field for custom menu link. Same as #85, no need to do anything in MenuLinkContent anymore.
@fago's feedback from #69 has been addressed, we have a change record draft, I think it's time to get this in.
Comment #89
amateescu commentedHard to keep up with HEAD these days/hours. Yet Another Reroll!
Comment #90
RavindraSingh commentedIts needs reroll as per latest core api changes.
Getting failed when tried attached is a ref.
Comment #91
RavindraSingh commentedchanging to needs work.
Comment #92
RavindraSingh commentedComment #94
plach#90 applies cleanly on the latest head here.
(didn't review the patch)
Comment #95
jibranDoesn't need re-roll. @RavindraSingh are you sure you have the latest changes in HEAD on your local clone?
Comment #96
RavindraSingh commentedYes, I think and now. And now its also sending the message
plach queued 89: 2164601-list_auto_create-89.patch for re-testing. see #90
Comment #97
fagoYep, agreed this is ready. I had another look at the patch, looks all good to me.
I took a look at the change record draft and overhauled it, such that it provides a good summary of the changes. Please check it: https://www.drupal.org/node/2404453
Also, I guess this will need a beta evaluation summary.
Comment #98
yched commentedThanks folks.
It is a "triaged critical" already, so not sure it needs a beta evaluation summary ?
I'll write one if needed :-)
Checking the draft record
Comment #99
yched commentedMade a couple adjustments to the change record, otherwise looks great
https://www.drupal.org/node/2404453/revisions/view/8079535/8079585
Comment #100
yched commentedEr - not sure why I kept a now useles FieldItemList::__construct() for so long :-)
Trivial change, so keeping at RTBC.
+ yay at comment #100.
Comment #101
alexpottThis looks dodgy - should we have a method to create a detached item? Also the duplicated code between BaseFieldDefinition and FieldSotrageConfig is painful. Looks like we could use
\Drupal::typedDataManager()->getPropertyInstance($items, 0, NULL);.No need for the
...Comment #102
yched commentedReroll after #2374019: Cleanup the use of custom item properties in EntityReferenceFormatterBase
@alexpott #101:
1. "allowed values", yeah... :-( We discussed that with @fago in Ghent, and preferred not to introduce an official API in FieldItemList for creating an orphaned $item, a FieldItem is supposed to be created in a parent entity, we'd rather not make that too blurry (although #2227227: FieldTypePluginManager cannot instantiate FieldType plugins, good thing TypedDataManager can instantiate just about anything might or might not reconsider that)
There are still a couple places where we need to call logic on the FieldItem class without having an actual field value in an actual entity. In retrospect, it was a very early design mistake in Field API D8 to make the Item class be the main class holding the field type logic, and we've removed most cases along the months, but "allowed values" is one of the last remaining. There are/were plans for a better API for OptionsProvider (#2329937: Allow definition objects to provide options), but that's complex, and a bit stalled since Ghent :-/
But true, meanwhile, it seems
\Drupal::typedDataManager()->getPropertyInstance($items, 0)should do the job in there. Trying that.2. Fixed.
Comment #104
RavindraSingh commentedComment #106
yched commentedThanks @larowlan
Green, back to RTBC ?
Comment #107
alexpottThis issue addresses a critical bug and is allowed per https://www.drupal.org/core/beta-changes. Committed 0509fbe and pushed to 8.0.x. Thanks!
Comment #109
yched commentedW0000000t !
See you in #2392845: Add a trait to standardize handling of computed item lists :-)
Comment #110
pfrenssenYaay! Congratulations!
Comment #112
swentel commentedPing from #2349819: String field type doesn't consider empty string as empty value