Comment module causes PHP 7.4 Notices on uninstall

@Taran2L what are the notices and are they PHP7.4 specific?

@Taran2L thankyou for patch, patch in comment #2 LGTM.

Digging into this one a bit more, the issue is not that comment module tries to delete it's fields more than once, it's that comment module tries to delete its field by calling field_delete_field, which happily tries to delete the fields off of the disabled entity type: comment. This causes the notice.

Essentially the notice is caused by this code:

function field_delete_field($field_name) {
  // Delete all non-deleted instances.
  $field = field_info_field($field_name);

  if (isset($field['bundles'])) {
    foreach ($field['bundles'] as $entity_type => $bundles) {
      foreach ($bundles as $bundle) {
        $instance = field_info_instance($entity_type, $field_name, $bundle);
        field_delete_instance($instance, FALSE);
      }
    }
  }

that call to field_info_instance is returning NULL because the entity type (comment) doesn't really 'exist' any more in the running Drupal.

I think the comment in https://www.drupal.org/project/drupal/issues/998048#comment-3826740 is spot on, but the documentation hasn't been updated (we shouldn't be calling field_delete_field in an uninstall hook.) Given that this patch does remove the call though and because it does indeed iterate over all instances of the field and delete them, the field will eventually get marked for deletion either way. So it's all good!

A quick manual test suggests that it's okay to remove the call to field_delete_field('comment_body');

Steps:

$ drush site-install

Save settings on a content type e.g. /admin/structure/types/manage/article

$ drush vget comment
comment_anonymous_article: 0
comment_article: '2'
comment_default_mode_article: 1
comment_default_per_page_article: '50'
comment_form_location_article: 1
comment_page: 0
comment_preview_article: '1'
comment_subject_field_article: 1

mysql> SHOW TABLES LIKE '%comment%';
+--------------------------------+
| Tables_in_drupal7x (%comment%) |
+--------------------------------+
| comment                        |
| field_data_comment_body        |
| field_revision_comment_body    |
| node_comment_statistics        |
+--------------------------------+
4 rows in set (0.00 sec)

$ drush -y pm-disable comment && drush -y pm-uninstall comment
The following extensions will be disabled: comment
Do you really want to continue? (y/n): y
comment was disabled successfully.                                          [ok]
The following modules will be uninstalled: comment
Do you really want to continue? (y/n): y
comment was successfully uninstalled.                                       [ok]

$ drush vget comment
No matching variable found.                                              [error]

mysql> SHOW TABLES LIKE '%comment%';
Empty set (0.00 sec)

So that all looks good. However, there are still some entries left over in the db:

mysql> SELECT COUNT(1) FROM field_config WHERE field_name LIKE '%comment%';
+----------+
| COUNT(1) |
+----------+
|        1 |
+----------+
1 row in set (0.00 sec)

mysql> SELECT COUNT(1) FROM field_config_instance WHERE field_name LIKE '%comment%';
+----------+
| COUNT(1) |
+----------+
|        2 |
+----------+
1 row in set (0.00 sec)

The row in field_config is marked as "deleted" after the uninstall, whereas the two in field_name_instance are not AFAICS. I'm not sure whether that's a bug?

Either way, the outcome is the same with and without the patch.

It would be good to have a test which covers this; there isn't one yet.

D8 does this:

  // Remove the comment fields.                                                    
  $storage = \Drupal::entityTypeManager()->getStorage('field_storage_config');  
  $fields = $storage->loadByProperties(['type' => 'comment']);                     
  $storage->delete($fields);

...but D7 obviously doesn't have entityTypeManager etc..

Either way, the outcome is the same with and without the patch.

Actually that was wrong.

It's field_delete_field('comment_body'); which marks comment_body for deletion (the actual deletion typically happens on a cron run).

I'm still not sure whether it's a bug that the instances don't seem to be marked for deletion, but it seems like it might be.

I don't think we can just drop the call to field_delete_field from the uninstall hook though.

I think it is a bug that the instances are not being marked for deletion.

If we move the call to field_delete_field to the end of the uninstall hook, it looks to me like this works as it should.

The field and the instances are all marked for deletion, and the call to field_delete_field doesn't cause a Notice because the bundles have already been deleted by the time it's called, so it doesn't iterate over bundles for which field_info_instance will return null.

Let's see if the PHP 7.4 tests agree.

I am inclined to say we should add a test which verifies this (and would prove that without this change D7 has a bug here). It shouldn't be a difficult test to add.

Hmm, oh yeah - there are no tests for the uninstall, so we won't get the Notices.

I'll have a look at adding test coverage.

I wrote some tests, but AFAICS what they prove is the correctness of the comment @Steven Jones linked to above about the Drupal WTF with inactive vs. deleted field instances.

Changing the order in which the comment module's uninstall hook deletes the instances and the field does make a difference, in that if we call field_delete_field('comment_body') last, the instances do get marked for deletion (which they don't with the existing order).

