Fix Drupal\system\Tests\Update\DbUpdatesTrait is deprecated in Drupal 8.4.0 and will be removed before Drupal 9.0.0.

Cleaning up tags.

Re-rolled (because the patch from #3 no longer applies) and corrected the use statements.

Pick this up at Drupal Camp scotland.

So, we might be outside of my experience with tests. But a functional test that installs a test module that has dependencies on test code in system.module is probably not class loaded. Right? I'm not sure how to fix that. However, I did find a couple issues with the deprecation notices. Fixed here.

Ignore patch in 14. The interdiff was correct. The patch had some noise in it.

The problem here is that both entity_test.install and entity_test_update.install both run DbUpdatesTrait::includeUpdates() when they are *included.* That means not inside any hook or anything, just when the file is loaded.

Since Drupal\system\Tests namespace is always autoloaded at runtime, this wasn't a problem. But since we're doing a better job of isolating test infrastructure from runtime, it's now technical debt.

This trait was added in #2542748: Automatic entity updates can fail when there is existing content, leaving the site's schema in an unpredictable state which also fixes a critical. The usage in entity_test.install was added then. #2860654: Field storage CRUD operations must use the last installed entity type and field storage definitions adds the usage in entity_test_update.install.

DbUpdatesTrait is only used by 4 tests, and two of them don't actually use the trait's methods.

So here's what I did:

• Fix the deprecation error message to have the right namespace.
• Remove references to DbUpdatesTrait from tests that don't actually need it.
• Remove references to DbUpdatesTrait from the two install files. This is a bit of an experiment... Let's see if it represents a regression against #2542748: Automatic entity updates can fail when there is existing content, leaving the site's schema in an unpredictable state

Added needs CR update because a) The namespace is wrong in the CR and b) this might be another API change which needs another change record. And if it's not an API change, we still need to document it.

OK, so here's the problem:

An install file running in the fixture of a functional test doesn't autoload the PHPUnit functional test namespaces for a module that isn't enabled.

The system module is not enabled for the two failing tests from #18: Drupal\Tests\system\Functional\Entity\Update\UpdateApiEntityDefinitionUpdateTest and Drupal\Tests\system\Functional\Update\EntityUpdateToPublishableTest

This means that Drupal\Tests\system\Functional\Update\DbUpdateTrait can't be autoloaded during install time, and we shouldn't manually require_once for a trait.

Therefore we either add the system module as a dependency of entity_test and entity_test_update (results in fail), or we split out the behavior of DbUpdatesTrait::includeUpdates into a namespace that *is* autoloaded.

This patch does the latter, creating Drupal\system\Tests\Update\GranularUpdateTestTrait, which sort of un-deprecates part of the trait we previously deprecated.

Another option is to just unwrap the behavior of includeUpdates() into the install files themselves, but then you violate DRY, and you also end up making copypasta typos, like I did when I tried it.

