Fix field_test_field_attach_delete_bundle()

@Taran2L
yes, I also find that field_test_field_attach_delete_bundle() is not needed. thanks

This one seems a bit less trivial than some of the tiny syntax tweaks; we obviously want to make sure we're not just removing code from the tests because there's a bug in it which PHP 7.4 is "sensitive" about if that code is otherwise valid / valuable.

However, in this case it does look like perhaps this function never really did anything and is largely copy-pasta - hence the check against $bundle_old which isn't initialized, as the IS mentions.

AFAICS this code came from #443422-15: 'per field' storage engine and has changed very little since.

Using Xdebug to step through the hook being invoked e.g. by \EntityFieldQueryTestCase, it looks like $field = field_info_field($field_name); is always returning null - hence the Notice - and the rest of the code effectively does nothing.

The reason we're always getting null is that the $instances array is not keyed by $field_name - it comes from field_read_instances() (within field_attach_delete_bundle()) and is keyed numerically.

I _think_ changing the code like this might make it more "correct":

 function field_test_field_attach_delete_bundle($entity_type, $bundle, $instances) {
   $data = _field_test_storage_data();
 
-  foreach ($instances as $field_name => $instance) {
-    $field = field_info_field($field_name);
+  foreach ($instances as $instance) {
+    $field = field_info_field_by_id($instance['field_id']);
     if ($field['storage']['type'] == 'field_test_storage') {
       $field_data = &$data[$field['id']];
       foreach (array('current', 'revisions') as $sub_table) {
         foreach ($field_data[$sub_table] as &$row) {
-          if ($row->bundle == $bundle_old) {
+          if ($row->bundle == $bundle) {
             $row->deleted = TRUE;
           }
         }

However, the only tests in core which actually seem to use this alternative storage backend are:

• \FieldAttachStorageTestCase::testFieldAttachSaveLoadDifferentStorage
• \FieldAttachStorageTestCase::testFieldStorageDetailsAlter

It looks like these tests do result in hook_field_attach_delete_bundle() being invoked, but again it looks like field_test_field_attach_delete_bundle() is redundant, as none of the instances that are passed to it use field_test_storage.

However the hook does get invoked when other tests call field_attach_delete_bundle() and causes this Notice.

So question is should we fix it (it won't actually do anything in core's tests at least, and has never worked properly anyway), or just remove it?

In general less code is good, so yes I think I agree with nuking this from orbit.

Out of interest, let's see if my "fixed" version above passes tests, but even if it does I don't think it's really achieving much; we're not increasing test coverage within core, and if field_test_storage has been used in contrib tests (seems unlikely), this hook has never worked so it's not likely any assertions have been based on it.

Ok, interesting so tests pass with the patched version... although that only means that we've avoided the Notice because we're no longer trying to access array elements on a null.

Anyone else have any thoughts about whether there's any value in fixing this rather than blowing it away?

I would have no problem with removing this hook per #2 but if we want to address the remaining bugs around bundle / instance deletion (e.g. #1340390: "Deleted" but "inactive" fields and instances cannot be removed during cron cleanup (e.g. for uninstalled entities). and #2337139: Fields with instances attached to unknown entities are deleted on cron run) later, we may want to use / expand on these tests.

So I'm going to vote to do the fix instead, per #9

Either works in terms of avoiding the PHP 7.4 Notice.

Fixing seems the correct action to do to me too. It's a bug the changes in PHP 7.4 made evident.

RTBM, approved - let's get this in. I agree that #9 is better.

Thanks, all!

Thanks everyone!