Follow-up of #1957748: hook_field_prepare_view() is passed empty field values for EntityNG Entities

Right now, only field_attach_view() and field_attach_prepare_view() functions make sure they are working with a BC entity, the others have to be called with one, so the caller needs to make sure.

It would be easier to convert and later on remove it again when the other field attach functions would ensure this, as there are dozens if not hundreds of direct calls t those functions in the field tests, that I am converting in #1822000: Remove Drupal\field_test\Plugin\Entity\Type\TestEntity in favor of EntityTest.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Berdir’s picture

Status: Active » Needs review
FileSize
7.48 KB

Easy as that... I think ;)

Status: Needs review » Needs work

The last submitted patch, ensure-bc-1974474-1.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
594 bytes
7.48 KB

Well. me = fail.

Status: Needs review » Needs work

The last submitted patch, ensure-bc-1974474-3.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
851 bytes
7.37 KB

I've messed up field_attach_load(), this should be better.

Berdir’s picture

Issue tags: +Entity Field API

Tagging.

dawehner’s picture

+++ b/core/modules/field/field.attach.incundefined
@@ -821,6 +821,9 @@ function _field_invoke_widget_target() {
+  // Enable BC if necessary.

@@ -892,6 +895,11 @@ function field_attach_load($entity_type, $entities, $age = FIELD_LOAD_CURRENT, $
+  // Enable BC if necessary.

@@ -1028,6 +1036,9 @@ function field_attach_load_revision($entity_type, $entities, $options = array())
+  // Enable BC if necessary.

@@ -1077,6 +1088,9 @@ function field_attach_validate(EntityInterface $entity, array $options = array()
+  // Enable BC if necessary.

@@ -1114,6 +1128,9 @@ function field_attach_form_validate(EntityInterface $entity, $form, &$form_state
+  // Enable BC if necessary.

@@ -1135,6 +1152,9 @@ function field_attach_extract_form_values(EntityInterface $entity, $form, &$form
+  // Enable BC if necessary.

@@ -1156,6 +1176,9 @@ function field_attach_presave($entity) {
+  // Enable BC if necessary.

@@ -1197,6 +1220,9 @@ function field_attach_insert(EntityInterface $entity) {
+  // Enable BC if necessary.
...

I guess it would be helpful to explain why it's done here.

Berdir’s picture

Can easily change that within the patch, but to what? I just took the existing comment and copied to all other places.

Berdir’s picture

Title: Make sure within field_attach_*() that the are working with a BC entity » Make sure within field_attach_*() that we are working with a BC entity
dawehner’s picture

What about: Enable BC if the entity is NG.?

Berdir’s picture

Doesn't seem to explain something either :)

"Field API needs a BC mode entity" or "Ensure we are working with a BC mode entity" maybe?

dawehner’s picture

I like the second one.

Berdir’s picture

Here's a patch with that, then :) No interdiff, not really necessary I think ;)

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Thanks!

dawehner’s picture

A somehow related issue is #1977464: The custom field which add for comment con't show in views which caused a bug with all EntityNGs, though I think it's valid to have these two issues separated.

alexpott’s picture

#13: ensure-bc-entity-1974474-12.patch queued for re-testing.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 0be9849 and pushed to 8.x. Thanks!

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