When you add a new field to a content type and export configuration (via something like drush cex), the configuration for the teaser display mode is typically lacking a reference to the field you just created. The configuration for the teaser display mode will only get updated later once the display mode is actually used.

Steps to reproduce:

  1. Start with a fresh install of Drupal (I'm using Lightning). Run drush cex to capture a baseline configuration.
  2. Create a new content type foo and add a field to that content type (I think any field will do, but I used an entity reference)
  3. Run drush cex to export the new content type, field, and display modes.
  4. Open the config file core.entity_view_display.node.foo.teaser

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

Actual result:
The config file for the teaser display mode contains no reference to any field other than the default body field.

If you then go edit and save the teaser display mode in the UI, OR re-install the site from scratch using drush si --config-dir, and then re-export configuration, you'll note that the teaser display mode now properly contains the new field.

This may sound like a minor problem, but it actually wreaks havoc with a good configuration management / CI system, because a developer will typically add fields to a content type and capture the changes in config, thinking everything is fine, and then only much later in seemingly unrelated operations will the changes to the display mode suddenly come through. It also causes problems if you are attempting to verify configuration integrity (the way that BLT does), because this configuration will randomly show up later on and cause configuration integrity validation to fail.

CommentFileSizeAuthor
#60 interdiff-drupal-2915036-54-60.txt6.1 KBDane Powell
#60 drupal-2915036-60.patch11.79 KBDane Powell
#54 interdiff-52-54.txt827 bytesndf
#54 drupal-2915036-54--service-approach.patch10.52 KBndf
#52 drupal-2915036-52--service-approach.patch9.56 KBndf
#52 interdiff--2915036-49--2915036-52.txt2.29 KBndf
#49 drupal-2915036-48--service-approach.patch10.14 KBndf
#49 interdiff-40-48.txt12.12 KBndf
#46 interdiff-34-40--combined.txt7.42 KBndf
#40 interdiff-34-40--combined.patch7.42 KBndf
#40 drupal-2915036-40--service-approach--and-patch-2680939-15--combined.patch12.63 KBndf
#40 interdiff-32-40--service-approach.txt3 KBndf
#40 drupal-2915036-40--service-approach.patch8.24 KBndf
#40 interdiff-32-40--failing-test-only.txt0 bytesndf
#40 drupal-2915036-40--failing-test-only.patch3.2 KBndf
#34 drupal-2915036-32--service-approach--and-patch-2680939-12--combined.patch10.48 KBndf
#32 interdiff-2915036-#30-#31service.txt6.4 KBndf
#32 drupal-2915036-31--service-approach--and-patch-2680939#12--combined.patch10.48 KBndf
#32 drupal-2915036-31--service-approach.patch8.02 KBndf
#32 drupal-2915036-31--failing-test-only.patch3.2 KBndf
#30 interdiff-drupal-2915036-24-26.txt2.91 KBDane Powell
#30 drupal-2915036-26.patch5.79 KBDane Powell
#24 interdiff-drupal-2915036-15-24.txt606 bytesDane Powell
#24 drupal-2915036-24.patch4.49 KBDane Powell
#15 interdiff-drupal-2915036-13-15.txt1.57 KBDane Powell
#15 drupal-2915036-15-PASS.patch4.29 KBDane Powell
#15 drupal-2915036-15-FAIL.patch2.35 KBDane Powell
#13 2915036-field-13-PASS.patch3.72 KBtim.plunkett
#13 2915036-field-13-FAIL.patch1.86 KBtim.plunkett
#13 2915036-field-13-interdiff.txt1.19 KBtim.plunkett
#12 drupal-2915036-12.patch4.31 KBDane Powell
#11 2915036-field-11.patch2.01 KBtim.plunkett
#11 2915036-field-11-interdiff.txt1.76 KBtim.plunkett
#10 drupal-2915036-10.patch1.88 KBDane Powell
#8 interdiff-drupal-2915036-6-8.txt691 bytesDane Powell
#8 drupal-2915036-8.patch2.04 KBDane Powell
#6 drupal-2915036-6.patch2.22 KBDane Powell
#3 drupal-2915036-3.patch1.54 KBDane Powell
#2 drupal-2915036-2.patch1.54 KBDane Powell
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

Dane Powell created an issue. See original summary.

Dane Powell’s picture

Version: 8.3.x-dev » 8.4.x-dev
Status: Active » Needs review
FileSize
1.54 KB

This patch saves all active view modes whenever you add a new field, so that their respective configurations are updated. Normally the field simply gets added to the hidden region, although I suppose in the future folks might want to hook into this somehow to add fields to other regions.

Dane Powell’s picture

FileSize
1.54 KB

Minor typo

tim.plunkett’s picture

Status: Needs review » Needs work

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.

Dane Powell’s picture

True, this could be a problem for form display modes too (for those entity types that support them, i.e. not nodes). We've just never seen it because it's rare to have multiple form display modes.

I agree that we should solve this for both form and view display modes, and for all entity types. I'll take a shot at re-rolling the patch.

Dane Powell’s picture

Status: Needs work » Needs review
FileSize
2.22 KB

Okay, this patch takes the approach you suggested. It implements hook_ENTITY_TYPE_insert and hook_ENTITY_TYPE_update for field_storage_config in order to update both view and form display modes whenever a field is inserted or updated.

I actually think we only need to act on inserts, not updates, but this was necessary in order to work around a small problem: $field_storage->getBundles() returns null when called from field_field_storage_config_insert(). It seems that the only way to get the bundles associated with the new field storage config is to check later during field_field_storage_config_update(). If you know how to solve that, I'm happy to refactor this.

Status: Needs review » Needs work

The last submitted patch, 6: drupal-2915036-6.patch, failed testing. View results

Dane Powell’s picture

Status: Needs work » Needs review
FileSize
2.04 KB
691 bytes

This should fix the test errors.

I'd still love any insight into why we can't handle everything in hook_ENTITY_TYPE_insert (see comment above).

Status: Needs review » Needs work

The last submitted patch, 8: drupal-2915036-8.patch, failed testing. View results

Dane Powell’s picture

FileSize
1.88 KB

I figured out half the problem... we should be acting on field inserts, not field storage inserts. The field storage is created before the field is, and the storage is obviously not attached directly to any bundles, so it's not possible for us to update display modes for bundles until the field is created.

Still, this fails tests due to this error:

Error: Call to a member function getConfigDependencyName() on null (docroot/core/lib/Drupal/Core/Entity/EntityDisplayBase.php:314)

My best guess is because the display mode configuration depends on the field configuration, and the field is not 100% saved at this point or something.

@tim.plunkett can you suggest any way to fix the failing tests?

tim.plunkett’s picture

Version: 8.4.x-dev » 8.5.x-dev
Component: configuration entity system » field system
Status: Needs work » Needs review
Issue tags: +Needs tests
FileSize
1.76 KB
2.01 KB

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.

Dane Powell’s picture

FileSize
4.31 KB

Awesome, thanks!

I tried to write a test for this, but my test-fu is not strong. The field doesn't seem to be attached to the view mode ($display->getComponent('field_test') is always null), and I can't figure out why. This is what I have so far.

tim.plunkett’s picture

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.

ndf’s picture

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.

Dane Powell’s picture

EntityViewDisplay::load() must recalculate the view mode on the fly to accommodate any new fields. The problem only occurs when you access the raw configuration, as you would when doing a configuration export. These patches should work.

The last submitted patch, 15: drupal-2915036-15-FAIL.patch, failed testing. View results

tim.plunkett’s picture

Issue tags: -Needs tests

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

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

borisson_’s picture

Status: Needs review » Reviewed & tested by the community

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

ndf’s picture

+++ 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?

tim.plunkett’s picture

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

Dane Powell’s picture

I've also verified empirically that display modes are correctly updated when fields are removed.

ndf’s picture

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

xjm’s picture

Priority: Normal » Major
Status: Reviewed & tested by the community » Needs work

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?

Dane Powell’s picture

ndf’s picture

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];
Dane Powell’s picture

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

