Entity displays need to depend on the module that provides base fields

Since this is a major bug, shouldn't it go into 8.4.x?

Patch is looking pretty good, here's a review:

1. +++ b/core/lib/Drupal/Core/Entity/EntityDisplayBase.php
   @@ -298,12 +299,19 @@ public function calculateDependencies() {
   +        // If the field is provider by a module different than the module that
   

   "is provider" -> "is provided" ;)

2. +++ b/core/modules/system/system.post_update.php
   @@ -81,3 +81,15 @@ function system_post_update_classy_message_library() {
   +    // calculateDependencies() will add any missing base field dependencies.
   

   Let's also mentioned the class name, not just the generic method name :)

3. +++ b/core/modules/system/tests/src/Functional/Update/UpdateEntityDisplayBaseFieldDependenciesTest.php
   @@ -0,0 +1,32 @@
   +  public function testUpdate() {
   +    $this->runUpdates();
   +  }
   

   This is not really useful since it doesn't do any assertion that a dependency has been added, so we need an assertion before running the updates on a set of dependencies and another assertion after the update process to check that the set of dependencies has been updated.

4. +++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityDisplayBaseTest.php
   @@ -150,4 +160,35 @@ public function testOnDependencyRemoval() {
   +  public function testBaseFields() {
   

   How about renaming this to testBaseFieldDependencies()?

Here's an update path test. New database needed because there are other updates like system_post_update_add_region_to_entity_displays() that would do the necessary saves to running with an old database is not good.

How about using an update fixture that makes the system act like system_post_update_add_region_to_entity_displays() already ran instead of providing yet another db dump :)

You can find an example where we're already doing this in core/modules/hal/tests/fixtures/update/drupal-8.rest-hal_update_8301.php.

Ohh, right, I missed the interdiff from #9. Those assertions look exactly like what I would expect from this test :)

+++ b/core/modules/system/tests/src/Functional/Update/UpdateEntityDisplayBaseFieldDependenciesTest.php
@@ -18,7 +18,7 @@
+      __DIR__ . '/../../../../tests/fixtures/update/drupal-8.4.2.bare.standard.entity_test_extra_with_base_fields.php.gz',

I don't think we should have this dump specific to version 8.4.2. I discussed this scenario with @catch some time ago and the outcome of that discussion was that we should try to allow upgrades across multiple minor releases at a time (directly from 8.1.x to 8.4.0 for example) on a best-effort basis, meaning that we can't guarantee that it will work but we also should *not* actively prevent that upgrade from working.

In our case, the bug that we're fixing here was not introduced in 8.4.2, it probably exists before 8.0.0 so the dump that we should be using is one that has 8.0.0 (maybe rc1 even) and uses the standard profile, because chances are that you can easily find an existing form/view display that is missing dependencies of a base field which should be fixed by this update.

The reason I'm not keen on adding db dump so easily because they're essentially a black box, no one but the person who created them actually knows what they contain (sometimes even that person forgets :D), so they are realistically one-off blobs of data that we keep piling on without much enticement for re-usability :)

Thanks for your work on this. I can't recall whether I did any manual testing when the issue was first started, but I will check it again.

I re-queued the patch in #16 but it needs a re-roll.

I'm trying to sort out #3038746: Remove deprecated EntityDefinitionUpdateManagerInterface::applyUpdates() from hook_install and hook_uninstall and having the fields remain after uninstall is making the task harder to test.

I have manually re-tested the patch in #16 on 8.5.11 (just the first two files, not the addition to testing) and it appiles OK and does the job required. After uninstalling the module the two base fields are immediately removed from core.entity_form_display.node.article.default (checked using config inspector). The fields are also removed from the content type 'manage form display' tab.

I have not tested on 8.6 or 8.7 as the patch was created for 8.5.

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.