Display mode configurations don't get updated with new fields

Instead of doing this via the UI, I think it should be a hook_ENTITY_TYPE_insert() implementation.
It would likely be system_field_storage_config_insert(), since #2551893: Add events for matching entity hooks still hasn't happened.

Additionally, I think this should re-save both the EntityViewDisplay and EntityFormDisplay? Without checking, I believe the form would suffer the same problem.

The concern here is with updating existing displays, right? Not necessarily creating new ones every time.
entity_get_display() and entity_get_form_display() are both deprecated, because they combine load and create in an unclear way.

Switching to loading only fixes the fails for me.
Also switching away from the deprecated EntityManager.

This should be possible to write tests for.

In the Issue Summary, you mention

Expected result:
The config file for the teaser display mode contains the field you just created under the "hidden" key.

This means it will never show up for ->getComponents(). We'd need to check under hidden.

So, I updated the test to do so.

However, running the test without the fix should fail. But for me it passes. field_test is already under hidden without the changes to field.module!
Which means there's something missing between the Steps to Reproduce and the test as written.

Added an issue that seems related.
There language settings where only updated in the config-exports after saving the displays one-by-one. Like here.

Good catch! Guessing it was \Drupal\Core\Entity\EntityDisplayBase::init() interfering.

Looks good to me, but someone else needs to sign-off.

This looks really good, and has test-coverage that fails when the fix isn't there.

