Problem/Motivation

You cannot save entity_view_display configuration for the menu_link_content entity type.
During the saving process, a PluginNotFoundException exception is thrown regarding the weight field.

This bug was introduced in #2271419: Allow field types, widgets, formatters to specify config dependencies. When base field definitions were updated, fields formatted for the weight field were replaced with number instead of number_integer.

Steps to reproduce are described in comment #39

The reason this bug does not throw exception previously is that the weight field is not used in the rendering process using the FieldFormatter plugin.

Workaround:
You can avoid the bug using a custom implementation of hook_entity_base_field_info_alter().
Details are described in comment #3.

Proposed resolution

Replace the incorrect integer FieldFormatter id with the correct number_integer FieldFormatter id.

Remaining tasks

The one-line patch in comment #2 fixes the problem.

The hard part is creating tests.

The single-line fix shows no effect on testing results. That means that existing tests do not cover the FieldFormatter and FieldWidget id in the base field definition of entities from core modules.

Based on the comment in #42, later commments suggested using the one-line patch from #2 and moving the tests to the follow-up issue #2915024: FieldWidget and FieldFormatter usage test. Comment #57 clarified that this was not the right thing to do, so later comments restored the tests.

Update path:

Comment #42 suggets we might need to test that this issue fixes the upgrade issues reported in #2883949: The "integer" plugin does not exist.. Perhaps we can leave that issue open for further discussion. (See comment #42 for details. This summary may not be accurate.)

@voleger points out,

Manually reproduced the bug on Drupal 8.0.0. That means no broken data were recorded into the active configuration, so we don't need any update path for this fix.

User interface changes

No changes.

API changes

No changes

Data model changes

No changes.

Original report by voleger

Wrong field view display option type "integer" for weight field. It throw PluginNotFoundException in some cases.
Proposed resolution: Change type to correct value "number_integer"

CommentFileSizeAuthor
#78 menu_link_content-fix_used_widget_formatter_id-2903161-66-8.5.x.patch7.49 KBbenjifisher
#73 interdiff-66-71.txt1.19 KBbenjifisher
#71 menu_link_content-fix_used_widget_formatter_id-2903161-71-8.4.x.patch7.23 KBvoleger
#71 menu_link_content-fix_used_widget_formatter_id-2903161-71-8.4.x-test-only.patch6.61 KBvoleger
#66 menu_link_content-fix_used_widget_formatter_id-2903161-66-8.5.x.patch7.49 KBvoleger
#66 menu_link_content-fix_used_widget_formatter_id-2903161-66-8.5.x-test-only.patch6.87 KBvoleger
#66 interdiff-2903161-63-66.txt2.57 KBvoleger
#63 menu_link_content-fix_used_widget_formatter_id-2903161-63-8.5.x.patch6.14 KBvoleger
#63 menu_link_content-fix_used_widget_formatter_id-2903161-63-8.5.x-test-only.patch5.52 KBvoleger
#63 interdiff-2903161-58-63.txt8.23 KBvoleger
#58 menu_link_content-fix_used_widget_formatter_id-2903161-58-8.5.x.patch10.65 KBvoleger
#58 menu_link_content-fix_used_widget_formatter_id-2903161-58-8.5.x-test-only.patch10.03 KBvoleger
#58 interdiff-2903161-30-58.txt14.77 KBvoleger
#2 menu_link_content-wrong_field_view_display_option_type-2903161-2-8.5.x.patch638 bytesvoleger
#39 Selection_030.png56.09 KBvoleger
#39 Selection_029.png21.5 KBvoleger
#33 menu_link_content-wrong_field_view_display_option_type-2903161-30-8.5.x-test-only.patch5.08 KBvoleger
#30 menu_link_content-wrong_field_view_display_option_type-2903161-30-8.5.x.patch5.7 KBvoleger
#30 interdiff-2903161-23-30.txt1.49 KBvoleger
#23 menu_link_content-wrong_field_view_display_option_type-2903161-23-8.5.x.patch5.32 KBvoleger
#23 interdiff-2903161-20-23.txt5.46 KBvoleger
#20 interdiff-2903161-18-20.txt1009 bytesvoleger
#18 interdiff-2903161-16-18.txt3.38 KBvoleger
#16 interdiff-2903161-2-16.txt4.27 KBvoleger
#23 menu_link_content-wrong_field_view_display_option_type-2903161-23-8.5.x-test-only.patch4.69 KBvoleger
#20 menu_link_content-wrong_field_view_display_option_type-2903161-20-8.5.x.patch5.05 KBvoleger
#18 menu_link_content-wrong_field_view_display_option_type-2903161-18-8.5.x.patch5.05 KBvoleger
#16 menu_link_content-wrong_field_view_display_option_type-2903161-16-8.5.x-test-only.patch5.06 KBvoleger
#14 menu_link_content-wrong_field_view_display_option_type-2903161-14-8.5.x-test-only.patch4.44 KBvoleger
#10 menu_link_content-wrong_field_view_display_option_type-2903161-10-8.5.x-test-only.patch2.83 KBvoleger
#7 menu_link_content-wrong_field_view_display_option_type-2903161-7-8.5.x-test-only.patch4.35 KBvoleger
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

