Fix incorrect FieldFormatter id for "weight" field in base field definition in display options

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

@voleger please provide steps to reproduce & to add test

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.

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.

   * @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) { 

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()));

Looks good to me.

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

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

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?

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?

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.

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.

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.

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

@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.

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

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!

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!

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.

Copy-editing the issue summary.

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

@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.

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.

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.

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.

@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.

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.

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.

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.

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

@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

Oh. missed that - thanks @andypost

Patch from #78 applies to 8.4 without offsets

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?

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: The "media" entity type does not exist after update to Drupal 8.3 and/or #2883949: The "integer" plugin does not exist., both currently marked as "postponed".

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!

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.

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.