1. +++ b/core/modules/system/src/Tests/Update/GranularUpdateTestTrait.php
   @@ -0,0 +1,28 @@
   +trait GranularUpdateTestTrait {
   ...
   +  public static function includeUpdates($module, $group) {
   

   This doesn't need to be trait - it could be final class with a private constructor. It's never properly being used as a trait other than in deprecated code. And in the non deprecated code it can't be used so there's no point it being a trait.

   Also in some ways it is a shame to have this in system/src/Tests because it means this code is autoloadable during the regualr Drupal runtime whereas it should only be used by test modules. However we've not really got a better location atm so I guess that is okay.

2. +++ b/core/modules/system/tests/modules/entity_test/entity_test.install
   @@ -54,5 +54,7 @@ function entity_test_schema() {
   -DbUpdatesTrait::includeUpdates('entity_test', 'entity_definition_updates');
   -DbUpdatesTrait::includeUpdates('entity_test', 'status_report');
   +// Include specific update files for this module.
   +foreach (['entity_definition_updates', 'status_report'] as $group) {
   +  GranularUpdateTestTrait::includeUpdates('entity_test', $group);
   +}
   

   The foreach is more complex that the two calls before which probably should be preferred because they make you have to think less.

3. +++ b/core/modules/system/tests/src/Functional/Update/EntityUpdateAddRevisionDefaultTest.php
   @@ -16,7 +16,6 @@
   -  use DbUpdatesTrait;
   
   +++ b/core/modules/system/tests/src/Functional/Update/EntityUpdateAddRevisionTranslationAffectedTest.php
   @@ -16,7 +16,6 @@
   -  use DbUpdatesTrait;
   
   +++ b/core/tests/Drupal/Tests/Listeners/DeprecationListenerTrait.php
   @@ -107,7 +107,6 @@ public static function getSkippedDeprecations() {
   -      'Drupal\system\Tests\Update\DbUpdatesTrait is deprecated in Drupal 8.4.0 and will be removed before Drupal 9.0.0. Use \Drupal\FunctionalTests\Update\DbUpdatesTrait instead. See https://www.drupal.org/node/2896640.',
   

   Yay! Like it.

There aren't any more simpletest-based tests in core, according to http://simpletest-countdown.org/

In that spirit, here's the simplest possible patch.

Just one minor tweak I think.

Now without the cruft....

Ok so that still leaves #20 to deal with. To stay with the 'minimal possible patch' (which I like), lets forget DRY for weird includes in .install files in test-only code. So that would leave us with something like this....

It's weird that this passes, since this trait is also in used within entity_test_update.install. 'entity_test_update' appears all over the tests, like for instance Drupal\KernelTests\Core\Entity\EntitySchemaTest. That one should fail based on deprecations if we remove the message from the listener.

I think it passes because kernel tests don't install the modules, so the install file is never loaded. Perhaps?

Here's a functional test that installs it: Drupal\Tests\system\Functional\Update\EntityUpdateAddRevisionTranslationAffectedTest

It ran and passed, because it has @group legacy. Frownyface. But no wonder:

[.. after removing @group legacy ..]

$ ./vendor/bin/phpunit -c core/ --testsuite functional --filter EntityUpdateAddRevisionTranslationAffectedTest
PHPUnit 6.5.14 by Sebastian Bergmann and contributors.

Testing 
..                                                                  2 / 2 (100%)

Time: 1.31 minutes, Memory: 156.00MB

OK (2 tests, 481 assertions)

Remaining deprecation notices (118)

  34x: Drupal\system\Tests\Update\DbUpdatesTrait is deprecated in Drupal 8.4.0 and will be removed before Drupal 9.0.0. Use \Drupal\FunctionalTests\Update\DbUpdatesTrait instead. See https://www.drupal.org/node/2896640.
    17x in EntityUpdateAddRevisionTranslationAffectedTest::testAddingTheRevisionTranslationAffectedField from Drupal\Tests\system\Functional\Update
    17x in EntityUpdateAddRevisionTranslationAffectedTest::testDeletedEntityType from Drupal\Tests\system\Functional\Update

  11x: The revision_user revision metadata key is not set for entity type: block_content See: https://www.drupal.org/node/2831499
    6x in EntityUpdateAddRevisionTranslationAffectedTest::testDeletedEntityType from Drupal\Tests\system\Functional\Update
    5x in EntityUpdateAddRevisionTranslationAffectedTest::testAddingTheRevisionTranslationAffectedField from Drupal\Tests\system\Functional\Update

  11x: The revision_created revision metadata key is not set for entity type: block_content See: https://www.drupal.org/node/2831499
    6x in EntityUpdateAddRevisionTranslationAffectedTest::testDeletedEntityType from Drupal\Tests\system\Functional\Update
    5x in EntityUpdateAddRevisionTranslationAffectedTest::testAddingTheRevisionTranslationAffectedField from Drupal\Tests\system\Functional\Update

  11x: The revision_log_message revision metadata key is not set for entity type: block_content See: https://www.drupal.org/node/2831499
    6x in EntityUpdateAddRevisionTranslationAffectedTest::testDeletedEntityType from Drupal\Tests\system\Functional\Update
    5x in EntityUpdateAddRevisionTranslationAffectedTest::testAddingTheRevisionTranslationAffectedField from Drupal\Tests\system\Functional\Update

  11x: The revision_user revision metadata key is not set for entity type: node See: https://www.drupal.org/node/2831499
    6x in EntityUpdateAddRevisionTranslationAffectedTest::testDeletedEntityType from Drupal\Tests\system\Functional\Update
    5x in EntityUpdateAddRevisionTranslationAffectedTest::testAddingTheRevisionTranslationAffectedField from Drupal\Tests\system\Functional\Update

  11x: The revision_created revision metadata key is not set for entity type: node See: https://www.drupal.org/node/2831499
    6x in EntityUpdateAddRevisionTranslationAffectedTest::testDeletedEntityType from Drupal\Tests\system\Functional\Update
    5x in EntityUpdateAddRevisionTranslationAffectedTest::testAddingTheRevisionTranslationAffectedField from Drupal\Tests\system\Functional\Update

  11x: The revision_log_message revision metadata key is not set for entity type: node See: https://www.drupal.org/node/2831499
    6x in EntityUpdateAddRevisionTranslationAffectedTest::testDeletedEntityType from Drupal\Tests\system\Functional\Update
    5x in EntityUpdateAddRevisionTranslationAffectedTest::testAddingTheRevisionTranslationAffectedField from Drupal\Tests\system\Functional\Update

  4x: Drupal\node\Plugin\Action\DeleteNode is deprecated in Drupal 8.6.x, will be removed before Drupal 9.0.0. Use \Drupal\Core\Action\Plugin\Action\DeleteAction instead. See https://www.drupal.org/node/2934349.
    2x in EntityUpdateAddRevisionTranslationAffectedTest::testAddingTheRevisionTranslationAffectedField from Drupal\Tests\system\Functional\Update
    2x in EntityUpdateAddRevisionTranslationAffectedTest::testDeletedEntityType from Drupal\Tests\system\Functional\Update

  2x: \Drupal::entityManager() is deprecated in Drupal 8.0.0 and will be removed before Drupal 9.0.0. Use \Drupal::entityTypeManager() instead in most cases. If the needed method is not on \Drupal\Core\Entity\EntityTypeManagerInterface, see the deprecated \Drupal\Core\Entity\EntityManager to find the correct interface or service. See https://www.drupal.org/node/2549139
    1x in EntityUpdateAddRevisionTranslationAffectedTest::testAddingTheRevisionTranslationAffectedField from Drupal\Tests\system\Functional\Update
    1x in EntityUpdateAddRevisionTranslationAffectedTest::testDeletedEntityType from Drupal\Tests\system\Functional\Update

  2x: Drupal\comment\Plugin\Action\PublishComment is deprecated in Drupal 8.5.x, will be removed before Drupal 9.0.0. Use \Drupal\Core\Action\Plugin\Action\PublishAction instead. See https://www.drupal.org/node/2919303.
    1x in EntityUpdateAddRevisionTranslationAffectedTest::testAddingTheRevisionTranslationAffectedField from Drupal\Tests\system\Functional\Update
    1x in EntityUpdateAddRevisionTranslationAffectedTest::testDeletedEntityType from Drupal\Tests\system\Functional\Update

  2x: Drupal\comment\Plugin\Action\SaveComment is deprecated in Drupal 8.5.x, will be removed before Drupal 9.0.0. Use \Drupal\Core\Action\Plugin\Action\SaveAction instead. See https://www.drupal.org/node/2919303.
    1x in EntityUpdateAddRevisionTranslationAffectedTest::testAddingTheRevisionTranslationAffectedField from Drupal\Tests\system\Functional\Update
    1x in EntityUpdateAddRevisionTranslationAffectedTest::testDeletedEntityType from Drupal\Tests\system\Functional\Update

  2x: Drupal\comment\Plugin\Action\UnpublishComment is deprecated in Drupal 8.5.x, will be removed before Drupal 9.0.0. Use \Drupal\Core\Action\Plugin\Action\UnpublishAction instead. See https://www.drupal.org/node/2919303.
    1x in EntityUpdateAddRevisionTranslationAffectedTest::testAddingTheRevisionTranslationAffectedField from Drupal\Tests\system\Functional\Update
    1x in EntityUpdateAddRevisionTranslationAffectedTest::testDeletedEntityType from Drupal\Tests\system\Functional\Update

  2x: Drupal\node\Plugin\Action\PublishNode is deprecated in Drupal 8.5.x, will be removed before Drupal 9.0.0. Use \Drupal\Core\Action\Plugin\Action\PublishAction instead. See https://www.drupal.org/node/2919303.
    1x in EntityUpdateAddRevisionTranslationAffectedTest::testAddingTheRevisionTranslationAffectedField from Drupal\Tests\system\Functional\Update
    1x in EntityUpdateAddRevisionTranslationAffectedTest::testDeletedEntityType from Drupal\Tests\system\Functional\Update

  2x: Drupal\node\Plugin\Action\SaveNode is deprecated in Drupal 8.5.x, will be removed before Drupal 9.0.0. Use \Drupal\Core\Action\Plugin\Action\SaveAction instead. See https://www.drupal.org/node/2919303.
    1x in EntityUpdateAddRevisionTranslationAffectedTest::testAddingTheRevisionTranslationAffectedField from Drupal\Tests\system\Functional\Update
    1x in EntityUpdateAddRevisionTranslationAffectedTest::testDeletedEntityType from Drupal\Tests\system\Functional\Update

  2x: Drupal\node\Plugin\Action\UnpublishNode is deprecated in Drupal 8.5.x, will be removed before Drupal 9.0.0. Use \Drupal\Core\Action\Plugin\Action\UnpublishAction instead. See https://www.drupal.org/node/2919303.
    1x in EntityUpdateAddRevisionTranslationAffectedTest::testAddingTheRevisionTranslationAffectedField from Drupal\Tests\system\Functional\Update
    1x in EntityUpdateAddRevisionTranslationAffectedTest::testDeletedEntityType from Drupal\Tests\system\Functional\Update

Technical debt with interest. @group legacy was added in order to get update tests passing, since they call deprecated code. https://www.drupal.org/node/2985785 The unfortunate side-effect being that the test runner won't tell us which code is using deprecated update-related code. This could be fixed by adding the specific deprecation messages at some point in the heirarchy, but I'm not sure that's possible. Anyway, that's out of scope here.

I'd say +1 on unwrapping the DbUpdatesTrait::includeUpdates() into the install files and then finding other uses and unwrapping it there. That way those entity test modules work with either BTB or WTB so we have BC for the unfortunate soul still using simpletest out there somewhere.

@Mile23 nice finds!

Updated entity_test_update.install, I don't find any further use of this Trait anywhere.

Reran EntityUpdateAddRevisionTranslationAffectedTest without @legacy and it came back green without the Drupal\system\Tests\Update\DbUpdatesTrait deprecation message (but with all the others still there obviously)

Looks good but \Drupal\Tests\system\Functional\Update\DbUpdatesTrait::includeUpdates doesn't have any usages now, so I'd suggest we deprecate that method as well because it is not possible to use it?

It's not possible to use it statically inside an install file, but you could use it if you just use the trait in a class and then self:: it, right? Not saying that is likely that anybody uses it that way, but they could....

Mostly, if the reasoning is that it can't be used anyway, why bother deprecating it, just toss it.

Mostly, if the reasoning is that it can't be used anyway, why bother deprecating it, just toss it.

+1 from me.

Nice find @Berdir, thanks for the feedback @alexpott.

Tossed.

Updated the CR for this, to indicate that \Drupal\system\Tests\Update\DbUpdatesTrait::includeUpdates has no replacement.

> It's not possible to use it statically inside an install file, but you could use it if you just use the trait in a class and then self:: it, right? Not saying that is likely that anybody uses it that way, but they could....

Yes, but it is about including update functions, they can not live anywhere else other than in .install, soo..

Committed ba5cf8e and pushed to 8.8.x. Thanks!

diff --git a/core/modules/system/tests/modules/entity_test/entity_test.install b/core/modules/system/tests/modules/entity_test/entity_test.install
index b61c24a69d..9a846afd62 100644
--- a/core/modules/system/tests/modules/entity_test/entity_test.install
+++ b/core/modules/system/tests/modules/entity_test/entity_test.install
@@ -56,5 +56,5 @@ function entity_test_schema() {
 
 $index = \Drupal::state()->get('entity_test.db_updates.entity_definition_updates');
 module_load_include('inc', 'entity_test', 'update/entity_definition_updates_' . $index);
-$index = \Drupal::state()->get( 'entity_test.db_updates.status_report');
+$index = \Drupal::state()->get('entity_test.db_updates.status_report');
 module_load_include('inc', 'entity_test', 'update/status_report_' . $index);
diff --git a/core/modules/system/tests/modules/entity_test_update/entity_test_update.install b/core/modules/system/tests/modules/entity_test_update/entity_test_update.install
index d807fc3ae2..7e709f1596 100644
--- a/core/modules/system/tests/modules/entity_test_update/entity_test_update.install
+++ b/core/modules/system/tests/modules/entity_test_update/entity_test_update.install
@@ -6,4 +6,4 @@
  */
 
 $index = \Drupal::state()->get('entity_test_update.db_updates.entity_rev_pub_updates');
-module_load_include('inc', 'entity_test_update', 'update/entity_rev_pub_updates_' . $index);
\ No newline at end of file
+module_load_include('inc', 'entity_test_update', 'update/entity_rev_pub_updates_' . $index);

Fixed up coding standards on commit.