voleger created an issue. See original summary.

voleger’s picture

That code help me in module to avoid PluginNotFoundException:

/**
 * Implements hook_entity_base_field_info_alter().
 *
 * @see hook_entity_base_field_info_alter()
 * @see https://www.drupal.org/node/2346329
 */
function menu_item_extras_entity_base_field_info_alter(&$fields, EntityTypeInterface $entity_type) {
  /** @var \Drupal\Core\Field\BaseFieldDefinition[] $fields */
  if ($entity_type->id() === 'menu_link_content' && isset($fields['weight'])) {
    $options = $fields["weight"]->getDisplayOptions('view');
    // Lets check if field definition is buggy.
    if ($options['type'] === 'integer') {
      $options['type'] = 'number_integer';
      // @TODO: This should be fixed in the menu_link_content core module.
      $fields['weight']->setDisplayOptions('view', $options);
    }
  }
}

I guess it also related to #2895464: The "integer" plugin does not exist. issue.

voleger’s picture

tacituseu’s picture

Priority: Normal » Major

Fix looks good, at the very least it is a Major.

andypost’s picture

Issue tags: +Needs tests

@voleger please provide steps to reproduce & to add test

voleger’s picture

This is a patch with the test only.
Not sure about correct placement and naming.
But this test enables all modules with Entity Type definitions, then load their Base Field Definitions, then check if it possible to load required formatter and widget plugins using types from display options of loaded Base Field Definitions.

Status: Needs review » Needs work
tacituseu’s picture

This part:
&& is_readable($module->getPath() . '/src/Entity')
is most likely responsible for tripping on "link_default" and "text_textfield" as neither of link nor text defines an entity.

voleger’s picture

in #7 test I tried to check all defined entity types. Who knew, maybe there are some similar problems?
Another iteration of the test:
Testing only menu_link_content module now. Loading "menu_link_content" entity type definition and checking if plugins exist that used in field display options of base field definitions.

voleger’s picture

Status: Needs work » Needs review
tacituseu’s picture

I like the test in #7, I think checking all of them is the right way to go, it acted like link and text modules were not enabled for taxonomy, menu_link_content, shortcut.
Thought it might have exposed bad dependencies but checked that and all of them have them defined properly, so either $this->enableModules() isn't enforcing dependencies or there is something else going on here.

tacituseu’s picture

   * @param array $modules
   *   A list of modules to enable. Dependencies are not resolved; i.e.,
   *   multiple modules have to be specified with dependent modules first.
   *   The new modules are only added to the active module list and loaded.
   */
  protected function enableModules(array $modules) { 
voleger’s picture

I got it)
I will also check if the module has field plugin definitions.
is_readable($module->getPath() . '/src/Plugin/Field')
So here is new test only patch:

The last submitted patch, 10: menu_link_content-wrong_field_view_display_option_type-2903161-10-8.5.x-test-only.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

voleger’s picture

voleger’s picture

voleger’s picture

So here we have working test without fixes in #14
and test with fixes in #18.
Latest version of patch is in #20.

tacituseu’s picture

Status: Needs review » Needs work

1. + * Tests the availability of field plugin definitions.
2.
+ $this->assertEmpty($plugin_not_found, implode(" ", $plugin_not_found));
why the implode() ? instead of separate:

$this->fail(sprintf('PluginNotFoundException here for "%s" entity type in "%s" field view display options. Original message: %s', $entity_type_id, $field_id, $e->getMessage()));

Status: Needs review » Needs work
voleger’s picture

Status: Needs work » Needs review
tacituseu’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me.

voleger’s picture

voleger’s picture

tacituseu’s picture

Alternatively, instead of enabling all modules, it can be done like in #2901692-18: FieldDefinitionIntegrityTest does not respect module dependencies.

voleger’s picture

voleger’s picture

Back to RTBC?

tacituseu’s picture

Status: Needs review » Reviewed & tested by the community

Test only patch with the latest change would be useful too, otherwise all good.

voleger’s picture

Status: Needs review » Needs work
tacituseu’s picture

Status: Needs work » Reviewed & tested by the community
Berdir’s picture