I don't know what you mean by this. Are you saying that with this patch applied on a vanilla Drupal install, when you add a new field it's not added to the default display mode? This would be surprising, since I've tested this use case extensively. Can you provide steps to reproduce whatever it is that you're seeing?

Default displays already get updated whenever you modify fields (before this patch). The original bug report here is only related to ancillary displays (teasers, etc--see the steps to reproduce). This patch only affect these ancillary display modes, it doesn't touch the default displays.

Dane Powell’s picture

Tim, xjm, ndf, would one of you mind reviewing the comments I added in #24? I think this is ready to merge as far as most of us are concerned.

ndf’s picture

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

Dane Powell’s picture

See comments #19 and #20: the case of deleting is already handled by \Drupal\Core\Entity\EntityDisplayBase::onDependencyRemoval(), and updating should never result in needing this kind of update, because the conflict only comes with a field is added or removed. I can try to add test cases for these regardless, but I can assure you that they aren't necessary and would probably duplicate existing test cases (I'm sure onDependencyRemoval already has test coverage).

Dane Powell’s picture

This patch adds a test for removing a field and ensuring that it also gets removed from display modes, as well as a test for default display modes.

A test for updating fields would be useless, AFAIK, because updating a field doesn't update any of the configuration objects that we're worried about.

This should really be ready to merge now.

