So there's no way to return back fields after module enabled again, also this blocks #731724: Convert comment settings into a field to make them work with CMI and non-node entities which was converted to annotations

steps to reproduce
1) enable telephone module
2) add telephone field to article
3) disable and then enable telephone module
4) visit admin/structure/types/manage/article/fields - no telephone field!

Also admin/reports/fields

Notice: Undefined index: field_tel in Drupal\field_ui\FieldListController->buildRow() (line 115 of core/modules/field_ui/lib/Drupal/field_ui/FieldListController.php).
Warning: Invalid argument supplied for foreach() in Drupal\field_ui\FieldListController->buildRow() (line 115 of core/modules/field_ui/lib/Drupal/field_ui/FieldListController.php).

field_list.png

  foreach ($modules as $module => $module_info) {
    // Collect field types and storage backends exposed by the module.
    $field_types = (array) $module_handler->invoke($module, 'field_info');
    $storage_types = (array) $module_handler->invoke($module, 'field_storage_info');

    if ($field_types || $storage_types) {
      foreach ($fields as $uuid => &$field) {
        // Associate field types.
        if (isset($field_types[$field['type']]) && ($field['module'] !== $module || !$field['active'])) {
          $field['module'] = $module;
          $field['active'] = TRUE;
          $changed[$uuid] = $field;
        }

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

andypost’s picture

Issue tags: +Field API, +Entity Field API

taggin

yched’s picture

Oops, indeed. I missed that use of hook_field_info() :-(.

This check needs to switch to using getDefinitions() and the"field type" plugin manager somehow. Although I guess if the current code is not using field_info_field_types() in HEAD, it's probably because things are a bit more complex :-/

field_sync_status(), what's not to love about you...

andypost’s picture

Status: Active » Needs review
FileSize
2.15 KB

Initial attempt

andypost’s picture

Issue tags: +Needs tests

@yched what is a proper place for tests?
Patch works for me

andypost’s picture

Issue tags: -Needs tests

Also there's no way to prevent disabling of the module which fields in use

function field_system_info_alter(&$info, $file, $type) {
  // It is not safe to call field_read_fields() during maintenance mode.
  if ($type == 'module' && module_hook($file->name, 'field_info') && !defined('MAINTENANCE_MODE')) {
    $fields = field_read_fields(array('module' => $file->name), array('include_deleted' => TRUE));

swentel’s picture

I also hit a bug while trying to remove field_read_x(s) - see the interdiff in https://drupal.org/node/2018319#comment-7540395 - it's probably not entirely related to this, but yeah, that function is annoying :)

andypost’s picture

Here's test and new patch

andypost’s picture

FileSize
4.19 KB
626 bytes

And better text check

yched’s picture

Hard for me to provide a proper review right now, but patch looks reasonable.
Telephone module tests seem a weird place to put those tests though. It's generic field api behavior, should be tested in field module using test_field field type.

andypost’s picture

@yched to properly test this we need module that not hidden

yched’s picture

Ok, but then still put the test in field.module, not telephone.module, it's not code from telephone module that is being tested.
A comment explaining why it uses telephone instead of test_field would be welcome too :-)

larowlan’s picture

So this moves the test out of telephone module into field module.
First a fail, then a pass.

andypost’s picture

Great! I think this RTBC

penyaskito’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +RTBC July 1
penyaskito’s picture

This issue was RTBC and passing tests on July 1, the beginning of API freeze.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

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