Support from Acquia helps fund testing for Drupal Acquia logo

Comments

blesss’s picture

.

blesss’s picture

blesss’s picture

yched’s picture

Status: Active » Needs review

Thanks @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 :-)

yched’s picture

Status: Needs review » Reviewed & tested by the community
  1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
    @@ -781,7 +781,7 @@ public function updateOriginalValues() {
             foreach ($this->fields[$name] as $langcode => $field) {
    -          $field->filterEmptyValues();
    +          $field->filterEmptyItems();
               $this->values[$name][$langcode] = $field->getValue();
    

    While we're in here, we could rename the $field variable to $items, which is now the "official" terminology.

  2. +++ b/core/modules/field/lib/Drupal/field/Tests/FieldTestBase.php
    @@ -56,7 +56,7 @@ function assertFieldValues(EntityInterface $entity, $field_name, $expected_value
         $field = $values = $e->getTranslation($langcode)->$field_name;
         // Filter out empty values so that they don't mess with the assertions.
    -    $field->filterEmptyValues();
    +    $field->filterEmptyItems();
         $values = $field->getValue();
    

    Same here

Other than that, looks good! Would be nice to do the above cleanups as well, but otherwise RTBC.

oushen’s picture

Looks good, let's do this!

InternetDevels’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
10.52 KB
3 KB

Renamed variables.

yched’s picture

Status: Needs review » Reviewed & tested by the community

Yay, thanks!
RTBC if testbot approves

yched’s picture

The last submitted patch, 7: drupal_core-rename_filterEmptyValues-2141539-7.patch, failed testing.

webchick’s picture

Status: Reviewed & tested by the community » Needs work

Doing the bot's work for it...

JStanton’s picture

Assigned: Unassigned » JStanton
JStanton’s picture

Assigned: JStanton » Unassigned
FileSize
8.91 KB
JStanton’s picture

Status: Needs work » Needs review
yched’s picture

Thanks @JStanton! Let's do this.

yched’s picture

Status: Needs review » Reviewed & tested by the community

Gah. I meant to set back to RTBC...

yched’s picture

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 13: drupal_core-rename_filterEmptyValues-2141539-13.patch, failed testing.

yched’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
8.9 KB
yched’s picture

Issue tags: +beta target

API change, would really make sense to get this in before the beta

yched’s picture

Priority: Normal » Major

"beta target" -> bumping priority accordingly

sun’s picture

I'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?

yched’s picture

Hmmm - 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.

yched’s picture

@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 ?

sun’s picture

But you can still provide your own, different logic in a custom FieldItemList class for your field / field type ?

While 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?

yched’s picture

The current FieldItemList::filterEmptyValues() appears to originate from the idea that the contained field items/values would belong to configurable fields

Not 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 ;-) ?

webchick’s picture

Priority: Major » Normal

beta target or no, there is absolutely no way that renaming a function to something else is a major task.

yched’s picture

@webchick: ok, opinions seem to differ between core committers then.

Rtbc, though ;-)

xjm’s picture

xjm’s picture

Issue tags: +RTBC 4+ weeks

This guy has been in the reroll-and-RTBC cycle for more than four weeks, so tagging accordingly.

yched’s picture

yched’s picture

$weeks_in_rtbc++ :-p

This will become stale again after #2167267: Remove deprecated field_attach_*_view(), would be good to get it in...

yched’s picture

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 19: drupal_core-rename_filterEmptyValues-2141539-19.patch, failed testing.

yched’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
7.56 KB

Reroll

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x.

webchick’s picture

Title: Rename FieldItemListInterface::filterEmptyValues() to filterEmptyItems() » Change notice: Rename FieldItemListInterface::filterEmptyValues() to filterEmptyItems()
Priority: Normal » Major
Status: Fixed » Active
Issue tags: +Needs change record

Oops.

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.

webchick’s picture

Title: Change notice: Rename FieldItemListInterface::filterEmptyValues() to filterEmptyItems() » Rename FieldItemListInterface::filterEmptyValues() to filterEmptyItems()
Priority: Major » Normal
Status: Active » Postponed
Issue tags: -Needs change record

...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.

yched’s picture

@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.

webchick’s picture

Status: Postponed » Reviewed & tested by the community

Ok cool, I'll get to this a bit later if no one beats me to it.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

webchick’s picture

Title: Rename FieldItemListInterface::filterEmptyValues() to filterEmptyItems() » Change notice: Rename FieldItemListInterface::filterEmptyValues() to filterEmptyItems()
Priority: Normal » Major
Status: Fixed » Active
Issue tags: +Needs change record

Oops.

yched’s picture

Status: Active » Fixed
Issue tags: -RTBC over 4 weeks, -Needs change record

Thanks @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.

Status: Fixed » Closed (fixed)

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

sun’s picture

@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?

yched’s picture

Title: Change notice: Rename FieldItemListInterface::filterEmptyValues() to filterEmptyItems() » Rename FieldItemListInterface::filterEmptyValues() to filterEmptyItems()

@sun: replied over there.

(+ forgot to remove the title prefix in #43 above)