ndf’s picture

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.

ndf’s picture

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

Status: Needs review » Needs work
ndf’s picture

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

The last submitted patch, 32: drupal-2915036-31--failing-test-only.patch, failed testing. View results

The last submitted patch, 32: drupal-2915036-31--service-approach.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Status: Needs review » Needs work

The last submitted patch, 34: drupal-2915036-32--service-approach--and-patch-2680939-12--combined.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

ndf’s picture

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.

ndf’s picture

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.

ndf’s picture

Status: Needs review » Needs work

The last submitted patch, 40: interdiff-34-40--combined.patch, failed testing. View results

ndf’s picture

Status: Needs work » Needs review

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!

Dane Powell’s picture

Status: Needs review » Reviewed & tested by the community

#40 looks good

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 40: interdiff-34-40--combined.patch, failed testing. View results

ndf’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
7.42 KB

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

larowlan’s picture

Status: Reviewed & tested by the community » Needs work
  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?

ndf’s picture

Assigned: Unassigned » ndf

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.

ndf’s picture

Status: Needs work » Needs review
FileSize
12.12 KB
10.14 KB

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.

Dane Powell’s picture

larowlan, I think you're in the best position to review this since you requested changes, can you please review? We _really_ need to get this committed, we are burning countless dev hours constantly hitting this bug.

Lendude’s picture

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.

ndf’s picture

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

Status: Needs review » Needs work

The last submitted patch, 52: drupal-2915036-52--service-approach.patch, failed testing. View results

ndf’s picture

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

Lendude’s picture

+++ 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'));

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

anavarre’s picture

Issue tags: +Display modes
lcatlett’s picture

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.

ndf’s picture

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

Status: Needs review » Needs work

The last submitted patch, 60: drupal-2915036-60.patch, failed testing. View results

Dane Powell’s picture

Version: 8.6.x-dev » 8.5.x-dev
Status: Needs work » Needs review

I think we should still target 8.5.x for this since it's a major bug.

Also I'm at a loss to explain the test failure in #60. Does anyone else understand what's causing that?

ndf’s picture

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.

ndf’s picture

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

The last submitted patch, 54: drupal-2915036-54--service-approach.patch, failed testing. View results

ndf’s picture

Status: Needs review » Active

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.

ndf’s picture

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.