However, when cron runs field_cron() calls field_purge_batch() which we'd hope would delete the instances, but they get filtered out by field_read_instances() because the field that they're associated with is now inactive:

https://git.drupalcode.org/project/drupal/-/blob/7.x/modules/field/field...

  foreach ($results as $record) {
    // Filter out instances on unknown entity types (for instance because the
    // module exposing them was disabled).
    $entity_info = entity_get_info($record['entity_type']);
    if ($include_inactive || $entity_info) {

So the instances don't get purged despite the fact they've been marked deleted = 1, which is frustrating.

It seems like we might want to specify include_deleted in that call to field_read_instances(), which looks like it would "fix" this cleanup step, but that's probably too dramatic a change to make at this stage.

I'll include the tests I came up with FWIW but the last check for the deletion of the instances is not going to pass with or without the recent patches.

I think moving the call to field_delete_field() to the end of the hook should still fix the Notice(s) which brought us here in the first place, and these tests should hopefully help prove that... but we're going to have to remove (/ tweak) the final assertion if we want the tests to pass without making more drastic changes to the fields system.

So, we expect both of these patches to fail tests. They should show, however, that the PHP 7.4 Notices go away if we move field_delete_field() to the end of the hook.

Within the scope of this issue, that's probably all we can do, so we'll need to tweak the tests so that they don't fail as a result of the orphaned field instances.

The output comparing exceptions from the tests is not very clear, but I'm fairly sure the new tests are effective at "surfacing" the Notices we're concerned with here.

It looks like moving field_delete_field() to the end of the hook reduces the number of notices overall, but also introduces two new ones:

> exception: [Notice] Line 427 of modules/image/image.module:
> exception: [Notice] Line 424 of modules/field/field.crud.inc:

These are the result of the invocation of hooks within field_delete_field() AFAICS; likely because the return value of field_info_field($field_name) is not an array with all of the expected elements e.g. $field['type'] and $field['storage']['module'].

There are quite a few more of these exceptions between modules/field/field.crud.inc and modules/image/image.module which all look fairly similar.

I'm going to review some of the other child issues in #3081386: [META] Fully support PHP 7.4 in Drupal 7 to see whether the two new ones we're introducing here would be addressed by any of those, and will come back to this issue.

We'll also want to fix the new tests here so they don't fail; I'm thinking we should file a followup (if there's not one already) pointing out the problem with orphan field instances being left in the db, and make the tests here pass e.g. by using field_read_instances() to check that the instances have been purged (it won't return the orphaned instances) or by explicitly checking that there are no instances left in the db which have not at least been flagged as deleted = 1. Neither feel great, but fixing the underlying problem is not within the scope of this issue.

Having said this was out-of-scope of this issue, it looks like making this change may fix the issue for uninstalling comment module:

 function field_purge_batch($batch_size) {
   // Retrieve all deleted field instances. We cannot use field_info_instances()
   // because that function does not return deleted instances.
-  $instances = field_read_instances(array('deleted' => 1), array('include_deleted' => 1));
+  $instances = field_read_instances(array('deleted' => 1), array('include_deleted' => 1, 'include_inactive' => 1));

...in that we don't end up with orphaned instances which have been marked for deletion but will actually never be purged because they're associated with an inactive field / module.

I am curious whether this will break other things in the test suite.

Of course, even if it does not, that doesn't mean it wouldn't break things "out in the wild", but perhaps it's not too risky a thing to do; this should only be acting on instances which have been marked as ready to be deleted, IIUC. I'm not sure what legitimate reason we'd ever have to skip purging those?

Ok, so yes it looks like this now passes with the new tests which try to ensure no cruft is left behind by the comment module's field(s).

The patch does introduce a couple of new exceptions in PHP 7.4 (but fixes several others), as noted in #19 but I think we need to fix quite a few of those between modules/field/field.crud.inc and modules/image/image.module which all look fairly similar. I am not certain those are being addressed in any other PHP 7.4 issue that I've come across yet.

We could fix the ones that this patch causes here, and the rest in another issue... or perhaps create one new issue for all of them that we know about (including the two new ones the patches here introduce). Not much different either way, but I don't really want to commit a patch in this issue which introduces new exceptions.

Added fixes for the two new exceptions in field and image modules (see interdiff).

I would appreciate any other reviews of #23 before I mark it as RTBC / Pending Drupal 7 commit.

You could certainly argue trying to fix the orphaned field instances problem is out of scope of addressing PHP 7.4 compatibility, but in some cases the language becoming stricter is revealing underlying bugs.

I think there's little question that the existing functionality is broken, and it feels like it would be a hack to try to resolve the PHP 7.4 exceptions in this uninstall hook without addressing the brokenness to some extent.

With the additional tests this now seems a fairly safe change to me.

FWIW comparing all of the fails/exceptions from the latest test on #23 it looks like an improvement on the current "baseline" test at #3081386-70: [META] Fully support PHP 7.4 in Drupal 7 although a diff of the results isn't that easy to read.

baseline:

l=https://www.drupal.org/pift-ci-job/1640782 ; curl -s $l | grep -oE '(exception|fail): .* Line [0-9]* of .*:' | perl -pe 's#<.*?>##g' | sort | uniq -c | sort -rn
     26 exception: [Notice] Line 782 of modules/field/field.crud.inc:
     16 exception: [Notice] Line 926 of modules/forum/forum.module:
     16 exception: [Notice] Line 460 of modules/field/tests/field_test.storage.inc:
     13 exception: [Notice] Line 781 of modules/field/field.crud.inc:
     13 exception: [Notice] Line 774 of modules/field/field.crud.inc:
     13 exception: [Notice] Line 773 of modules/field/field.crud.inc:
     13 exception: [Notice] Line 772 of modules/field/field.crud.inc:
      7 exception: [Notice] Line 481 of modules/image/image.module:
      7 exception: [Notice] Line 480 of modules/image/image.module:
      3 fail: [PHP Deprecated] Line 222 of modules/search/search.extender.inc:
      2 exception: [Notice] Line 1088 of modules/field_ui/field_ui.admin.inc:
      2 exception: [Notice] Line 1029 of modules/field_ui/field_ui.admin.inc:
      1 fail: [Completion check] Line 2994 of modules/system/system.test:
      1 exception: [Notice] Line 633 of modules/field/field.crud.inc:
      1 exception: [Notice] Line 617 of modules/field/field.crud.inc:

test from #23

l=https://www.drupal.org/pift-ci-job/1658344 ; curl -s $l | grep -oE '(exception|fail): .* Line [0-9]* of .*:' | perl -pe 's#<.*?>##g' | sort | uniq -c | sort -rn
     16 exception: [Notice] Line 926 of modules/forum/forum.module:
     16 exception: [Notice] Line 460 of modules/field/tests/field_test.storage.inc:
     10 exception: [Notice] Line 784 of modules/field/field.crud.inc:
      5 exception: [Notice] Line 783 of modules/field/field.crud.inc:
      5 exception: [Notice] Line 776 of modules/field/field.crud.inc:
      5 exception: [Notice] Line 775 of modules/field/field.crud.inc:
      5 exception: [Notice] Line 774 of modules/field/field.crud.inc:
      3 fail: [PHP Deprecated] Line 222 of modules/search/search.extender.inc:
      2 exception: [Notice] Line 1088 of modules/field_ui/field_ui.admin.inc:
      2 exception: [Notice] Line 1029 of modules/field_ui/field_ui.admin.inc:
      1 fail: [Completion check] Line 2994 of modules/system/system.test:
      1 exception: [Notice] Line 635 of modules/field/field.crud.inc:
      1 exception: [Notice] Line 619 of modules/field/field.crud.inc:
      1 exception: [Notice] Line 481 of modules/image/image.module:
      1 exception: [Notice] Line 480 of modules/image/image.module:

I'll address the overall progress on these tests in the parent issue.

(cross-posted with your last comment)

Thanks @Taran2L - it's probably a good idea to address the field bug in a separate issue.

However, if I follow your steps I get a different result after the uninstall:

$ drush -y si standard

$ drush sqlq "SHOW TABLES LIKE '%comment%';"
comment
field_data_comment_body
field_revision_comment_body
node_comment_statistics

$ drush -y dis comment && drush -y pmu comment

$ drush sqlq "SHOW TABLES LIKE '%comment%';"

$ drush sqlq "SELECT field_name,deleted FROM field_config WHERE field_name LIKE '%comment%';"
comment_body    1

$ drush sqlq "SELECT field_name,deleted FROM field_config_instance WHERE field_name LIKE '%comment%';"
comment_body    0
comment_body    0

...which is what the test failures in #16 illustrate, e.g. https://www.drupal.org/pift-ci-job/1658015

That's how I ended up going down the rabbit hole of addressing that problem here.

Not sure how/why you're seeing a different outcome to that with your manual test?

Ok, but #27 (which I the patch I think you meant) also makes the change to field_purge_batch() so that the inactive instances aren't skipped and do get purged from the db.

Interesting that the test can pass with the call to field_delete_field() completely removed :) Will take a closer look.

We need to decide whether we're going to address the purging of instances here.

In fact without the call to field_delete_field() in the uninstall hook, the field and the instances get marked for deletion, but the instances won't actually be removed on cron (unless we change field_purge_batch()).

So if we want to separate the fixes but retain the new tests, we could change the test so that it'll accept instances being left over if they're marked for deletion.

We can then address the instances not being purged separately.

I think that's what these patches should show - the test only one should fail as with the field_delete_field() still in the uninstall hook, the instances are never marked for deletion. With it taken out, they are marked as deleted (but won't actually be purged by cron - we'll deal with that problem elsewhere though).

Ok, so I think that's the result we expected / wanted but this patch doesn't make the change to field_purge_batch() which we've said should be dealt with elsewhere.

Looks like this resolves the vast majority of the field.crud.inc exceptions:

$ l=https://www.drupal.org/pift-ci-job/1640782 ; r=https://www.drupal.org/pift-ci-job/1660447 ; curl -s $l | grep -oE '(fail|exception): .* Line [0-9]* of .*:' | perl -pe 's#<.*?>##g' | sort | uniq -c | sort -rn | diff - <(curl -s $r | grep -oE '(fail|exception): .* Line [0-9]* of .*:' | perl -pe 's#<.*?>##g' | sort | uniq -c | sort -rn) | grep --color=none '<\|>'

<      26 exception: [Notice] Line 782 of modules/field/field.crud.inc:
<      13 exception: [Notice] Line 781 of modules/field/field.crud.inc:
<      13 exception: [Notice] Line 774 of modules/field/field.crud.inc:
<      13 exception: [Notice] Line 773 of modules/field/field.crud.inc:
<      13 exception: [Notice] Line 772 of modules/field/field.crud.inc:
<       7 exception: [Notice] Line 481 of modules/image/image.module:
<       7 exception: [Notice] Line 480 of modules/image/image.module:

How does #30 look to you @Taran2L ?

Great, thanks for the review and for all your work on these and other PHP 7.4 issues @Taran2L

RTBM - thanks all!

Thanks everyone!