Oh, is this (one of the?) things that are causing the reported update errors on the missing integer plugin?

Did someone test what happens when this information is saved into a view display, which should happen when you create a configurable field on menu link entities. Does a cache clear update that?

voleger’s picture

I can't imagine how to reproduce the case when inexistent plugin recorded into the entity view display in active configuration. For now is not possible to save display mode for menu_link_content, because of PluginNotFoundException. Plugin manager will try to load plugins that used by field definitions before entity view display will be created.

xjm’s picture

Oh whoops. That's really subtle. It makes me wonder if we have the same bug anywhere else in core because I totally would not have caught this in a code review.

I blamed the changed line and it was introduced in #2301239: MenuLinkNG part1 (no UI or conversions): plugins (static + MenuLinkContent) + MenuLinkManager + MenuTreeStorage and not changed since. If it is indeed what was causing #2883949: The "integer" plugin does not exist. I wonder what new code would have been exercising it that wasn't before?

Nice work on the tests @voleger; good idea to check other definitions as well. What are the manual steps under which you normally encounter this bug? Is it only when using https://www.drupal.org/project/menu_item_extras?

+++ b/core/modules/field/tests/src/Kernel/FieldDefinitionAvailabilityTest.php
@@ -0,0 +1,121 @@
+  protected function checkDisplayOption($entity_type_id, $field_id, BaseFieldDefinition $field_definition, DiscoveryInterface $plugin_manager, $display_context) {
+    $form_display_options = $field_definition->getDisplayOptions($display_context);
+    if (!empty($form_display_options) && !empty($form_display_options['type'])) {
+      try {
+        $plugin_manager->getDefinition($form_display_options['type']);
+      }
+      catch (PluginNotFoundException $e) {
+        $this->fail(sprintf(
+          'PluginNotFoundException here for "%s" field %s display options of "%s" entity type. Original message: %s',
+          $field_id,
+          $display_context,
+          $entity_type_id,
+          $e->getMessage()
+        ));

I'm wondering whether we actually need this helper method (versus just letting the test fail when the exception is thrown). Setting NR to discuss that at least.

This seems really likely to trip people up in the future and to be wrong elsewhere. Compare:

[ibnsina:maintainer | Tue 15:08:04] $ grep -rl "'type' => 'number_integer'" *
core/modules/field/src/Tests/Number/NumberFieldTest.php
core/modules/field/tests/src/Kernel/Plugin/migrate/source/d7/FieldInstancePerFormDisplayTest.php
core/modules/migrate_drupal/tests/fixtures/drupal6.php
core/modules/migrate_drupal/tests/fixtures/drupal7.php
core/modules/rdf/tests/src/Kernel/Field/NumberFieldRdfaTest.php

versus:

[ibnsina:maintainer | Tue 15:30:31] $ grep -rl "'type' => 'integer'" *
core/modules/field/src/Tests/Number/NumberFieldTest.php
core/modules/field/tests/src/Kernel/FieldDataCountTest.php
core/modules/field/tests/src/Kernel/FieldImportDeleteUninstallTest.php
core/modules/menu_link_content/src/Entity/MenuLinkContent.php
core/modules/migrate/src/Plugin/MigrateDestinationInterface.php
core/modules/migrate/src/Plugin/MigrateSourceInterface.php
core/modules/migrate/tests/modules/migrate_high_water_test/src/Plugin/migrate/source/HighWaterTest.php
core/modules/migrate/tests/modules/migrate_query_batch_test/src/Plugin/migrate/source/QueryBatchTest.php
core/modules/migrate/tests/src/Kernel/MigrateBundleTest.php
core/modules/migrate/tests/src/Kernel/MigrateEmbeddedDataTest.php
core/modules/migrate/tests/src/Kernel/MigrateEntityContentBaseTest.php
core/modules/migrate/tests/src/Kernel/MigrateRollbackEntityConfigTest.php
core/modules/migrate/tests/src/Kernel/MigrateRollbackTest.php
core/modules/migrate/tests/src/Unit/MigrateSqlIdMapEnsureTablesTest.php
core/modules/migrate/tests/src/Unit/SqlBaseTest.php
core/modules/migrate_drupal/tests/src/Kernel/d6/EntityContentBaseTest.php
core/modules/node/tests/modules/node_access_test/node_access_test.module
core/modules/shortcut/src/Plugin/migrate/destination/ShortcutSetUsers.php
core/modules/shortcut/src/Plugin/migrate/source/d7/ShortcutSetUsers.php
core/modules/taxonomy/tests/src/Kernel/Migrate/MigrateTaxonomyTermStubTest.php
core/modules/user/src/Plugin/migrate/source/d6/ProfileFieldValues.php
core/modules/user/src/Plugin/migrate/source/d6/User.php
core/modules/user/src/Plugin/migrate/source/d7/User.php
core/modules/user/tests/src/Kernel/Migrate/MigrateUserAdminPassTest.php
core/modules/views/tests/src/Kernel/Handler/FieldFieldTest.php
core/modules/views/tests/src/Kernel/Handler/SortTranslationTest.php
core/modules/views/tests/src/Kernel/QueryGroupByTest.php
core/modules/views_ui/tests/src/Functional/ReportFieldsTest.php
core/tests/Drupal/KernelTests/Core/Config/ConfigSchemaTest.php
core/tests/Drupal/KernelTests/Core/Entity/EntityQueryAggregateTest.php
core/tests/Drupal/KernelTests/Core/TypedData/TypedDataTest.php
core/tests/Drupal/Tests/Core/Entity/EntityFieldManagerTest.php

#2218199: Move email and number field types to \Drupal\Core\Field, remove modules seems to have created a disconnect between the field type and the formatter, when we made number a core field type.

Compare
core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/IntegerFormatter.php
versus
core/lib/Drupal/Core/Field/Plugin/Field/FieldType/IntegerItem.php

I wonder if we can fix the naming to be what people expect, with a deprecated wrapper or such?

voleger’s picture

FileSize
21.5 KB
56.09 KB

With that bug, I faced during next steps (it requires some code):
1. Set "bundle_entity_type" and "field_ui_base_route" for "menu_link_content" entity type defnition and set "bundle_of" for "menu" entity type defnition:

/**
 * Implements hook_entity_type_build().
 */
function menu_item_extras_entity_type_build(array &$entity_types) {
    $content_entity = 'menu_link_content';
    // Set entity type to be bundled.
    /** @var \Drupal\Core\Entity\ContentEntityTypeInterface $mlc */
    $mlc = $entity_types[$content_entity];
    $mlc->set('bundle_entity_type', 'menu');
    $mlc->set('field_ui_base_route', 'entity.menu.edit_form');
    // Set entity to be a bundle entity type for previous entity.
    /** @var \Drupal\Core\Config\Entity\ConfigEntityTypeInterface $menu */
    $menu = $entity_types['menu'];
    $menu->set('bundle_of', $content_entity);
}

Cache clear and entity update.

2a. Go to 'Manage fields' tab (for example "/admin/structure/menu/manage/main/fields");
3a. Try to add field:

The "integer" plugin does not exist.

2b. Go to 'Manage display' tab (for example "/admin/structure/menu/manage/main/display");
3b. Try to save view display:

PluginNotFoundException: The "integer" plugin does not exist.

voleger’s picture

In both situations fail message become before saving/updating entity view display. In the result - entity_view_display can't be saved in active config.

voleger’s picture

The helper method is needed because the original exception message is not enough to completely understand where is that problem.
The lack of information is the reason why there so many issues created. Without understanding why an exception is thrown, the situation will not change. So let's leave a little bit of information in the tests.

Berdir’s picture

Status: Needs review » Reviewed & tested by the community

Ok, so it fails already before saving, which means we don't need an update path. People did report this on updating to 8.3 for example, so I'm wondering if it was possible to save it in an earlier version. But to fix *that*, we'd actually have to inject it into/before the update function that resaves those entities (which might actually work if it's a post update hook).

However, I'm not sure if it's worth doing that and it might not be trivial. Maybe we can update those postponed issues, reference this and then discuss there whether we need an upgrade path for that.

Re the naming, yes, that the name is different is a bit unfortunate, but that's actually quite common to have mismatches between field types, widgets and formatter plugin ID's, e.g. link has link_default widget and link formatter, string has string_textfield widget and string as formatter, string_long has string_textarea widget and basic_string as default formatter. I don't think we need to change that, we don't really have a BC system for plugin ID's. But maybe instead we could add stricter, earlier validation, so that when you use an invalid type when defining a base field, it throws an exception immediately with helpful context?

In that sense, I also think the exception handling in the test is useful, as it does tell you which entity/field this happens on.

Long story short, I'd vote to get this in asap, with those possible follow-ups/updates on related existing issues.

xjm’s picture

Alright, would very much like to see those followups so we can fix this one and hopefully fix some of the fatals, but I do want to make sure we have for checking for more and planning for more in the long term. Thanks @Berdir and @voleger.

xjm’s picture

Issue tags: +Needs followup

Can we file the followups descreibed in #42? It would really help for reviewing and defining the scope of this patch, because otherwise the scope feels like "look at everything called integer and number_integer in all of core.

xjm’s picture

Status: Reviewed & tested by the community » Needs review

For me we need the followups before reviewing and committing this, given that similar bugs may be causing other criticals.

voleger’s picture

  • \Drupal\Core\Field\Plugin\Field\FieldType\IntegerItem::class plugin has id integer;
  • \Drupal\Core\Field\Plugin\Field\FieldFormatter\IntegerFormatter::class plugin has id number_integer;
  • \Drupal\Core\Field\Plugin\Field\FieldWidget\NumberWidget::class plugin has id number;

In the provided test we check usage of FieldFormatter and FieldWidget plugins by core entity type definitions.
So it does not mean that we should everywhere replace integer by number_integer.
The case with menu_link_content entity type show that somehow in configuration placed usage of FieldType instead of FieldFormatter plugin.

voleger’s picture

Maybe it will be better to add some validation for setDisplayOptions method in \Drupal\Core\Field\BaseFieldDefinition::class ?

voleger’s picture

I manually reproduced steps from #39 using Drupal 8.0.0. The same behaviour.
So I don't think that we should provide an upgrade path for that.

voleger’s picture

voleger’s picture

In #2271419: Allow field types, widgets, formatters to specify config dependencies updated usage related FieldWidget plugin id, but forgot to update usage FieldFormatter plugin id.
That's the reason of this bug.
Commit http://cgit.drupalcode.org/drupal/commit/?id=1508939
Diff of changes in menu_link_content http://cgit.drupalcode.org/drupal/diff/core/modules/menu_link_content/src/Entity/MenuLinkContent.php?id=1508939

voleger’s picture

voleger’s picture

Discussed that issue with @andypost

So I create the followup issue that shows wrong used plugin id in entity base field definition.
That incorrect value used from 8.0.x-dev version of Drupal core.
Plugin system does not allow to save that incorrect value to the entity view display. That was manually tested on 8.0.0 version of Drupal core.

To make possible to pass the test from #2915024: FieldWidget and FieldFormatter usage test is the commit a patch from #2 comment.

benjifisher’s picture

@volegar:

Please update the issue summary. Why are all the patches hidden except the one from Comment #2? Is that because the tests will be added in a follow-up issue? Also, the comment in #3 looks like a work-around, and that should be mentioned in the issue summary.

In short: if you want someone to review the issue, then make it easier.

Also, remember to un-assign yourself when you are done.

P.S. If the tests are not to be included, then it would help to have steps to reproduce added to the summary.

voleger’s picture

Status: Needs review » Needs work

Thanks for the comment.

This issue shows that actually validation mechanism is missed in the field component of Drupal core. That's why we have such a bug since 8.0.x-dev.

Anyway, I will update the issue summary.

voleger’s picture

Title: Wrong field view display option type "integer" for weight field » Used incorrect FieldFormatter id for "weight" field in base field definition display options.
Assigned: voleger » Unassigned
Issue summary: View changes
Issue tags: -Needs issue summary update

Updated summary.

andypost’s picture

Title: Used incorrect FieldFormatter id for "weight" field in base field definition display options. » Fix incorrect FieldFormatter id for "weight" field in base field definition in display options
Status: Needs work » Reviewed & tested by the community

Let's make title actionable & patch #2 fix that, also there's follow-up for test for all base fields #2915024: FieldWidget and FieldFormatter usage test

xjm’s picture

Status: Reviewed & tested by the community » Needs work

Automated test coverage for bug fixes is a Drupal core gate. We should not scope test coverage to followup issues. Please update the shown files on the issue to show the correct patch; #2 is lacking the test coverage that is required for this to be committed. Thanks!

andypost’s picture

Status: Needs review » Reviewed & tested by the community

RTBC again

I thought that the issue will add specific test for menu link entity only but having test coverage for all fields looks impressive!

benjifisher’s picture

Status: Reviewed & tested by the community » Needs work

This looks like a really good piece of work, but I think there are some things that should be fixed. I am trying to save one of the core committers the trouble of pointing these out.

1. Do we really need this?

@@ -14,6 +19,8 @@
 class FieldDefinitionIntegrityTest extends KernelTestBase {
 
   /**
+   * Modules to enable.
+   *
    * @var array
    */
   public static $modules = ['system'];

I am not sure: do we use {@inheritdoc} for properties or just for methods? If you want to leave it in, I will not insist.

2. Nice job of DRY (It would have been easier to copy and paste the code) but I think we can do better:

  /**
   * Enable modules and them dependencies by defined filter.
   *
   * @param callable $filter_callback
   *   Callback function that get \Drupal\Core\Extension\Extension object
   *   as the first argument.
   *
   * @see \Drupal\Core\Extension\Extension
   * @see array_filter();
   */
  protected function enableModulesByFilter(callable $filter_callback) {
    $modules = system_rebuild_module_data();
    $modules = array_filter($modules, $filter_callback);
    // Gather the dependencies of the modules.
    $dependencies = NestedArray::mergeDeepArray(array_map(function (Extension $module) {
      return array_keys($module->requires);
    }, $modules));
    $modules = array_unique(NestedArray::mergeDeep(array_keys($modules), $dependencies));

    $this->enableModules($modules);
  }

This method is called twice: the second time is this:

    $this->enableModulesByFilter(function (Extension $module) {
      // Filter contrib, hidden, already enabled modules and modules in the
      // Testing package.
      return ($module->origin === 'core'
        && empty($module->info['hidden'])
        && $module->status == FALSE
        && $module->info['package'] !== 'Testing'
        && is_readable($module->getPath() . '/src/Entity'));
    });

There are a few reasons I do not like this method.

First, I am an old-fashioned guy, with no experience in functional programming, and functions that accept callable arguments are just more confusing than functions that accept string and return array.

Second, the enableModulesByFilter() method captures only about half the complexity. Each of the two places that call it need to create the callable argument.

Third, this method violates the single-responsibility principle. It generates a list of modules and then it enables those modules. I like functions that return information better than functions that have global effects.

I think it would be clearer if you defined a function like this:

  /**
   * Find modules with a specified subdirectory.
   *
   * @param string $subdirectory
   *   The required path, relative to the module directory.
   *
   * @return array
   *   A list of module names satisfying these criteria:
   *   - provided by core
   *   - not hidden
   *   - not already enabled
   *   - not in the Testing package
   *   - containing the required $subdirectory
   *   and all modules required by any of these modules.
   *
   * @see \Drupal\Core\Extension\Extension
   * @see array_filter();
   */
  protected function modulesWithSubdirectory($subdirectory) {
    // ...
  }

and then you could use the function like this:

  public function testFieldPluginDefinitionAvailability() {
    $modules = $this->modulesWithSubdirectory('/src/Entity');
    $this->enableModules($modules);
    // ...
  }

3. This looks like an unrelated fix:

     /** @var \Drupal\Component\Plugin\Discovery\DiscoveryInterface $field_type_manager */
-    $field_type_manager = \Drupal::service('plugin.manager.field.field_type');
+    $field_type_manager = $this->container->get('plugin.manager.field.field_type');
 
     /** @var \Drupal\Component\Plugin\Discovery\DiscoveryInterface $field_type_manager */
-    $field_formatter_manager = \Drupal::service('plugin.manager.field.formatter');
+    $field_formatter_manager = $this->container->get('plugin.manager.field.formatter');
 
     /** @var \Drupal\Component\Plugin\Discovery\DiscoveryInterface $field_type_manager */
-    $field_widget_manager = \Drupal::service('plugin.manager.field.widget');
+    $field_widget_manager = $this->container->get('plugin.manager.field.widget');

Yes, that is the right way to do it, but unless it affects the results of the tests, I think you should leave this for another issue. Maybe that issue is already out there, and someone is trying to fix it in all of Drupal core. If so, then we do not want to break that patch.

4. Same idea:

@@ -76,46 +75,138 @@ public function testFieldPluginDefinitionIntegrity() {
     foreach ($field_type_manager->getDefinitions() as $definition) {
       // Test default field widgets.
       if (isset($definition['default_widget'])) {
-        if (in_array($definition['default_widget'], $available_field_widget_ids)) {
-          $this->pass(sprintf('Field type %s uses an existing field widget by default.', $definition['id']));
-        }
-        else {
-          $this->fail(sprintf('Field type %s uses a non-existent field widget by default: %s', $definition['id'], $definition['default_widget']));
-        }
+        $this->assertTrue(
+          in_array($definition['default_widget'], $available_field_widget_ids),
+          sprintf('Field type %s uses an existing field widget by default.', $definition['id'])
+        );
       }

and three similar changes. Unless we really need this change now, we should leave it out. Leaving it out will make the patch easier to review and get it accepted sooner.

5. Maybe PHP's empty() is one of my pet peeves.

  protected function checkDisplayOption($entity_type_id, $field_id, BaseFieldDefinition $field_definition, DiscoveryInterface $plugin_manager, $display_context) {
    $display_options = $field_definition->getDisplayOptions($display_context);
    if (!empty($display_options) && !empty($display_options['type'])) {

This is exactly the same as

  protected function checkDisplayOption($entity_type_id, $field_id, BaseFieldDefinition $field_definition, DiscoveryInterface $plugin_manager, $display_context) {
    $display_options = $field_definition->getDisplayOptions($display_context);
    if (!empty($display_options['type'])) {

The difference between if (!empty($display_options['type'])) and if ($display_options['type']) is that the empty() version works (without even a Notice) even if $display_options is unset.

benjifisher’s picture

Issue summary: View changes

Copy-editing the issue summary.

I am not sure that my summary of @Berdir's comments in #42 is correct.

voleger’s picture

Thanks for the review.

Regarding #62:

1). Reverted changes. It's out of the scope of this issue.

2). I agree. It's better to return meaningful data for existing enableModules() method. Updated.

3). Reverted changes. It's out of the scope of this issue.

4). Reverted changes. It's out of the scope of this issue.

5). Yep, a little bit messy expression here. Fixed.

benjifisher’s picture

@volegar: Thanks for the updated patch.

  /**
   * Tests the integrity of field plugin definitions.
   */
  public function testFieldPluginDefinitionIntegrity() {
    // Enable all core modules that provide field plugins, and their
    // dependencies.
    $modules = system_rebuild_module_data();
    $modules = array_filter($modules, function (Extension $module) {
      // Filter contrib, hidden, already enabled modules and modules in the
      // Testing package.
      if ($module->origin === 'core'
        && empty($module->info['hidden'])
        && $module->status == FALSE
        && $module->info['package'] !== 'Testing'
        && is_readable($module->getPath() . '/src/Plugin/Field')) {
        return TRUE;
      }
      return FALSE;
    });
    // Gather the dependencies of the modules.
    $dependencies = NestedArray::mergeDeepArray(array_map(function (Extension $module) {
      return array_keys($module->requires);
    }, $modules));
    $modules = array_unique(NestedArray::mergeDeep(array_keys($modules), $dependencies));

    $this->enableModules($modules);
    // ...
  }

Please replace this with another call to the new method modulesWithSubdirectory(). Other than that, the changes look good.

Funny: when I first looked at the interdiff, I wondered why you were replacing $this->container->get('plugin.manager.field.field_type') with \Drupal::service('plugin.manager.field.field_type'). I had to reread my own comments to remind myself that I asked you to remove unrelated fixes.

benjifisher’s picture

Status: Needs review » Reviewed & tested by the community

Thanks, that is much more DRY!

My understanding is that DIRECTORY_SEPARATOR is not needed here, although some consider it a good idea. I see that it is used in a few other places in Drupal core, and I am not aware of any coding standard for or against using it. So I think we can leave this to your discretion.

This patch satisfies all the points I raised in #61, so back to RTBC.

catch’s picture

Version: 8.5.x-dev » 8.4.x-dev
Status: Reviewed & tested by the community » Needs work

Committed/pushed to 8.5.x, thanks!

This should be backported to 8.4.x, but it doesn't cherry-pick cleanly so needs a re-roll against that branch.

catch’s picture

Issue tags: +Needs re-roll
voleger’s picture

Added array_diff() for list of module to enable regarding self::$modules because Drupal test throws an exception till 8.5.x version if test try to enable system module again.

benjifisher’s picture

The attached is not a standard interdiff. To generate it, I applied the patch from #71 to the current HEAD of 8.4.x, then compared the single file FieldDefinitionIntegrityTest.php to the version in 8.5.x, which now includes the patch from #66.

Note that the comment was changed by Commit 3a4f6f2979b from #2901692: FieldDefinitionIntegrityTest does not respect module dependencies. The original patch for that issue was applied to both 8.4.x and 8.5.x, then reverted on both branches, and then a new patch was committed on just the 8.5.x branch.

Unfortunately, the current patch changes the behavior of testFieldPluginDefinitionIntegrity(), in effect applying the patch from #2901692: FieldDefinitionIntegrityTest does not respect module dependencies. I am not sure why that patch was not applied to 8.4.x, but I do not think that we should mix up the two issues by applying it here.

I will leave the status at NR, add the related issue, and see if I can get some comments on why it was not fixed in 8.4.x.

benjifisher’s picture

@dawehner reopened #2901692: FieldDefinitionIntegrityTest does not respect module dependencies. I suggest that we put off this issue until that one is resolved. If that does not get committed to the 8.4.x branch, then on this issue we can either put in just the bug fix, no change to tests, or else put in the bug fix and the new test, without changing the existing one. That last option would break the DRY principle.

voleger’s picture

#2901692: FieldDefinitionIntegrityTest does not respect module dependencies fixed!

Lets try to run test for 8.4.x with patch from #66 comment.

benjifisher’s picture

Right, the patch from #66 applies cleanly. That is not surprising, because the only commits on 8.4.x or 8.5.x that are not on the other branch are related to this issue or to #2901692.

Let's start by re-testing the test-only patch from #66. (I will do that.) If that fails in the expected way, then we should re-upload the real patch from #66.

voleger’s picture

Same behavior as 8.5.x.

It looks like the patch from #66 comment ready to go in 8.4.x.

benjifisher’s picture

Right, the test-only patch fails in the expected way:

There was 1 failure:

1) Drupal\Tests\field\Kernel\FieldDefinitionIntegrityTest::testFieldPluginDefinitionAvailability
PluginNotFoundException here for "weight" field view display options of "menu_link_content" entity type. Original message: The "integer" plugin does not exist.

/var/www/html/core/modules/field/tests/src/Kernel/FieldDefinitionIntegrityTest.php:178
/var/www/html/core/modules/field/tests/src/Kernel/FieldDefinitionIntegrityTest.php:146

I will re-upload the patch from #66.

voleger’s picture

No failures. Back to RTBC?

benjifisher’s picture

Status: Needs review » Reviewed & tested by the community

Yes. To summarize the last few comments:

In #69, @catch committed the patch from #66 to 8.5.x and wrote,

This should be backported to 8.4.x, but it doesn't cherry-pick cleanly so needs a re-roll against that branch.

The patch did not apply to the 8.4.x branch because #2901692: FieldDefinitionIntegrityTest does not respect module dependencies was committed only on the 8.5.x branch. Now that that issue has been committed to the 8.4.x branch, the files affected by this patch are the same on 8.4.x and 8.5.x.

Just to be sure, we re-tested the patch from #66 and the test-only patch there, and got the expected results.

  • catch committed 3cadcd5 on 8.5.x
    Issue #2903161 by voleger, tacituseu, xjm, benjifisher, andypost, Berdir...
larowlan’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: -Needs re-roll
  1. +++ b/core/modules/field/tests/src/Kernel/FieldDefinitionIntegrityTest.php
    @@ -115,7 +105,114 @@ public function testFieldPluginDefinitionIntegrity() {
    +   * Helper method that tries to load plugin definitions.
    

    this seems like the wrong comment? Technically its loading the definition, but its loading and testing.

    Perhaps we should rename to assertValidDisplayOption and then the description would be 'Asserts that display options reference valid plugins'?

  2. +++ b/core/modules/field/tests/src/Kernel/FieldDefinitionIntegrityTest.php
    @@ -115,7 +105,114 @@ public function testFieldPluginDefinitionIntegrity() {
    +  protected function modulesWithSubdirectory($subdirectory) {
    

    this is slick

Nice work here

andypost’s picture

@larowlan If we gonna rename method for 8.4 then it needs follow-up to rename in 8.5 where the patch already accepted #69

larowlan’s picture

Status: Needs review » Reviewed & tested by the community

Oh. missed that - thanks @andypost

andypost’s picture

Patch from #78 applies to 8.4 without offsets

  • xjm committed 4fd27cb on 8.4.x authored by catch
    Issue #2903161 by voleger, tacituseu, xjm, benjifisher, andypost, Berdir...
xjm’s picture

Status: Reviewed & tested by the community » Fixed

It cherry-picks cleanly now, so I just cherry-picked it. Thanks!

I don't think the followup I asked for in #42 ever got filed; can we file it and link here?

benjifisher’s picture

When I updated the issue summary, I read through #42 a few times. It was not clear to me what follow-up should be filed. Are we looking for a test of the upgrade path to 8.3?

I think that @Berdir in #42 suggested either a follow-up or else updates to #2869449: PluginNotFound exception after update to Drupal 8.3 and/or #2883949: The "integer" plugin does not exist., both currently marked as "postponed".

xjm’s picture

Thanks @benjifisher. What I'm looking for is a followup to check over other places where integer is used that should maybe be number_integer instead, and for this:

Re the naming, yes, that the name is different is a bit unfortunate, but that's actually quite common to have mismatches between field types, widgets and formatter plugin ID's, e.g. link has link_default widget and link formatter, string has string_textfield widget and string as formatter, string_long has string_textarea widget and basic_string as default formatter. I don't think we need to change that, we don't really have a BC system for plugin ID's. But maybe instead we could add stricter, earlier validation, so that when you use an invalid type when defining a base field, it throws an exception immediately with helpful context?

Thanks!

Berdir’s picture

I created #2924861: Add validation for chosen widget/formatter plugin in base fields for the second part, but I wouldn't know where to look for the first. This issue already added a test for all base fields to have a valid type and the follow-up that I created would then take care of whatever this would have missed, if anything.

benjifisher’s picture

Thanks, @Berdir. I have added that as a related issue. I think that satisfies @xjm's request, so I am removing the "Needs followup" tag.

Status: Fixed » Closed (fixed)

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