Trigger a deprecation message when a deprecated module or theme is enabled

Hey @catch,maybe this patch can help you with that.

For the failing tests I've adding checks to exclude deprecate modules from the test so that the deprecation message is not generated. Otherwise, all those tests would have to be '@legacy'. For readability the longer if statements that were on one line are now multi-line.

Thanks for the patch @quietone! This issue was on my queue for a while, and I think, filtering the deprecated modules is the best approach for the tests. I also like the if statement changed to be multi-line, definitely improves the readability. For me it is almost ready for RTBC.

Just one question, do you think we should have a change record explaining the deprecation message?

I think it's a good idea @catch, I updated the https://www.drupal.org/node/3236434, now I'm +1 for RTBC.

@murilohp: Thanks for looking into this! I rolled back your edits on https://www.drupal.org/node/3236434 until this issue lands, since that is a published change record, so documenting this issue as if it happened is misleading. I think its fine to update it again once this issue lands, but not before.

Thanks for rolling back the edits and for the explanation @Gábor Hojtsy! Once this lands, I'll update the change record.

This looks like a wonderful patch!

+++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
@@ -125,6 +125,9 @@ public function install(array $module_list, $enable_dependencies = TRUE) {
       if ($module_data[$module]->info[ExtensionLifecycle::LIFECYCLE_IDENTIFIER] === ExtensionLifecycle::OBSOLETE) {
         throw new ObsoleteExtensionException("Unable to install modules: module '$module' is obsolete.");
       }
+      if ($module_data[$module]->info[ExtensionLifecycle::LIFECYCLE_IDENTIFIER] === ExtensionLifecycle::DEPRECATED) {
+        @trigger_error("The module '{$module_data[$module]->info['name']}' is deprecated.", E_USER_DEPRECATED);

+++ b/core/lib/Drupal/Core/Extension/ThemeInstaller.php
@@ -156,6 +156,10 @@ public function install(array $theme_list, $install_dependencies = TRUE) {
+          @trigger_error("The theme '{$theme_data[$theme]->info['name']}' is deprecated.", E_USER_DEPRECATED);

+++ b/core/tests/Drupal/KernelTests/Core/Extension/ModuleInstallerTest.php
@@ -146,4 +146,16 @@ public function testObsoleteInstall() {
+    $this->expectDeprecation("The module 'Deprecated module test' is deprecated.");

+++ b/core/tests/Drupal/KernelTests/Core/Theme/ThemeInstallerTest.php
@@ -153,6 +153,18 @@ public function testInstallThemeWithUnmetModuleDependencies($theme_name, $instal
+    $this->expectDeprecation("The theme 'Deprecated theme test' is deprecated.");

I would use $module and $theme directly in the error. I believe that is the machine name. This error is triggered for tests to fail, so the machine name would be more accurate for that scenario to figure out why things fail IMHO.

#10. the error messages are now using the module or theme machine name. I also added the lifecycle_link to the message.

Since Drupal\Tests\system\Functional\Module\NonStableModulesTest and Drupal\Tests\system\Functional\System\StatusTest now install deprecated modules and themes, and both deprecated modules and themes trigger a deprecation message, maybe we should add a @group legacy into the tests, I tried here, let's see if it pass now.

+++ b/core/modules/system/tests/src/Functional/Module/NonStableModulesTest.php
@@ -44,6 +44,8 @@ protected function setUp(): void {
+   * @group legacy

This is testing the install of experimental and should not have to be legacy. This is needed because the file is using $modules with a deprecated module. I think that declaration needs to be removed and all the tests reviewed to find out where it needs to be installed.

I'll have a look.

This removed the installation of 'deprecated_module_test' during the setup.

Doing that results in failures in testDeprecatedConfirmForm which I tracked down to the fact that the test module, 'deprecated_module_contrib' is considered to be a core module not a contrib module. Then I found that module 'deprecated_module_test' had deprecated_module_test_system_info_alter with a comment indicating that it should be in 'deprecated_module_contrib'. So I moved that but then found that the hook is not called in the test. If it was then the module would be seen as contrib, which it should be. I don't know why the hook is not called. Anyone?

The patch looks good and is almost ready for RTBC.

I have just 1 remark:

+++ b/core/tests/Drupal/KernelTests/Core/Asset/ResolvedLibraryDefinitionsFilesMatchTest.php
@@ -103,7 +104,12 @@ protected function setUp(): void {
+        || $module->info[ExtensionLifecycle::LIFECYCLE_IDENTIFIER] === ExtensionLifecycle::EXPERIMENTAL

Why do we add this line. It looks like it is out of scope for this issue. If not, then please explain.

@daffie, thanks. Just wish I could figure out why deprecated_module_contrib_system_info_alter is not executed in the failing test.

The explanation for the additional checking in seeming unrelated tests is in comment #4.

A bit of empirical testing and I found that the hook is only executed when the lifecyle is 'stable'. That needs to be fixed.

@quietone: good find! that sounds like would be its own issue?

@Gábor Hojtsy, thanks, but fortunately I was wrong. More on that in a bit.

First, this needs a reroll.

There is no problem with the hooks running. My mistake was thinking the test was installing the module, when, in fact, it was just getting the confirm form.

Previously, testDeprecatedConfirmForm was working because a hook in deprecated_module_test, which is installed in the test, modifies the info for deprecated_module_contrib. There were no comments in the test to explain this behavior so when I moved the hook I broke it. Perhaps others more familiar with hook_system_info_alter would have realized that. But I think it would be better for the test to be explicit about the location of a module that should be considered contrib. Therefor, I added a few lines to move deprecated_module_contrib from the test directory to the modules/contrib in the test.

Well, I didn't think that through did I?

This restores the hook and adds a comment explaining why 'deprecated_module' must be installed.

Darn, the test passed locally because I wasn't working on a clean environment.

This updates the comments and Installs the correct module to get the hook to run.

And lets fix the last failing test.

Thanks for the patch @quietone, it looks good to me, I just noticed some minor stuff.

1. +++ b/core/modules/system/tests/src/Functional/Module/NonStableModulesTest.php
   @@ -315,18 +310,29 @@ public function testDeprecatedConfirmForm(): void {
   +    // Test installing a deprecated core  module and a deprecated contributed
   

   There's two spaces between "core" and "module" words

2. +++ b/core/modules/system/tests/src/Functional/Module/NonStableModulesTest.php
   @@ -315,18 +310,29 @@ public function testDeprecatedConfirmForm(): void {
   +    // a 'project' not 'core. This test must be done with
   

   I think you maybe missed a quote here on 'core

Thanks for the patch @elber! However, I really don't know what happened, your patch removed a lot of stuff. I made this new one addressing my previous comment(#30).

+++ b/core/modules/system/tests/src/Functional/Module/NonStableModulesTest.php
@@ -315,18 +310,29 @@ public function testDeprecatedConfirmForm(): void {
+    // Test installing a deprecated core module and a deprecated contributed
+    // module. There should be a confirmation form with a deprecated warning for
+    // a 'project' not 'core'. This test must be done with
+    // 'deprecated_module_test' installed because 'deprecated_module_test' uses
+    // hook_system_info_alter to set the origin of 'deprecated_module_contrib'
+    // to something other than 'core'.

Great that we add this kind of testing, only I cannot find anything about it in the IS or anywhere else for that matter. Could somebody explain this?

@daffie: what do you think is missing from the issue summary? Explain the internals of the test? We usually don't do that?

I agree with #37 -- I think the test coverage comment is self-explanatory and consistent with existing patterns in core. Leaving the IS tag on in case anyone has ideas for improvements, but moving back to NR. This is quite important for the Drupal 10 requirements.

(Removing credit for broken patch.)

@daffie: https://api.drupal.org/api/drupal/core%21modules%21system%21tests%21modu... is the code for making deprecated_module_test appear like it is not a core module.

There could be multiple techniques for this, however this is a pre-existing test :) Upgrade status uses the project key in the info.yml file, however that module itself only uses that to decide whether something is contrib or not.

I think the patch looks good on another review, thanks all!

Needs CR and release note please. :)

Also, I was expecting runtime to also be included in the scope of this. I'd be okay with that as a followup. If there are reasons runtime is not included, please document them. Thanks!

Started a change record.

A few more review points:

1. +++ b/core/modules/config/tests/src/Functional/ConfigImportAllTest.php
   @@ -47,9 +48,13 @@ public function testInstallUninstall() {
   +      // Filter contrib, hidden, already enabled modules, deprecated and modules
   +      // in the Testing package.
   
   +++ b/core/modules/jsonapi/tests/src/Functional/TestCoverageTest.php
   @@ -33,13 +33,14 @@ protected function setUp(): void {
   +      // Filter out contrib, hidden, testing, experimental, and deprecated
   +      // modules. We also don't need to enable modules that are already enabled.
   

   This is a bit of a jumble, grammatically. We could instead use the text from the JSON:API test in the next hunk:

   // Filter out contrib, hidden, testing, experimental, and deprecated
   +      // modules. We also don't need to enable modules that are already enabled.

2. +++ b/core/modules/system/tests/src/Functional/Module/InstallUninstallTest.php
   @@ -60,9 +60,13 @@ public function testInstallUninstall() {
   -    // Test uninstalling without hidden, required, and already enabled modules.
   +    // Test uninstalling without hidden, required, deprecated, and already
   +    // enabled modules.
   

   We could also reuse that same comment here:
   "Test uninstalling modules. Filter out..."

3. +++ b/core/modules/system/tests/src/Functional/Module/NonStableModulesTest.php
   @@ -315,18 +310,29 @@ public function testDeprecatedConfirmForm(): void {
   +    // Test installing a deprecated core module and a deprecated contributed
   +    // module. There should be a confirmation form with a deprecated warning for
   +    // a 'project' not 'core'. This test must be done with
   

   "There should be a confirmation form with a deprecated warning for a 'project' not 'core'" does not make sense. Maybe it should be:

   There should be a confirmation form with a deprecated warning for a module with a 'project' other than 'core'.

   (If that is what is meant?)

   Also, I assume warnings for a deprecated module that is part of core are tested elsewhere already? Or if not, why not?

4. +++ b/core/modules/system/tests/src/Functional/Module/NonStableModulesTest.php
   @@ -315,18 +310,29 @@ public function testDeprecatedConfirmForm(): void {
   +    // hook_system_info_alter to set the origin of 'deprecated_module_contrib'
   

   hook_system_info_alter() should have parens.

5. +++ b/core/tests/Drupal/KernelTests/Core/Extension/ModuleInstallerTest.php
   @@ -146,4 +146,16 @@ public function testObsoleteInstall() {
   +  public function testDeprecatedInstall() {
   +    $this->expectDeprecation("The module 'deprecated_module' is deprecated. See http://example.com/deprecated");
   +    $this->container->get('module_installer')->install(['deprecated_module']);
   +  }
   

   Should it also test that the module was installed successfully despite being deprecated?

6. +++ b/core/tests/Drupal/KernelTests/Core/Theme/ThemeInstallerTest.php
   @@ -153,6 +153,18 @@ public function testInstallThemeWithUnmetModuleDependencies($theme_name, $instal
   +  public function testInstallDeprecated() {
   +    $this->expectDeprecation("The theme 'deprecated_theme_test' is deprecated. See https://example.com/deprecated");
   +    $this->themeInstaller()->install(['deprecated_theme_test']);
   

   Should it also test that the theme was installed successfully?

I'm quite glad this issue is happening! :D

This patch is fixing the items in #43

1. Fixed
2. Fixed
3. Reworked the comments for this new section. Hopefully easier to understand.
There are existing tests for core deprecated modules. The addition is this issue is adding tests for deprecated modules that do not have origin: core.
4. Fixed
5. Added
6. Added

Follow up made, #3262417: Trigger a deprecation message when a deprecated module or theme is used at runtime

I reviewed the CR. Mostly looked good, but I gave it a little cleanup. Removing that tag.

Also, the summary now has a reasonable release note draft, removing that tag, too.

Confirming that the interdiff in #44 addresses all the review points in #43. Almost there! A few other possible concerns / nits:

1. +++ b/core/tests/Drupal/KernelTests/Core/Extension/ModuleInstallerTest.php
   @@ -146,4 +146,17 @@ public function testObsoleteInstall() {
   +    $this->container->get('module_installer')->install(['deprecated_module']);
   +    $this->assertTrue(\Drupal::service('module_handler')->moduleExists('deprecated_module'));
   

   While this works, it's a little jarring to have one line reading from $this->container and the very next one using \Drupal::service(). I know there's an open, unresolved debate about this, but my understanding is that the convention / recommendation is to use \Drupal::service() in tests, not $this->container, to avoid weird hard to track down bugs with when the container is rebuilt in tests. Regardless of how you feel about that debate, doing both approaches in the same test seems a bit weird. Can we use \Drupal::service() consistently for all this?

2. +++ b/core/tests/Drupal/KernelTests/Core/Theme/StableLibraryOverrideTest.php
   @@ -63,9 +63,14 @@ protected function setUp(): void {
   +      // Filter contrib, hidden, experimental, deprecated, and already enabled
   +      // modules, and modules in the Testing package.
   +      if ($module->origin !== 'core'
   +        || !empty($module->info['hidden'])
   +        || $module->status == TRUE
   +        || $module->info['package'] == 'Testing'
   +        || $module->info[ExtensionLifecycle::LIFECYCLE_IDENTIFIER] === ExtensionLifecycle::EXPERIMENTAL
   +        || $module->info[ExtensionLifecycle::LIFECYCLE_IDENTIFIER] === ExtensionLifecycle::DEPRECATED) {
   

   We see this big identical if() multiple times in this patch. Rule of Three(tm)? Can we share this somewhere reasonable? E.g. somewhere in Drupal\Core\Extension? Or is that scope creep for the task at hand?

That's all I can find. Everything else looks great. Perhaps this is already RTBC and these points can be ignored. But I think at least 1 would be an improvement, even if #2 is better as a follow-up. Thanks much!

46.1 Fixed. I didn't even notice that.
46.2 There are actually two versions of the big 'if' statement; most exclude Deprecated but one does not. Since there are in tests I don't see the need to create a helper somewhere to share the code. Like you I don't know where it would go either. It may be long but it is easy to read.

1. Thanks.
2. Yeah, agree. There’s really no good spot, and it’s kinda scope creepy.

Everything looks good. No further concerns. RTBC!

Thanks again,
-Derek

Well, you could have a BaseTest for those which would deduplicate a lot. But since you RTBC'd, i'll guess ill keep the patch on my machine ;)

Ok, since it needs a reroll i'll post the optimized version. First the 9.4.x version, and ill get to work on the reroll right now.

The reroll for 10.0.x, seems there was something weird applying one of the files. The resulting patch is pretty much the same though.

The Base test class should not be run as a test. Changed to abstract as that is how jsonapi also does it.

Updated class name as suggested.

And the patch as applied to the 10.0.x branch. But it seems to be the same... The interdiff is the same.

The duplication cost of that method is pretty minor. And other than that the duplication in the TemplateOverrideTest is pretty minimal. Notice that the TemplateOverrideTest doesnt extend the testbase, the code that was deduplicated was mostly in the StableLibraryOverrideTest.

So i think introducing a new class for only those 2 lines is overkill imo.

It could use follow-up to display top-3 deprecation warnings from code

Oops, one point above didn't quite get fixed:

1. +++ b/core/modules/system/tests/src/Functional/Module/InstallUninstallTest.php
   @@ -60,8 +60,8 @@
   -    // Test uninstalling without hidden, required, deprecated, and already
   -    // enabled modules.
   +    // Filter contrib, hidden, already enabled modules, deprecated and modules
   +    // in the Testing package.
   

   Looks like this copied the jumbled comment instead of the un-jumbled one. It should be:

   // Filter out contrib, hidden, testing, experimental, and deprecated
   // modules. We also don't need to enable modules that are already enabled.

2. +++ b/core/modules/system/tests/src/Functional/Module/NonStableModulesTest.php
   @@ -319,20 +319,24 @@
   +    // Now, test when installing a non-core deprecated module alone and then
   +    // with a core deprecated module. First, install 'deprecated_module_test'
   +    // because it uses hook_system_info_alter() to set the origin of
   +    // 'deprecated_module_contrib' to something other than 'core'.
   

   This makes sense, thanks!

BTW, there are actually three hunks where we filter out certain kinds of modules, not just two. We could add an ExtensionListTestTrait::filter() or something. But that could be a followup.

Thanks @xjm.

63.1: Ok fair enough, let me prepare a new patch.
63.2: Follow-up would be better i think for now imo.

Added d9 and d10 patch.

I took a look at the optimizations.

1. +++ b/core/tests/Drupal/KernelTests/Core/Theme/StableLibraryOverrideTestBase.php
   @@ -0,0 +1,141 @@
   +  protected function enableAllCoreModules(): void {
   

   This title is not correct. The method filters our experimental modules, which can and are core modules, for example, ckeditor5, field_layout, help_topics, workspaces. It also filters out hidden, and migrate_drupal_multilingual is a core module and hidden.

2. +++ b/core/tests/Drupal/KernelTests/Core/Theme/StableLibraryOverrideTestBase.php
   @@ -0,0 +1,141 @@
   +   *   name, and so on.
   

   'and so on' is vague. At least, I do not know what that means.

3. +++ b/core/tests/Drupal/KernelTests/Core/Theme/StableLibraryOverrideTestBase.php
   @@ -0,0 +1,141 @@
   +   *   name, and so on.
   

   And again

4. +++ b/core/tests/Drupal/KernelTests/Core/Theme/StableLibraryOverrideTestBase.php
   @@ -0,0 +1,141 @@
   +        || $module->status == TRUE
   ...
   +    $this->allModules[] = 'system';
   +    $this->allModules[] = 'user';
   +    $this->allModules[] = 'path_alias';
   

   This method filters out the installed modules and then adds just these three. Why just these three modules?

5. +++ b/core/tests/Drupal/KernelTests/Core/Theme/StableLibraryOverrideTestBase.php
   @@ -0,0 +1,141 @@
   +    $this->themeManager = $this->container->get('theme.manager');
   +    $this->themeInitialization = $this->container->get('theme.initialization');
   +    $this->libraryDiscovery = $this->container->get('library.discovery');
   

   These do not fit with the title of the method. The should go into the setup().

Forgot to set to NW.

1. I've updated the method name: enableAllCoreModules => enableVisibleAndStableCoreModules.
2. name, and so on. => name, then by asset type.
3. name, and so on. => name, then by asset type.
4. I have not questioned the existing code, only restructured it a little. It is commited in the original issue introducing this class, see https://git.drupalcode.org/project/drupal/-/commit/250534625ec2b61ef8694... (https://www.drupal.org/project/drupal/issues/3050374). Not sure why, i don't see any mention in that issue regarding the why. We could investigate, but in my opinion, don't examine this in this issue.
5. Good point! I've moved the intialization to the setup method. Hopefully this doesnt mean it tried to get a service of something not installed. But we'll see :)

Added the 9.4 patch, 10.x will follow soon.

I missed an return type in the d9 patch that triggers an error in d10. I'd rather keep the patches as close to eachother as possible so uploading a new one for d9.

Yeah i was afraid this would happen. The modules of those services are not loaded yet. There are 2 options. Duplicate the initialization to the setUp methods of the tests, or move them back to the new method.

Since the initilization of the services is not really something that makes sense in the context of the method, i'll opt for adding it to the setUp's of the 2 tests.

The patches, also removed the setUp method from the testbase since that is no longer needed.

All code changes look good to me.
The added trigger_errors() has testing.
For me it is RTBC.

The stuff with adding @group legacy is a bit strange as those test method would normally be removed in a new major version. @catch Said that it is going to be changed in #3263109: Use PHPStan for deprecation checking.

This can we fixed on commit.

+++ b/core/modules/system/tests/src/Functional/Module/InstallUninstallTest.php
@@ -60,9 +60,13 @@ public function testInstallUninstall() {
+        || $module->status == TRUE || $module->info['package'] == 'Testing'

Why is this on a single line? For both patches.

Ideally, let's not "fix on commit" (for #79). This puts certain changes outside our review process and puts pressure on the committer to context-switch between reviewing/committing and editiing files. So that can be addressed on top of the reroll. Thanks!

I'll reroll and fix-before-fixing-on-commit...

Back to RTBC.

I've opened #3264811: Add FileSystemModuleDiscoveryDataProviderTrait::coreStableModuleListDataProvider as a follow-up. Either calling FileSystemModuleDiscoveryDataProviderTrait::coreModuleListDataProvider should filter out deprecated modules or something. Calling and enabling all the modules provided by it in a functional test (in contrib) causes test failures. Core only uses it in Kernel tests.