Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
We have now settled on the ItemList / Item naming, so the filterEmptyValues() method is now very poorly named, should rather be filterEmptyItems().
Comments
Comment #1
blesss CreditAttribution: blesss commented.
Comment #2
blesss CreditAttribution: blesss commentedComment #3
blesss CreditAttribution: blesss commentedComment #4
yched CreditAttribution: yched commentedThanks @blesss!
After posting a patch, don't forget to set the issue to "needs review" so that the testbot runs and other contributers the patch is up for reviewing :-)
Comment #5
yched CreditAttribution: yched commentedWhile we're in here, we could rename the $field variable to $items, which is now the "official" terminology.
Same here
Other than that, looks good! Would be nice to do the above cleanups as well, but otherwise RTBC.
Comment #6
oushen CreditAttribution: oushen commentedLooks good, let's do this!
Comment #7
InternetDevels CreditAttribution: InternetDevels commentedRenamed variables.
Comment #8
yched CreditAttribution: yched commentedYay, thanks!
RTBC if testbot approves
Comment #9
yched CreditAttribution: yched commented7: drupal_core-rename_filterEmptyValues-2141539-7.patch queued for re-testing.
Comment #11
webchickDoing the bot's work for it...
Comment #12
JStanton CreditAttribution: JStanton commentedComment #13
JStanton CreditAttribution: JStanton commentedComment #14
JStanton CreditAttribution: JStanton commentedComment #15
yched CreditAttribution: yched commentedThanks @JStanton! Let's do this.
Comment #16
yched CreditAttribution: yched commentedGah. I meant to set back to RTBC...
Comment #17
yched CreditAttribution: yched commented13: drupal_core-rename_filterEmptyValues-2141539-13.patch queued for re-testing.
Comment #19
yched CreditAttribution: yched commentedSimple reroll after #2143263: Remove "Field" prefix from FieldDefinitionInterface methods
Comment #20
yched CreditAttribution: yched commentedAPI change, would really make sense to get this in before the beta
Comment #21
yched CreditAttribution: yched commented"beta target" -> bumping priority accordingly
Comment #22
sunI'm not sure whether this rename is correct and desired. Here is why:
filterEmptyValues() was supposed to remove empty values only, but not entire entire fields/properties.
The fact/bug that the entire field/property within a FieldItemList is removed has the consequence that e.g. the FieldItem's update() method is no longer invoked upon an entity update — the entire field item is gone.
I just discovered that weird behavior in #1548204: Remove user signature and move it to contrib and tracked it down to FieldItemList::filterEmptyValues():
1. Enter a field value + save → FieldItem::update() is invoked
2. Empty out the field value + save → FieldItem::update() is NOT invoked
That is a major problem, especially for computed fields like e.g. the path field.
Thoughts?
Comment #23
yched CreditAttribution: yched commentedHmmm - no, AFAIK filterEmptyValues() has always been about removing FieldItem objects that are empty (and it exists at the level of a FieldItemList for this reason).
The *naming* around FieldItem / FieldItemList objects has shifted, making the method name irrelevant & misleading, but the actual effect and conceptual role of the method has never changed.
The fact that empty Items get removed and thus cannot fire update() does more seem like a flaw inherent to the design choice of moving D7's hook_field_update() to a method in the Item objects themselves... Thus, no object = no method call...
Might indeed be an issue, but not related to this rename.
Comment #24
yched CreditAttribution: yched commented@sun: the actual method that is invoked is FieldItemList::update().
The default implementation just iterates on calling FieldItem::update() on all item objects that are present in the entity at this point.
But you can still provide your own, different logic in a custom FieldItemList class for your field / field type ?
Comment #25
sunWhile that is true, it is odd that I have to override a generic base implementation for a common use-case, in order to not do something it happens to do by default.
Isn't it rather the other way around?
The current FieldItemList::filterEmptyValues() appears to originate from the idea that the contained field items/values would belong to configurable fields — (1) additional widget values that were left empty in the form are to be ignored and (2) if an item in between other items has been emptied out, the consecutive items are supposed to automatically "move up", eliminating the empty slot.
That behavior appears to be very specific for configurable multi-value fields, and thus, shouldn't it be part of a specialized FieldItemList implementation for configurable fields, instead of the default FieldItemList that applies to all possible fields?
Comment #26
yched CreditAttribution: yched commentedNot that I know of, filterEmptyValue() was part of the initial EntityNG patch, while configurable fields were not integrated at all yet.
Also, it is currently very much needed, because, for reason not entirely clarified yet, the current Entity/Field/TypedData code keeps creating empty FieldItem elements that seem to appear now and then with the lifecycle of an entity, and that interfere at various steps and need to be filtered out repeatedly (see this rename patch for the number of places the function is called - most of them were added as stopgaps...). #1988492: Avoid having empty field items by default is the issue about that.
But again, this patch is simply about renaming the method to reflect what it does (and has always been doing) according to the terminology changes that happened around it since it got introduced. That's a pure consistency, no-brainer, non-polemical task. Can we not derail it and discuss the conceptual validity of the method itself in a separate issue if need be ;-) ?
Comment #27
webchickbeta target or no, there is absolutely no way that renaming a function to something else is a major task.
Comment #28
yched CreditAttribution: yched commented@webchick: ok, opinions seem to differ between core committers then.
Rtbc, though ;-)
Comment #29
xjm19: drupal_core-rename_filterEmptyValues-2141539-19.patch queued for re-testing.
Comment #30
xjmThis guy has been in the reroll-and-RTBC cycle for more than four weeks, so tagging accordingly.
Comment #31
yched CreditAttribution: yched commented19: drupal_core-rename_filterEmptyValues-2141539-19.patch queued for re-testing.
Comment #32
yched CreditAttribution: yched commented$weeks_in_rtbc++ :-p
This will become stale again after #2167267: Remove deprecated field_attach_*_view(), would be good to get it in...
Comment #33
yched CreditAttribution: yched commented19: drupal_core-rename_filterEmptyValues-2141539-19.patch queued for re-testing.
Comment #35
yched CreditAttribution: yched commentedReroll
Comment #36
webchickCommitted and pushed to 8.x.
Comment #37
webchickOops.
I don't know that we need a change notice about this specifically, but we definitely need a pass thru the existing change notices + docs under https://drupal.org/developing/api/8 to make sure that there are no wrong examples.
Comment #38
webchick...Nope. :(
Sorry, this conflicts with #2002134: Move TypedData metadata introspection from data objects to definition objects which is a major, beta blocker, tagged "Avoid commit conflicts". Reverted the patch temporarily, marking this one postponed on that.
Since it's the same peoples' patches breaking each other, let me know which one you want in first, but for now going with the standard.
Comment #39
yched CreditAttribution: yched commented@webchick:
#2002134: Move TypedData metadata introspection from data objects to definition objects still has architectural discussions going in, it's at a the very least still a couple days away. The "Avoid commit conflicts" tag over there is a bit premature IMO.
I'd rather have this one in once and for all, I'll reroll the other one.
Comment #40
webchickOk cool, I'll get to this a bit later if no one beats me to it.
Comment #41
webchickCommitted and pushed to 8.x. Thanks!
Comment #42
webchickOops.
Comment #43
yched CreditAttribution: yched commentedThanks @webchick.
Found no mention of filterEmptyValues() in existing change records (the D7 equivalent was an internal _prefixed function, _field_filter_items()).
Found no mention in the API doc pages either.
Comment #45
sun@yched: The concern I raised in #22 just re-appeared in #1980822-40: Support any entity with path.module and appears to block that issue now. Do you have any advice how we could attack the problem of
FieldItem::delete()
not getting invoked when a previously existing field value is emptied out?Comment #46
yched CreditAttribution: yched commented@sun: replied over there.
(+ forgot to remove the title prefix in #43 above)