Coming from #2013939: Remove hook_field_[CRUD]_() in favor of hook_ENTITY_TYPE_[CRUD]() calls - field_has_data() should move to a method.

CommentFileSizeAuthor
#11 2018731-move-field-has-data-11.patch21.32 KBswentel
#11 interdiff.txt2.58 KBswentel
#8 2018731-move-field-has-data-8.patch21.76 KBswentel
#8 interdiff.txt610 bytesswentel
#6 2018731-move-field-has-data-6.patch21.75 KBswentel
#6 interdiff.txt15.13 KBswentel
#2 2018731-move-field-has-data.patch10.09 KBswentel

Comments

swentel’s picture

swentel commented
Issue tags: +Field API

Tagging

swentel’s picture

swentel commented
Status: Active » Needs review
StatusFileSize
new 2018731-move-field-has-data.patch10.09 KB

Let's see.

Status: Needs review » Needs work

The last submitted patch, 2018731-move-field-has-data.patch, failed testing.

yched’s picture

yched commented

This looks good to me - well, aside from the test fails ;-)

swentel’s picture

swentel commented

uhu, fixing them now, patch in a few minutes :)

Also, I've just added a protected property locally so we can do something like this (like getStorageDetails() or getSchema(): good or not ?

    if (!isset($this->hasData)) {
      $storage_details = $this->getSchema();
      $columns = array_keys($storage_details['columns']);
swentel’s picture

swentel commented
Status: Needs work » Needs review
StatusFileSize
new interdiff.txt15.13 KB
new 2018731-move-field-has-data-6.patch21.75 KB

Forgot an insane amount of code to remove :)

Status: Needs review » Needs work

The last submitted patch, 2018731-move-field-has-data-6.patch, failed testing.

swentel’s picture

swentel commented
Status: Needs work » Needs review
StatusFileSize
new interdiff.txt610 bytes
new 2018731-move-field-has-data-8.patch21.76 KB

Even betterer

yched’s picture

yched commented

Well, I initially wanted to go that way too but caching that is a problem IMO. This info can get stale, and it's not easy to find the right places to clear it. I mean, an entity can have been created in a concurrent request, and there's no way to detect that.

[edit: and bad things can happen if we return false positives or false negatives here - especially false negatives, because we allow more things on a field that is supposed to be "empty"]

So given that in practice, very few code will need that data, I think it's fine to recompute it each time someone needs it.
Then all this patch does is move the code to a method, which is fine IMO.

Status: Needs review » Needs work

The last submitted patch, 2018731-move-field-has-data-8.patch, failed testing.

swentel’s picture

swentel commented
Status: Needs work » Needs review
StatusFileSize
new interdiff.txt2.58 KB
new 2018731-move-field-has-data-11.patch21.32 KB

Yeah, makes sense indeed.

yched’s picture

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

Thanks !
RTBC if green.

Status: Reviewed & tested by the community » Needs work
Issue tags: -Field API

The last submitted patch, 2018731-move-field-has-data-11.patch, failed testing.

swentel’s picture

swentel commented
Status: Needs work » Needs review
Issue tags: +Field API

#11: 2018731-move-field-has-data-11.patch queued for re-testing.

swentel’s picture

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

Major botfluke. RTBC as per #12

yched’s picture

yched commented

Yay ! Thanks !

alexpott’s picture

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

Committed 30f6062 and pushed to 8.x. Thanks!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Title: Move field_has_data() to Field::hasData() » Change notice: Move field_has_data() to Field::hasData()
Priority: Normal » Critical
Status: Fixed » Active
Issue tags: +Needs change record

Oh... this will need a change notitce... field_has_data existed in D7...

chx’s picture

chx commented
Title: Change notice: Move field_has_data() to Field::hasData() » Move field_has_data() to Field::hasData()
Priority: Critical » Normal
Status: Active » Fixed

https://drupal.org/node/2021853

Status: Fixed » Closed (fixed)

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

xjm’s picture

xjm
she/her
Primary language English
commented
Issue tags: -Needs change record

Untagging. Please remove the tag when the change notification task is completed.