+++ b/core/modules/field/field.module
@@ -347,6 +349,43 @@ function field_form_config_admin_import_form_alter(&$form, FormStateInterface $f
+/**
+ * Implements hook_ENTITY_TYPE_insert() for 'field_config'.
+ *
+ * Allow other view modes to update their configuration for the new field.
+ */
+function field_field_config_insert(FieldConfigInterface $field) {

Is this also working when a field is removed?

\Drupal\Core\Entity\EntityDisplayBase::onDependencyRemoval() handles that case, which is much more essential to the runtime integrity of the site. This issue is more about maintaining the true state of the config at all times, and wouldn't affect runtime at all.

Ok thanks for replies tim.plunkett and Dane Powell
Also RTBC

Stuff that creates noise in config diffs is major typically, so promoting.

The added hook implementation doesn't explain why it's necessary; probably we should add a comment documenting that?

Playing around with this patch in a separate drush command. So I could deploy it on an existing code-base.
https://github.com/DxEx/drupal8-drush-rebuild-displays

Noticed that the 'default' displays are missing now. I guess not on purpose.

Can we add these display to the $view_modes and $form_modes arrays?

$view_modes = $entity_display_repository->getViewModes($entity_type_id);
$view_modes['default'] = ['status' => TRUE];
$form_modes = $entity_display_repository->getFormModes($entity_type_id);
$form_modes['default'] = ['status' => TRUE];

#26 sorry for the delayed reply Dane Powell

Code looks solid.

Now it's called hook_ENTITY_TYPE_insert
Shouldn't this also run on _delete and _update?

Do you mind to add test coverage for removing and updating?
And maybe also for the 'default' key.
Then everything is covered.

I have tested this patch and it works as expected.
Personally I prefer the extended tests from #30 so thanks!

But now I also tested #2680939: Display configuration not fully removed after uninstalling Language module with and without this patch. This patch doesn't solve that issue.
The interesting thing is that this patch does contain the code that fixes that issue!
Uninstalling language module does not trigger field_field_config_insert()<code> (nor <code>field_field_config_update and field_field_config_remove)

When I run the code inside field_field_config_insert() for all entities, the one I published in #25, #2680939: Display configuration not fully removed after uninstalling Language module is fixed though!

Thinking about that I want to propose a refactor of this patch:
- Move the executable part (inside field_field_config_insert()) to a service so that not only field_field_config_insert() can call it, but also language_uninstall() and other functions/modules that want to rebuild the displays.
- I am not sure if this we should optimize the code to do rebuild the displays per 'field' (as in the current patch). It could be ok to just rebuild all displays of all entities.

Rewrote #30 to use a service.
This way #2680939: Display configuration not fully removed after uninstalling Language module can be fixed with the same service.

In the executable code (that is in the service) 2 changes are made:
- The 'default' display is added to all bundles. This is done because those were not parsed when uninstalling language module (#2680939)
- The 'status' for display check is removed. This is done because core.entity_view_display.node.article.rss was skipped when uninstalling language module (#2680939). This change seems harmless because there is a NULL check already.

Hereby 3 patches:
1. Failing test only
2. Test + service patch for this issue only
3. Combined patch with #2680939

The interdiff is a not so useful but also added

Let's try the combined patch again. It validates locally. Removed the # from the filename.

A javascript test is failing (Field.Drupal\field\Tests\FieldImportDeleteUninstallUiTest) that is not in this patch.
That test does things with fields, so that suspicious.
#2680939: Display configuration not fully removed after uninstalling Language module went green though.
Maybe the changes with 'default' display and 'status' parsing are the reason.
I must dive in to it. But that will take a few days. Especially because my javascript tests are not running locally.

For now I re-queued the tests here, maybe it is the test-bot.

Ran some variations of #34 patch on #2274167: [ignore] Patch testing issue
The default-display part breaks core/modules/field/src/Tests/FieldImportDeleteUninstallUiTest.php

+      // Explicitly add the default displays.
+//      $view_modes['default'] = ['status' => TRUE];
+//      $form_modes['default'] = ['status' => TRUE];

Analysis so far: test FieldImportDeleteUninstallUiTest invokes
field_field_config_insert()
and that triggers saving a default display that fails the test.

Also added this patch to #2680939: Display configuration not fully removed after uninstalling Language module

#39 was triggered by updating the default-form-display in field_field_config_insert().

Solved this by adding a parameter $include_default_displays to the service.

Now both #2680939: Display configuration not fully removed after uninstalling Language module and this issue should be green!

Needs review.

Both drupal-2915036-40--service-approach.patch and drupal-2915036-40--service-approach--and-patch-2680939-15--combined.patch are green yay!
Also #2680939: Display configuration not fully removed after uninstalling Language module#15 is green!

Test bot doesn't like the interdiff. Re-uploading #40 and re-scheduling the tests.

1. +++ b/core/modules/field/field.module
   @@ -335,6 +335,26 @@ function field_form_config_admin_import_form_alter(&$form, FormStateInterface $f
   + * Allow other view modes to update their configuration for the new field.
   + * Otherwise, configuration for view modes won't get updated until the mode
   + * is used for the first time, creating noise in config diffs.
   ...
   + * @see https://www.drupal.org/node/2915036
   

   We don't reference issues in @see comments. Can you remove this.

   Can you put the comment inside the function body, so the docblock should just contain the Implements hook_ENTITY_TYPE_insert() for 'field_config'

2. +++ b/core/modules/field/field.services.yml
   @@ -5,3 +5,6 @@ services:
   +    arguments: ['@entity_type.manager', '@entity_display.repository', '@entity_type.bundle.info']
   
   +++ b/core/modules/field/src/EntityDisplayRebuilder.php
   @@ -0,0 +1,98 @@
   +  public function __construct(EntityTypeManagerInterface $entity_type_manager, EntityDisplayRepositoryInterface $entity_display_repository) {
   

   There's a mismatch here

3. +++ b/core/modules/field/src/EntityDisplayRebuilder.php
   @@ -0,0 +1,98 @@
   +  public function rebuildAllEntityTypeDisplays($include_default_displays = FALSE) {
   

   There are no calls to this, and no test coverage.

   Let's just remove it

4. +++ b/core/modules/field/src/EntityDisplayRebuilder.php
   @@ -0,0 +1,98 @@
   +
   

   nit blank link isn't needed

5. +++ b/core/modules/field/src/EntityDisplayRebuilder.php
   @@ -0,0 +1,98 @@
   +    $entity_bundles = entity_get_bundles($entity_type_id);
   

   entity_get_bundles is deprecated, let's inject the entity type bundle info service and use that instead

6. +++ b/core/modules/field/src/EntityDisplayRebuilder.php
   @@ -0,0 +1,98 @@
   +      $view_modes = $this->entityDisplayRepository->getViewModes($entity_type_id);
   +      $form_modes = $this->entityDisplayRepository->getFormModes($entity_type_id);
   

   These don't need to be inside the foreach ($entity_bundles) ... loop

7. +++ b/core/modules/field/src/EntityDisplayRebuilder.php
   @@ -0,0 +1,98 @@
   +        if ($display = EntityViewDisplay::load($entity_type_id . '.' . $bundle . '.' . $view_mode_id)) {
   ...
   +        if ($display = EntityFormDisplay::load($entity_type_id . '.' . $bundle . '.' . $form_mode_id)) {
   

   Can we use array_map here to create an array of ids and then use load multiple.

   e.g

   $view_mode_ids = array_map(function ($view_mode) use ($entity_type_id, $bundle) {
     return "$entity_type_id.$bundle.$view_mode";
   }, $view_modes);
   $view_modes = $this->entityTypeManager->getStorage('entity_view_display')->loadMultiple($view_mode_ids);
   

   That way we can load them all in one go, instead one by one.

   We could even make the callback in array_map a closure and reuse it for both form and view modes

   Also, we have the entityTypeManager injected, so lets use it instead of the static ::load method

8. +++ b/core/modules/field/tests/src/Kernel/DisplayModeUpdateTest.php
   @@ -0,0 +1,93 @@
   +    $default_config_name = 'core.entity_view_display.entity_test.entity_test.foobar';
   +    $foobar_config_name = 'core.entity_view_display.entity_test.entity_test.foobar';
   

   These are the same? Shouldn't one be .default?

9. +++ b/core/modules/field/tests/src/Kernel/DisplayModeUpdateTest.php
   @@ -0,0 +1,93 @@
   +      $fields['field_test']->delete();
   ...
   +    $this->assertArrayNotHasKey('field_test', \Drupal::config($default_config_name)->get('hidden'));
   +    $this->assertArrayNotHasKey('field_test', \Drupal::config($foobar_config_name)->get('hidden'));
   

   I assume this already exists in core because we're not doing anything for hook_entity_ENTITY_TYPE_delete in this patch?

   If so is this test coverage unneeded?

Thanks for the review larowlan

#47.3 Because of #2680939: Display configuration not fully removed after uninstalling Language module. I remove it in this patch. First things first.

#47.8 Missed this one!

#47.9 Dane Powell you were right. I was asking for this because of #2680939.

Others: will do. Expect a week or so.

47.1
Done

47.2
Done, see 47.5

47.3
Removed. Must be reintroduced in #2680939: Display configuration not fully removed after uninstalling Language module

47.4
Is this the last line?
Did not change anything.
I thought every php-file must end with on blank line.

47.5
Done

47.6
Done

47.7
done: arrap_map and loadMultiple()
done: use entityTypeManager
the closure thing I did not.

48.8
Extended the test to now include
view-mode default
view-mode foobar
form-mode default
form-mode foobar

Regarding the view-modes I found an a strange assumption in entity_test_entity_form_mode_info_alter().
I changed that _alter to add form-mode 'compact', without removing other form-mode. In this patch there is test-coverage for default and foobar.

48.9
Removed these tests.
I also validated this manually.

Been looking at some things with @ndf

+++ b/core/modules/field/src/EntityDisplayRebuilder.php
@@ -0,0 +1,92 @@
+    $view_modes = $this->entityDisplayRepository->getViewModes($entity_type_id);
+    $form_modes = $this->entityDisplayRepository->getFormModes($entity_type_id);

if we use getViewModeOptions and getFormModeOptions, this would allow us to get rid of the $include_default_displays parameters, which makes this much easier to understand.

Thanks lendude, definitely an improvement.

Updated the patch + interdiff with #49

Also updated the combined patch in #2680939: Display configuration not fully removed after uninstalling Language module

#52 failed because of core/modules/field/src/Tests/FieldImportDeleteUninstallUiTest.php with message

The configuration cannot be imported because it failed validation for the following reasons:
Configuration core.entity_form_display.entity_test.entity_test.default depends on the field.field.entity_test.entity_test.field_tel configuration that will not exist after import.

#52 enforces creating of 'default' displays when a new field is created. The above test must remove that to run successful.

+++ b/core/modules/field/tests/src/Kernel/DisplayModeUpdateTest.php
@@ -0,0 +1,106 @@
+    $default_view_display = 'core.entity_view_display.entity_test.entity_test.default';
+    $default_form_display = 'core.entity_form_display.entity_test.entity_test.default';
+    $foobar_view_display = 'core.entity_view_display.entity_test.entity_test.foobar';
+    $foobar_form_display = 'core.entity_form_display.entity_test.entity_test.foobar';

It would be nice to get rid of these hardcoded names and just use getConfigDependencyName() instead.

So store the display name in a var and then use that, so something like:

    // Create 'default' view-display.
    $default_view_display = EntityViewDisplay::create([
      'targetEntityType' => 'entity_test',
      'bundle' => 'entity_test',
      'mode' => 'default',
      'status' => TRUE,
    ]);
    $default_view_display->save();
    $this->defaultViewDisplayName = $default_view_display->getConfigDependencyName();

And then just
$this->assertArrayNotHasKey('field_test', \Drupal::config($this->defaultViewDisplayName)->get('hidden'));

I have resolved this and other related issues by overriding the default service container cache backends since drupal bootstraps the container to the database. I believe this is needed when updating the data, discovery, and config bins when deploying updates such as this to display mode configurations since the field storage needs to update at runtime.

See https://github.com/acquia/blt/blob/9.1.x/settings/memcache.settings.php for an example with memcache.

#58 Can you explain why the cache-backend is relevant here?
No one mentioned anything regarding caching nor the service-container to reproduce this issue.
For the record: In my local development testing I only used database-backend.

The failed test is introduced in 8.5.x #2940201: hook_field_widget_form_alter() can no longer affect the whole widget for multi-value fields
There is also a follow up, not sure if it is related #2943035: hook_field_widget_multivalue_form_alter and hook_field_widget_multivalue_WIDGET_TYPE_alter are not invoked for single widget that handle multiple values

While upgrading my local test environment to 8.5.x I broke it, not the first time 😬
So currently no xdebug in my test-suite.

One guess is that this part in the #2940201: hook_field_widget_form_alter() can no longer affect the whole widget for multi-value fields test

entity_get_form_display($this->field['entity_type'], $this->field['bundle'], 'default')
      ->setComponent($field_name, [
        'type' => $widget,
      ])
      ->save();

might give troubles. This will trigger the new $display_rebuilder service introduced here.

Just rescheduled the last passing patch (#54) against:
-8.4.x
-8.5.x
-8.6.x

Yes it the new widgetAlterTest.
Now branch 8.5 and 8.6 are failing.

Tried to understand the changes #2940201: hook_field_widget_form_alter() can no longer affect the whole widget for multi-value fields but I don't get the relation with this issue.

test testFieldFormMultipleWidgetTypeAlterSingleValues was also changed in #2943035: hook_field_widget_multivalue_form_alter and hook_field_widget_multivalue_WIDGET_TYPE_alter are not invoked for single widget that handle multiple values

So first #2940201: hook_field_widget_form_alter() can no longer affect the whole widget for multi-value fields
then #2943035: hook_field_widget_multivalue_form_alter and hook_field_widget_multivalue_WIDGET_TYPE_alter are not invoked for single widget that handle multiple values
#2943189: Clean up the test code for hook_field_widget_multivalue_form_alter() is still open, but that doesn't solve this issue.

Diving in this is the line that triggers the failing test:
$this->entityTypeManager->getStorage('entity_form_display')->loadMultiple($form_mode_ids);
That line is inside rebuildEntityTypeDisplays()
Note that this is only loading of data, nothing is saved here!

I also tried to revert the change with form-mode 'compact', but that doesn't make any difference.

It might be a state-thing inside the testFieldFormMultipleWidgetTypeAlterSingleValues test. see #58

Currently my gut-feeling is that this issue is only exposing an issue in the new test and/or hook.

The problem in #60 seems to be that when loadMultiple is called, all hook information gets cached. So after updating the component through the API, the hook cache has no idea that it needs to be refreshed. So we need to force it to refresh.

Using $this->rebuildAll(); might be a bit more then is needed but its mostly to show where the problem lies with this failing test.

So this problem stems mostly from \Drupal\field\Tests\FormTest doing API calls in functional tests which can cause the container and/or cache to be out of sync.

Unrelated failure (DST).

+++ b/core/modules/field/src/Tests/FieldImportDeleteUninstallUiTest.php
@@ -86,9 +86,10 @@ public function testImportDeleteUninstall() {
-    // Stage the field deletion
+    // Stage the field deletion including its dependencies.
...
+    $sync->delete('core.entity_form_display.entity_test.entity_test.default');

+++ b/core/modules/field/src/Tests/FormTest.php
@@ -689,6 +689,10 @@ protected function widgetAlterTest($hook, $widget) {
+    // We need to rebuild hook information after setting the component through
+    // the API.
+    $this->rebuildAll();
+

+++ b/core/modules/system/tests/modules/entity_test/entity_test.module
@@ -273,11 +273,9 @@ function entity_test_entity_form_mode_info_alter(&$form_modes) {
-      $form_modes[$entity_type] = [
-        'compact' => [
-          'label' => t('Compact version'),
-          'status' => TRUE,
-        ],
+      $form_modes[$entity_type]['compact'] = [
+        'label' => t('Compact version'),
+        'status' => TRUE,

These changes are fixing test-issues exposed by this patch. Seems ok to fix here.
RTBC from me too.

1. +++ b/core/modules/field/field.module
   @@ -334,6 +334,23 @@ function field_form_config_admin_import_form_alter(&$form, FormStateInterface $f
   +function field_field_config_insert(FieldConfigInterface $field) {
   ...
   +  $display_rebuilder->rebuildEntityTypeDisplays($field->getTargetEntityTypeId());
   

   In the context of inserting a new field config entity, we have a bundle in scope.

   Yet the display rebuilder does not take this into account, and instead rebuilds all form and view modes for all bundles of the given entity type.

   Shouldn't we limit it to the bundle of the field being inserted?

2. +++ b/core/modules/field/src/EntityDisplayRebuilder.php
   @@ -0,0 +1,86 @@
   + * Rebuilds displays.
   

   Could we get an extended comment here explaining a bit about where this is used with an @see reference to the field config insert hook where it is used from.

   The current short comment is a bit light on.

3. +++ b/core/modules/field/src/Tests/FormTest.php
   @@ -689,6 +689,10 @@ protected function widgetAlterTest($hook, $widget) {
   +    // We need to rebuild hook information after setting the component through
   +    // the API.
   +    $this->rebuildAll();
   

   Can we use less of a sledgehammer approach here? Can we clear just the entity or plugin caches that are stale?

4. +++ b/core/modules/field/tests/src/Kernel/DisplayModeUpdateTest.php
   @@ -0,0 +1,115 @@
   + * Tests exposing field definitions for configurable fields.
   

   That looks like a c/p error?

5. +++ b/core/modules/field/tests/src/Kernel/DisplayModeUpdateTest.php
   @@ -0,0 +1,115 @@
   +  protected $defaultViewDisplayName;
   +  protected $defaultFormDisplayName;
   +  protected $foobarViewDisplayName;
   +  protected $foobarFormDisplayName;
   

   These need docblocks right?

6. +++ b/core/modules/field/tests/src/Kernel/DisplayModeUpdateTest.php
   @@ -0,0 +1,115 @@
   +    $default_view_display = EntityViewDisplay::create([
   ...
   +    $default_form_display = EntityFormDisplay::create([
   ...
   +    EntityViewMode::create([
   ...
   +    $foobar_view_display = EntityViewDisplay::create([
   

   Can we get a follow-up to add a test-trait for creating these, see it a lot, would be a useful utility

7. +++ b/core/modules/system/tests/modules/entity_test/entity_test.module
   @@ -273,11 +273,9 @@ function entity_test_entity_form_mode_info_alter(&$form_modes) {
   -      $form_modes[$entity_type] = [
   -        'compact' => [
   -          'label' => t('Compact version'),
   -          'status' => TRUE,
   -        ],
   +      $form_modes[$entity_type]['compact'] = [
   +        'label' => t('Compact version'),
   +        'status' => TRUE,
   

   unrelated?

#73.1
Sounds sane to limit rebuildEntityTypeDisplays() with a bundle parameter.
But I have to dive in to it. Up till #30 the resaving was limited by bundle.
And I removed that option when refactoring to the service-approach.
Maybe this was done for #2680939: Display configuration not fully removed after uninstalling Language module

#73.7
This change is intentional.
Form mode display tests were overridden by the original form_mode_info_alter() because it drops all form-modes and replaces this with only 'compact'. Therefore this change where form_mode_info_alter() adds 'compact' to array for form-modes instead of replacing it.

Re-rolled, but still needs to address comments in #73.

Include a couple of changes that I managed to miss in the previous patch.

Still need to address comments in #73, but let's get this re-roll correct first.

We need a re-roll for 8.6 too.

Does anybody have an advice how to make the patch working for Drupal 8.6?

Here's a patch for 8.6.4.

Reroll and addresses #73 and does some clean up. Changed the service to a helper class since there should be no need to swap this out and we shouldn't make this API

#73.1 done
#73.2 done
#73.3 *shrug* it's a test and don't think we have any Testing API's to do this in a subtle manner, left this
#73.4 changed
#73.5 yup, added
#73.6 not sure, yeah its used fairly often but not all that hard to set up now, less API to maintain like this *shrug*
#73.7 see #74, this change is needed

Thanks Lendude for picking this up. Patch looks good to me.

I have a question regarding #73.1 and changing the service to a helper class:
In issue #2680939: Display configuration not fully removed after uninstalling Language module we want to use the EntityDisplayRebuilder introduced in this patch.
Is it still possible? Or is a service required then?

@ndf since all the calls there are also from hooks, it can just use it in the same way as this patch by calling

\Drupal::classResolver(EntityDisplayRebuilder::class)->rebuildEntityTypeDisplays($field->getTargetEntityTypeId(), $field->getTargetBundle());

All feedback has been addressed! So RTBC.

This issue has been RTBC for one and a half months, is there anything that prevents it from being committed?

Unrelated fail, back to RTBC

1. +++ b/core/modules/field/tests/src/Functional/FormTest.php
   @@ -713,6 +713,10 @@ protected function widgetAlterTest($hook, $widget) {
   +    // We need to rebuild hook information after setting the component through
   +    // the API.
   +    $this->rebuildAll();
   

   Can we get a follow-up issue to explore if we can add some testing API for this.

Adding issue credits, adding myself for review in #73

Also adding @sime, @Joseph Zhao, @fubarhouse and @Alex Skrypnyk who sat in on a one hour review of this patch as part of core process walkthrough

Committed 49d60f1 and pushed to 9.0.x. Thanks!

c/p to 8.9.x

As this is a major bug with no disruption, I feel this is eligble for backport to 8.8.x, so c/p to 8.8.x too.

Wondering if this will find it's way into non-dev 8.8 release?