#821290: Unexpected error after disabling - unistalling - re-enabling forum module added testEnableForumTaxonomyFieldDependency() to help test the odd code at the beginning of forum_enable():

  // If we enable forum at the same time as taxonomy we need to call
  // field_associate_fields() as otherwise the field won't be enabled until
  // hook modules_enabled is called which takes place after hook_enable events.
  field_associate_fields('taxonomy');

This is only one of two places field_associate_fields() is used: http://api.drupal.org/api/drupal/core!modules!field!field.module/functio...

Then, in #943772: field_delete_field() and others fail for inactive fields, the test was moved and stripped down to the point where it doesn't test this anymore.

#821290-31: Unexpected error after disabling - unistalling - re-enabling forum module:

There seems to be 4 options
* Move when modules_enabled is invoked however this seems to be a finalise process so doesn't seem to be option
* Add a call to associate fields as part of the individual install process and just clear the cache modules_enabled()
* Add a hook_enable to the taxonomy module
* Call field_associate_fields in forum_enable and add comments to explain why on earth it is happening.

Option 4 was the one chosen, but there is another possibility:
Changing forum_enable() to forum_modules_enabled().

Unfortunately, because of other changes to the module enabling process (and I think the switching from standard profile to minimal profile), re-adding the old test as-is currently fails.

Also, this blocks #1735118: Convert Field API to CMI because it attempts to change the parameters for field_associate_fields().

Comments

tim.plunkett’s picture

Title: Restore test coverage for forum_enable()'s usage of field_associate_fields() » Remove forum_enable()'s usage of field_associate_fields()
Status: Active » Needs review
StatusFileSize
new852 bytes

I now understand why the test was changed, since #943772: field_delete_field() and others fail for inactive fields now prevents taxonomy from being disabled until forum is uninstalled.

This is just a quick attempt at using hook_modules_enabled() instead.

tim.plunkett’s picture

This still has code coverage in ForumTest::testEnableForumField().

tim.plunkett’s picture

StatusFileSize
new961 bytes
new723 bytes

I added a comment about why we're using this hook instead of the old one, and a second patch removing the function call without switching the hook.

tim.plunkett’s picture

Well, crap. That disproves my theory.

In this case we're either still missing test coverage, or that's legacy code that I don't understand why it's still around.

moshe weitzman’s picture

I'm seeing all green here. I guess you expected a fail?

tim.plunkett’s picture

Issue tags: +Needs tests

Yes. Which means either that there is no test coverage, or the code is useless. Likely the former.

tim.plunkett’s picture

#3: forum-1740432-3-fail.patch queued for re-testing.

tim.plunkett’s picture

#3: forum-1740432-3-pass.patch queued for re-testing.

tim.plunkett’s picture

Priority: Major » Normal
Issue tags: -Needs tests

Now that #3 is failing as expected, this isn't major.

tim.plunkett’s picture

Status: Needs review » Closed (duplicate)
andypost’s picture

In #1907960-205: Helper issue for "Comment field" we have test failure when field-tables are exists but no field definition found. Not sure how this could be possible