Add 'lifecycle' key to .info.yml files

This would have to happen in 9.1.x at this point, but it would simplify the process of marking modules deprecated in preparation for removal etc. so +1.

I think hidden key should remain, as an extension can be both experimental and hidden. Here's a start, focusing on the status key and experimental part.

This patch explores Introduce a trait to enable modules for tests, however it should perhaps be done in a separate issue, so the patch from #6 is still the correct one to continue the work on.

Setting to NR for test bot, not an actual review.

#7

I'm not sure I understand, do you mean that an extension can be either hidden or experimental, but not both?

and $module->info['status'] should be gone with it

Can you elaborate on what you mean here?

Adding updated patch based on https://www.drupal.org/project/drupal/issues/3124762#comment-13554512 to use $module->info['status'] == 'hidden' for testing hidden status.

Adding another fix based on the patch in #6.

Please ignore the above patch in #19. Missed fixing a few other instances. Adding final updated patch below based on #6

Hi @rpsu
Made changes as you suggested in #22 please review.

Patch failed to apply. Re-rolled patch please review.

Interesting idea. Glad I happened to notice this issue!

#28 is a bogus reroll, missing most of the code from previous patches. So this needs a real re-roll.

Also, y'all should inject a 'status' field into some of the update manager test fixtures and see how badly those tests start failing. :( Update manager does its own thing with 'status' in the info files. So this needs a *lot* of work. Either:

A) Change this key to something else.
B) Change all of update manager to use 'update_status' in all the places it currently uses 'status'.

Edit: I was wrong, see below.

Oh yay, I love it when I'm wrong about stuff like this. ;)

Luckily for everyone, update_process_project_info() doesn't actually look at or copy a status key out of the .info files. So we're safe there.

Also, core/lib/Drupal/Core/Utility/ProjectInfo.php cares about a 'status', but that's the enabled/disabled bit, not something from the .info file. So that could be confusing, but I don't think it'll actually break this.

Carry on. ;)

Thanks/sorry,
-Derek

Sorry for that. Here is a new re-roll please review.

Excellent, thanks @Suresh Prabhu Parkala! Much better. ;)

Some general concerns on this now that I'm looking more closely:

1. +++ b/core/lib/Drupal/Core/Extension/ExtensionStatus.php
   @@ -0,0 +1,35 @@
   +
   +  /**
   +   * Extension is experimental and should be hidden from the user interface.
   +   */
   +  const HIDDEN = 'hidden';
   

   This doesn't make sense to me. Core uses 'hidden' for:

   • Testing modules
   • Experimental modules
   • Base themes

   Seems weird to have this be a separate status, since we can want it independently of the other values in here.

2. +++ b/core/lib/Drupal/Core/Extension/ExtensionStatus.php
   @@ -0,0 +1,35 @@
   +  /**
   +   * Extension is deprecated and installation should be prevented.
   +   */
   +  const DISABLED = 'disabled';
   

   This is new behavior. I'm not totally sure how this works.

   Also, core's been really confused over the terminology of "enable"/"disable" vs. "install/uninstall", etc. I fear this is going to rehash some of that pain, too.

3. +++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
   @@ -91,6 +92,9 @@ public function install(array $module_list, $enable_dependencies = TRUE) {
   +      if ($module_data[$module]->info['status'] === ExtensionStatus::DISABLED) {
   +        throw new DisabledExtensionException("Unable to install modules: module '$module' is disabled.");
   +      }
   

   Interesting. This definitely needs tests. Probably a usability review, too.

4. +++ b/core/modules/system/tests/src/Functional/Module/InstallUninstallTest.php
   @@ -211,7 +212,7 @@ public function testInstallUninstall() {
   @@ -294,7 +295,7 @@ protected function assertInstallModuleUpdates($module) {
   
   @@ -294,7 +295,7 @@ protected function assertInstallModuleUpdates($module) {
        $all_update_functions = $post_update_registry->getPendingUpdateFunctions();
        $empty_result = TRUE;
        foreach ($all_update_functions as $function) {
   -      list($function_module,) = explode('_post_update_', $function);
   +      [$function_module,] = explode('_post_update_', $function);
          if ($module === $function_module) {
            $empty_result = FALSE;
            break;
   

   This is out of scope and should be rolled back to make this patch easier to review.

5. +++ b/core/tests/Drupal/KernelTests/Core/Theme/Stable9LibraryOverrideTest.php
   @@ -64,7 +65,7 @@ protected function setUp(): void {
   -      if ($module->origin !== 'core' || !empty($module->info['hidden']) || $module->status == TRUE || $module->info['package'] == 'Testing' || $module->info['package'] == 'Core (Experimental)') {
   +      if ($module->origin !== 'core' || $module->info['status'] === ExtensionStatus::HIDDEN || $module->status == TRUE || $module->info['package'] == 'Testing' || $module->info['status'] === ExtensionStatus::EXPERIMENTAL) {
   
   +++ b/core/tests/Drupal/KernelTests/Core/Theme/Stable9TemplateOverrideTest.php
   @@ -63,7 +64,7 @@ protected function installAllModules() {
   -      if ($module->origin !== 'core' || !empty($module->info['hidden']) || $module->status == TRUE || $module->info['package'] == 'Testing' || $module->info['package'] == 'Core (Experimental)') {
   +      if ($module->origin !== 'core' || $module->info['status'] === ExtensionStatus::HIDDEN || $module->status == TRUE || $module->info['package'] == 'Testing' || $module->info['status'] === ExtensionStatus::EXPERIMENTAL) {
   
   +++ b/core/tests/Drupal/KernelTests/Core/Theme/StableLibraryOverrideTest.php
   @@ -64,7 +65,7 @@ protected function setUp(): void {
   -      if ($module->origin !== 'core' || !empty($module->info['hidden']) || $module->status == TRUE || $module->info['package'] == 'Testing' || $module->info['package'] == 'Core (Experimental)') {
   +      if ($module->origin !== 'core' || $module->info['status'] === ExtensionStatus::HIDDEN || $module->status == TRUE || $module->info['package'] == 'Testing' || $module->info['status'] === ExtensionStatus::EXPERIMENTAL) {
   
   +++ b/core/tests/Drupal/KernelTests/Core/Theme/StableTemplateOverrideTest.php
   +++ b/core/tests/Drupal/KernelTests/Core/Theme/StableTemplateOverrideTest.php
   @@ -61,7 +62,7 @@ protected function installAllModules() {
   -      if ($module->origin !== 'core' || !empty($module->info['hidden']) || $module->status == TRUE || $module->info['package'] == 'Testing' || $module->info['package'] == 'Core (Experimental)') {
   +      if ($module->origin !== 'core' || $module->info['status'] === ExtensionStatus::HIDDEN || $module->status == TRUE || $module->info['package'] == 'Testing' || $module->info['status'] === ExtensionStatus::EXPERIMENTAL) {
   

   I think we need to continue to honor the 'hidden' info property in all of these if()s, regardless of if we end up *also* having a 'hidden' status.

Tagging for tests (at the bare minimum, for the new behavior with trying to install a 'disabled' module, probably wouldn't hurt to have explicit tests for all the status values). We should confirm we've got existing test coverage that 'Experimental' gets grouped on the 'extend' page correctly.

Also tagging for a UX review, specifically on the behavior around 'disabled'.

Thanks again,
-Derek

I don't really understand what the difference is between "deprecated" and "disabled" here. Once a module is considered "deprecated" do we really want to allow new installs? When would we move a module from "deprecated" to "disabled"?

+100 to #34. From the current proposed resolution:

status: [hidden|experimental|released|deprecated||disabled]

I'd propose:

1. hidden: Remain a separate attribute, since it's independent of status.
2. disabled: Merge with 'deprecated'.
3. Should we add a "testing" status so we don't have to re-use the package for that? That way, we wouldn't have to lump all testing modules into a single package across core and contrib.

Also tagging for a summary update (needed before a UX review).

Thanks,
-Derek

Other thoughts:

1. Re: #32.1: I should have said "(some) Experimental modules". In fact, at this point it's just for demo_umami_content. Most experimental modules are visible (but not all).
2. If we introduce "status: testing", that would imply "hidden: true" (unless 'extension_discovery_scan_tests' setting is TRUE). Otherwise, you'd need "hidden: true" when appropriate (demo_umami_content, base themes, etc).
3. If we prevent new install of deprecated modules by default, there should probably be a site setting to override that and let you install it if needed. E.g. you're still using it, someone accidentally uninstalls, and now you're not allowed to re-enable it? Maybe we should never prevent you from installing, only generate warning messages about it (kinda like we do with 'experimental' already)?

Thanks,
-Derek

#35-36 What if we want to deprecate a testing module. Maybe we never do that :-)

I also agree with hidden being a separate attribute!

I don't see why hidden needs to remain independent of status?

• released: Normal module (also the default, if status key not present)
• experimental: Shown in the module listing as experimental, can be installed
• hidden: Not shown in the module listing, but can be installed
• deprecated: Shown in the module listing if installed, not shown if not installed, cannot be installed

Does this miss any use cases? Does demo_umami_content need to be "experimental" if it's hidden anyway?

Bikeshed time: does "released" make most sense here, do we just want "normal" or "default" or "available"?

I like the separation and I think of 'hidden' more as a behaviour then a status. The module can be experimental, but should be hidden, because there is currently no UI.
The module is deprecated, you shouldn't install it, but you can i.e. #36.2, but we're also hiding it to ensure you think about it twice :)

1. +++ b/core/lib/Drupal/Core/Extension/ExtensionStatus.php
   @@ -0,0 +1,35 @@
   +  /**
   +   * Extension is released. This is the default value of any extension.
   +   */
   +  const RELEASED = 'released';
   

   I agree with @longwave; "Released" doesn't make sense to me here. Everything they'll get in core releases has been "released" at least as beta, and most of the other statuses are stable statuses. I'd just call it "normal".

2. +++ b/core/lib/Drupal/Core/Extension/ExtensionStatus.php
   @@ -0,0 +1,35 @@
   +  /**
   +   * Extension is deprecated and installation should be prevented.
   +   */
   +  const DISABLED = 'disabled';
   

   This briefly terrified me that we were reintroducing disabled module status. What it actually is is that installation is prevented, so that sites with it already installed don't have fatal errors, but sites that don't already have it installed can't add it.

   We did this for the SimpleTest empty shell retroactively, but I'm not sure it's something we want as a typical practice?

3. +++ b/core/modules/field_layout/field_layout.info.yml
   @@ -1,7 +1,8 @@
   -package: Core (Experimental)
   +package: Core
   +status: experimental
   

   So, listing experimental modules in a separate package in the UI was a deliberate choice for UX. I think either the experimental status should magically put them in that package, or we should retain the Core (Experimental) package (but let the status key be what causes any other UI magic.

4. I agree that "hidden" probably needs to stay separate. Experimental modules might be hidden, or not, as might normal modules.

5. I think deciding whether or not to move "testing" into this could be a followup issue.

6. Are we planning to define how core responds to the deprecated status in a followup or such?

@catch suggested "obsolete" instead of "disabled" for the non-installable status.

> @catch suggested "obsolete" instead of "disabled" for the non-installable status.

+1 to that.

Or what about just 'deprecated', since that is a term we already use, and the comment says that already.

@joachim:

So deprecated would be used for something like aggregator if we move it to contrib - but in 9.x.x we don't want to prevent people installing it in a new minor release - it'll be supported in core until 9.x EOL. Sites don't need to uninstall aggregator before updating to 10.0.x, they need to add the contrib module to their repo so it can take over from core's version.

Obsolete is for something like entity_reference where we moved the functionality into core and all that's left is a stub, that needs to be uninstalled, then eventually removed from the repo altogether. In this case once the update hook to uninstall is in, we also need to prevent it being installed again (so that it doesn't go 'missing' when it's removed from the repo). As well as preventing install, this could probably also hide it from the UI.

There might be a case where a module starts as deprecated, then goes to obsolete, but I think they're two distinct states.

Agreed with "obsolete" and the separation of "obsolete" from "deprecated".

First a reroll.

Now working on the feedback since #31.

• Renamed ExtensionStatus::RELEASED to ExtensionStatus::NORMAL.
• Renamed ExtensionStatus::DISABLED to ExtensionStatus::OBSOLETE.
• Reverted hidden back to its own attribute.
• Reverted some out-of-scope changes.

Still need to write a test, but I think that would be easier once we have some consensus on the usability / intended behaviours.

Just 2 observations:

1. +++ b/core/lib/Drupal/Core/Extension/ExtensionStatus.php
   @@ -0,0 +1,30 @@
   +  const EXPERIMENTAL = 'experimental';
   ...
   +  const NORMAL = 'normal';
   ...
   +  const DEPRECATED = 'deprecated';
   ...
   +  const OBSOLETE = 'obsolete';
   

   Can we put these constant in an alphabetical order.

2. +++ b/core/modules/field_layout/field_layout.info.yml
   @@ -1,7 +1,8 @@
   -package: Core (Experimental)
   +package: Core
   
   +++ b/core/modules/help_topics/help_topics.info.yml
   @@ -1,7 +1,8 @@
   -package: Core (Experimental)
   +package: Core
   
   +++ b/core/modules/migrate_drupal_multilingual/migrate_drupal_multilingual.info.yml
   @@ -1,7 +1,8 @@
   -package: 'Core (Experimental)'
   +package: 'Core'
   
   +++ b/core/modules/system/tests/modules/experimental_module_requirements_test/experimental_module_requirements_test.info.yml
   @@ -1,5 +1,6 @@
   -package: Core (Experimental)
   +package: Core
   
   +++ b/core/modules/system/tests/modules/experimental_module_test/experimental_module_test.info.yml
   @@ -1,5 +1,6 @@
   -package: Core (Experimental)
   +package: Core
   
   +++ b/core/modules/workspaces/workspaces.info.yml
   @@ -2,7 +2,8 @@ name: Workspaces
   -package: Core (Experimental)
   +package: Core
   
   +++ b/core/profiles/demo_umami/modules/demo_umami_content/demo_umami_content.info.yml
   @@ -2,7 +2,8 @@ name: 'Umami demo: Content'
   -package: 'Core (Experimental)'
   +package: 'Core'
   

   I think that these changes are wrong. On the page where you can install modules /admin/modules the experimental core modules have their own section: "Core (Experimental)". After this change they will be merged into one section with all other core modules.

Regarding #49.2

I too agree that we should retain the "package: Core (Experimental)" as was pointed in #41.3 that it was a deliberate choice for UX (however reference to this decision certainly will help) and unless we have a way to address that we should retain this.

Sorted the constants name in core/lib/Drupal/Core/Extension/ExtensionStatus.php .
And reverted the packages as they were in .info.yml files.

1. +++ b/core/lib/Drupal/Core/Extension/ExtensionStatus.php
   @@ -0,0 +1,31 @@
   + * Extension statuses.
   ...
   +   * Extension is deprecated but fully functional.
   ...
   +   * Extension is experimental.
   ...
   +   * Extension is normal. This is the default value of any extension.
   

   I think we can expand a little more here

2. +++ b/core/lib/Drupal/Core/Extension/ExtensionStatus.php
   @@ -0,0 +1,31 @@
   +class ExtensionStatus {
   

   This class can be final

Fixed #49.1.

Regarding #49.2
I believe more suggestions needed.

Please ignore the above comment #54, added by mistake.

Sorry for the noise something unusual happened on my system.
Adding the related issue as per #53.

@adityasingh somehow you're unpablshed latest patch #51

The current "unpublished" patch still needs tests.

[bikeshed warning]

Re: #49.1:

Can we put these constant in an alphabetical order.

FWIW, the previous order was the "logical" order in the lifecycle of a core module:

1. Starts "experimental".
2. Stabilizes and goes "normal".
3. Eventually (maybe), becomes "deprecated" on the way out.
4. Finally (maybe), becomes "obsolete" and can't be enabled anymore...

So I think the original order made more sense. Not everything has to be alphabetical all the time. Perhaps we should add a comment above the four to this effect?

Thoughts?

Please don't re-roll this patch to undo the change until at least @daffie replies to this. ;)

Thanks,
-Derek

So I think the original order made more sense. Not everything has to be alphabetical all the time. Perhaps we should add a comment above the four to this effect?

I agree. And make sure that we add the comment with the lifecycle of a core module story.

This at least fixes #59 and #60. Leaving NW since there aren't new tests yet.

Re-reading the thread, it looks like #52 wasn't addressed. This fixes both points.

Seems like we need to set simpletest and entity_reference to status 'obsolete' as part of this issue. Would be weird to add pluming for these status values but not actually set the status accurately at the same time. Seems like both of those qualify for 'obsolete' given the current definitions. Also updating the descriptions accordingly so we're consistent with terminology.

We should probably also add the code to generate the warnings on the status page automatically, and remove them from the *.install files of the non-normal modules.

Re:

We should probably also add the code to generate the warnings on the status page automatically, and remove them from the *.install files of the non-normal modules.

Hrm, but if we do that, we either need to come up with a generic message to always display, or we need to introduce yet another .info key for 'status message' or something. That wouldn't be as easily translatable, either (right?). :( For example, we have:

function entity_reference_requirements($phase) {

  $requirements = [];

  if ($phase === 'install') {
    $requirements['entity_reference'] = [
      'title' => t('Entity Reference: Deprecated'),
      'description' => t('Entity Reference is deprecated, all functionality has been moved to Core.'),
      'value' => \Drupal::VERSION,
      'severity' => REQUIREMENT_ERROR,
    ];
  }

  return $requirements;
}

It's even worse for simpletest_requirements(), which includes placeholders in the message:

      'title' => t('SimpleTest'),
      'description' => t('SimpleTest has been removed from Drupal 9.0.0 and can no longer be installed. A contributed module is available for those who wish to continue using SimpleTest during the transition from Drupal 8 to 9. <a href=":change-record">See the change record for more information</a>.', [
	':change-record' => 'https://www.drupal.org/node/3091784',
      ]),

Maybe doing this automatically is more trouble than it's worth? Generalizing it might over-complicate it? Doing it in PHP code in each .install file gives us full translatability, and full flexibility to do whatever we need in each specific case. Should we remove that aspiration from the proposed resolution and scope of this issue? Or do we want to figure out how to make it generalizable?

Meanwhile, took a pass at updating the summary. It'll need further edits as this moves forward, but it's accurate / current for now.

Fix summary formatting bugs.

Here's a Kernel test for ObsoleteExtensionException. This will probably need more tests before it's done, but removing the tag for now and back to NR.

Cool, that fleshed out some hidden bugs in our test suite. ;)

1. core/modules/field/tests/src/Functional/EntityReference/EntityReferenceFileUploadTest.php doesn't need to install entity_reference.module for it to pass.
2. core/modules/help_topics/tests/src/Functional/HelpTopicsSyntaxTest.php should now be skipping obsolete modules.
3. core/modules/search/tests/src/Functional/SearchPageCacheTagsTest.php doesn't need to install entity_reference.module.

While I was grepping, I found another spot that's manually installing entity_reference that doesn't need to: core/modules/system/tests/src/Kernel/Entity/EntityReferenceSelectionReferenceableTest.php.

Finally, there was some logic in core/modules/system/tests/src/Functional/Module/InstallUninstallTest.php that was testing for the 'Core (Experimental)' package, when we should be using the status key, now.

All fixed. This should now be green...

I think entity_reference and simpletest probably represent the only two obsolete states we're going to have - obsolete with no replacement, and obsolete with a replacement in contrib. We could probably do a generic message pointing to a change record, but this would mean a change_record key in .info.yml which is a bit messy maybe. Also tbh I think we could drop the specific message for Simpletest - obsolete always has to mean the module can be uninstalled without damage to a site, and by the time a module is obsolete, it should also have been deprecated first, which should have a warning attached.

Superb work here — this is long overdue! @Gábor Hojtsy made me aware of this issue.

AFAICT this does not do any validation — yet. Everything in the patch is explicitly saying that every .info.yml file that contains a status key MUST have one of the values listed in ExtensionStatus, and if it's not defined, it defaults to NORMAL. So the only parsing change is:

+++ b/core/lib/Drupal/Core/Extension/InfoParserDynamic.php
@@ -107,6 +107,9 @@ public function parse($filename) {
+      if (!isset($parsed_info['status'])) {
+        $parsed_info['status'] = ExtensionStatus::NORMAL;
+      }

IMHO this should throw an InfoParserException when an invalid status is specified. Just like we already do if a required key is missing.

Re: #72: Thanks for the review and suggestion. Agreed some validation is worth adding. Here's a first stab, with lots of unit test coverage. Thoughts?

Also, why both "Needs change record" and "Needs change record updates"? Agreed we need a CR. But what needs updating, other than the CR that needs to be written? ;) Are you proposing that some other existing CRs need edits? If so, please clarify. If not, can we remove the ...updates tag?

Thanks again,
-Derek

Thanks, bot. ;) Whoops.

Patch looks good!

1. +++ b/core/modules/jsonapi/tests/src/Functional/TestCoverageTest.php
   @@ -38,7 +39,7 @@ protected function setUp(): void {
   -        && $module->info['package'] !== 'Core (Experimental)';
   +        && $module->info['status'] !== ExtensionStatus::EXPERIMENTAL;
   

   Should we also add the line: && $module->info['status'] !== ExtensionStatus::OBSOLETE?

2. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceRestTestCoverageTest.php
   @@ -42,7 +43,7 @@ protected function setUp(): void {
   -        $module->info['package'] !== 'Core (Experimental)';
   +        $module->info['status'] !== ExtensionStatus::EXPERIMENTAL;
   

   Same here.

3. +++ b/core/tests/Drupal/KernelTests/Core/Theme/Stable9TemplateOverrideTest.php
   @@ -63,7 +64,7 @@ protected function installAllModules() {
   +      if ($module->origin !== 'core' || !empty($module->info['hidden']) || $module->status == TRUE || $module->info['package'] == 'Testing' || $module->info['status'] === ExtensionStatus::EXPERIMENTAL) {
   
   +++ b/core/tests/Drupal/KernelTests/Core/Theme/StableLibraryOverrideTest.php
   @@ -64,7 +65,7 @@ protected function setUp(): void {
   -      if ($module->origin !== 'core' || !empty($module->info['hidden']) || $module->status == TRUE || $module->info['package'] == 'Testing' || $module->info['package'] == 'Core (Experimental)') {
   +      if ($module->origin !== 'core' || !empty($module->info['hidden']) || $module->status == TRUE || $module->info['package'] == 'Testing' || $module->info['status'] === ExtensionStatus::EXPERIMENTAL) {
   
   +++ b/core/tests/Drupal/KernelTests/Core/Theme/StableTemplateOverrideTest.php
   @@ -61,7 +62,7 @@ protected function installAllModules() {
   -      if ($module->origin !== 'core' || !empty($module->info['hidden']) || $module->status == TRUE || $module->info['package'] == 'Testing' || $module->info['package'] == 'Core (Experimental)') {
   +      if ($module->origin !== 'core' || !empty($module->info['hidden']) || $module->status == TRUE || $module->info['package'] == 'Testing' || $module->info['status'] === ExtensionStatus::EXPERIMENTAL) {
   

   Should status equals obsolete be added to the list?

4. I am missing testing that the modules: "entity_reference" and "simpletest" no longer appear on the page /admin/modules.
5. Should we add a warning on the page when an obsolete module is installed?

We've added the deprecated status here, but nothing uses it yet, not even a test. Should we just drop this for now and add it in a followup, where we can figure out what to do with deprecated modules?

@longwave: Fine by me, only the IS then needs an update. It is now saying the the remaining tasks:

3. Promote the non-released statuses in admin/modules page, optionally even visually
4. Promote the non-released statuses in admin/appearance page, optionally even visually
5. Use statuses in admin/reports/status page, providing warnings when using non-"released" extensions

Thanks for the reviews!

Re: #75.1-3: That's what #2877066: Provide a standard way for tests to find core modules without loading experimental modules is about, and point 2 from remaining tasks: "Introduce a trait to enable modules for tests". As usual, I'm not sure the best way to scope all this so that it actually gets committed.

Re: 75.4: That's because the hiding is due to the hidden flag in the .info.yml file, which is unchanged by this patch. Hidden should already have its own tests, and if not, that much is *definitely* out of scope here and should be a follow-up.

Re: 75.5: Yes, but that's again a scoping question. Both this issue and its parent #3135100: [policy and meta] Provide a proper mechanism for deprecating modules and themes talk about many of the same things. I'm not sure how much we should accomplish here vs. as new child issues of #3135100

Re: #76: Maybe. But I'm a bit reluctant to go this far with the status, but then not add the 'deprecated' status which is exactly what this issue was started for. But it's not up to me. ;) If core committers want to scope it that way, we can definitely do it.

I'm going to move this back to NR, since there's nothing really actionable to change here until the scope of this vs. the parent and related issues is more clear. Hope that's agreeable.

Thanks!
-Derek

So we have issues to deprecate modules (like aggregator) that can/will use the deprecated status, I think we should keep it in this patch.

Overall it seems worth adding the status report and extend/appearance page changes here if we can - if it gets tricky possibly split it out then? Status report should just be message text which isn't too bad, I could see extend/appearance getting more complex if we need to do any design changes though.

1. +++ b/core/lib/Drupal/Core/Extension/ExtensionStatus.php
   @@ -0,0 +1,55 @@
   +  /**
   +   * Extension is experimental. Warnings will be shown if installed.
   +   */
   +  const EXPERIMENTAL = 'experimental';
   ...
   +  /**
   +   * Extension is deprecated. Warnings will be shown if still installed.
   +   */
   +  const DEPRECATED = 'deprecated';
   

   If we were going to remove an experimental module would we deprecate it and leave it as experimental?

   Also if you install a deprecated module via the UI should you get a warning that you are installing a deprecated module?

2. +++ b/core/lib/Drupal/Core/Extension/ExtensionStatus.php
   @@ -0,0 +1,55 @@
   +  /**
   +   * Extension is normal. This is the default value of any extension.
   +   */
   +  const NORMAL = 'normal';
   

   Normal is an odd word... whilst reading the patch I kept on pondering what is normal? Is it worth having this constant and status - might not an empty status be sufficient?

3. +++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
   @@ -91,6 +92,9 @@ public function install(array $module_list, $enable_dependencies = TRUE) {
   +      if ($module_data[$module]->info['status'] === ExtensionStatus::OBSOLETE) {
   +        throw new ObsoleteExtensionException("Unable to install modules: module '$module' is obsolete.");
   +      }
   

   This one is super super tricky. I think we have to allow this during a configuration sync otherwise this could break deployments where an obsolete module is installed.

I strongly feel this patch should only add the API and then we decide how to handle each value in its own dedicated followup. That includes the changes to the experimental group -- right now, we're making a non-BC change by removing the behavior of having package: experimental. So we need to think more about how to do that and I'd sooner do it in a followup focused only on deprecating the experimental package in favor of the new key.

I agree that a null value should take the place of an explicit normal value for the key. normal would just be noise (on a lot of projects) saying "nothing to report".

fwiw I'm also fine with just adding the API here and opening follow-ups for everything else, only question for me is defining at what point we consider the issue enough to start using it for issues like deprecating aggregator (and focusing on those blocking issues first).

NW for #80; all three points seem blocking to me. Let's also revert the change to experimental package behavior and defer it to a followup, and just add the API itself in this issue. We can create a followup issue for each status (experimental, obsolete, deprecated) and prioritize the followup for what, exactly, we want the consequences of status: deprecated to be. Thanks!

1. +++ b/core/lib/Drupal/Core/Extension/ExtensionStatus.php
   @@ -0,0 +1,55 @@
   +final class ExtensionStatus {
   ...
   +  public static function isValid($status) : bool {
   +    $valid_statuses = [
   +      self::EXPERIMENTAL,
   +      self::NORMAL,
   +      self::DEPRECATED,
   +      self::OBSOLETE,
   +    ];
   +    return in_array($status, $valid_statuses, TRUE);
   

   Making it final makes it hard to convert it later to "enums" which is mostly accepted https://wiki.php.net/rfc/enumerations#voting

2. +++ b/core/modules/simpletest/simpletest.info.yml
   @@ -1,6 +1,7 @@
   -description: 'Deprecated. SimpleTest has been removed from core.'
   +description: 'Obsolete. SimpleTest has been removed from core.'
   

   This change could use separate issue

3. +++ b/core/modules/system/src/Form/ModulesListForm.php
   @@ -404,7 +405,7 @@ protected function buildModuleList(FormStateInterface $form_state) {
   -        if ($data[$name]->info['package'] == 'Core (Experimental)') {
   +        if ($data[$name]->info['status'] === ExtensionStatus::EXPERIMENTAL) {
   
   @@ -418,7 +419,7 @@ protected function buildModuleList(FormStateInterface $form_state) {
   -          if ($data[$dependency]->info['package'] == 'Core (Experimental)') {
   +          if ($data[$dependency]->info['status'] === ExtensionStatus::EXPERIMENTAL) {
   
   +++ b/core/modules/system/system.install
   @@ -65,7 +66,7 @@ function system_requirements($phase) {
   -      if (isset($info['package']) && $info['package'] === 'Core (Experimental)') {
   +      if (isset($info['status']) && $info['status'] === ExtensionStatus::EXPERIMENTAL) {
   

   this change is all round and some code could need BC-shim for it

4. +++ b/core/modules/system/tests/src/Kernel/Entity/EntityReferenceSelectionReferenceableTest.php
   @@ -45,7 +45,6 @@ class EntityReferenceSelectionReferenceableTest extends KernelTestBase {
        'field',
   -    'entity_reference',
        'node',
   

   separate issue too

Just a suggestion before we add this one more case of "status", which is already used a lot in Drupal, and which this discussion proves to have been more than a bit ambiguous: could we call this "lifecycle" instead of "status", to better communicate the intent ?

1. +++ b/core/lib/Drupal/Core/Extension/ExtensionStatus.php
   @@ -0,0 +1,55 @@
   + * 2. Stabilizes and goes "normal".
   

   💬 Like @alexpott I feel 'normal' is a loaded word, how about we just use 'supported' as the default. Sorry bikeshed.

2. +++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
   @@ -91,6 +92,9 @@ public function install(array $module_list, $enable_dependencies = TRUE) {
   +      if ($module_data[$module]->info['status'] === ExtensionStatus::OBSOLETE) {
   +        throw new ObsoleteExtensionException("Unable to install modules: module '$module' is obsolete.");
   +      }
   

   🐛 yeah, plus one for wrapping this in a check for if we're syncing config

3. If we were going to remove an experimental module would we deprecate it and leave it as experimental?

   I think we have to respect the lifecycle laid out here, if it is deprecated, its deprecated, and no longer experimental

4. Plus one for getting this in without the UI changes so we can unlock progress on things like aggregator and possibly forum

Tagging for DrupalCon North America 2021 contribution.

Posted by @fgm in #85:

Just a suggestion before we add this one more case of "status", which is already used a lot in Drupal, and which this discussion proves to have been more than a bit ambiguous: could we call this "lifecycle" instead of "status", to better communicate the intent ?

+1 on not calling the property "status" because of its ambiguity. I think "lifecycle" is a good term. I also thought "support_status" might be a good term. Or would that cause confusion with the term "Development status" for projects on drupal.org?

'lifecycle' doesn't seem bad to me.

'support_status' - if this was only ever going to be used for core then it wouldn't be too bad, but nothing will stop contrib modules specifying it, and then I think it will get confused with the d.o setting.

+1 to lifecycle, I think that is a good description.

However, to throw in some more bikeshedding on the default value: to me "lifecycle: stable" feels like a better description than "normal".

lifecycle: stable

sound good

Tagging for issue summary update to summarize the 90 odd comments and remaining work

Had a crack at the issue summary update.

Is the remaining task list accurate in the issue summary? Where can new people best help currently?

I don't think we need to do (or should) everything in 'remaining tasks' here, several of those could be done in follow-ups.

The issue currently does the following:
1. Introduces the API.
2. Implements the API for 'obsolete' modules (preventing them from being installed).

Although if we're going to switch to lifecycle from status, we need to update the patch for this.

In follow-ups we can:

1. Generically handle 'experimental' on module and theme listings.
2. Generically handle 'deprecated' on module and theme listings.
3. Show warnings on admin/reports.

Tried to tidy it up a bit, but it probably needs more.

For now just a re-roll.

I'll work on it.

Patch to switch to lifecycle from status.

Updated IS, changed a few left-over "normal" to "stable"

Adding the word lifecycles to dictionary.
Ops! I attached the interdiff two times. Comment #104 has the patch that I was suppose to add here.

Used 3124762-102.patch as base for the MR.

Random test-failure #3214565: [random test failure] Random fail in BuildTestTest::testPortMany, retest already underway.

We finally got there, MR changed 'status' to 'lifecycle' and default value to "stable" and is green.
Updated IS

Addressed all-but-one of the remarks made by @larowlan (thanks for those!).
The one remaining is a question if this should either be solved here, or if creating a follow-up to do so is enough for now.

Back to NR for more eyes on this.

Added another follow-up to the remaining tasks and added some tasks around change records and UX reviews

Added all the follow-ups, all we need now is the usability review.

The issue summary has 'user interface changes' but I think these have all been deferred to follow-ups.
So I'm going to remove those pieces and the tag as well as the steps.

Added a release notes snippet.

The only remaining task is change notice updates, which should happen when this is committed. Added the change notice links to the remaining tasks.

On the basis of all of that, I think this is RTBC.

I'm going to add issue-credits while I'm here.

We still need to address #84.3 / #86.2

To fix #115 we can move

    /** @var \Drupal\Core\Config\ConfigInstaller $config_installer */
    $config_installer = \Drupal::service('config.installer');
    $sync_status = $config_installer->isSyncing();

up to the top of the method and then check $sync_status.

Added one more review

I think we should allow the config installer to install obsolete modules.

Is related to #84.4 - entity_reference module throws now and I bet needs separate issue

Let's see what I can do here.

Thanks @andypost for the review, resolved two of his remarks.

2 Unresolved threads remaining, back to NR for more eyes/brains/random-body-parts on those.

@Spokje @andypost this issue needs to move the code that works out whether the install is happening during a config install up to the top of the ModuleInstaller::install() function and then use this flag on the check. The ModuleInstaller should not trigger the obsolete exception. Doing that will break existing sites who use config install for CI / development / other purposes.

The code is....

    /** @var \Drupal\Core\Config\ConfigInstaller $config_installer */
    $config_installer = \Drupal::service('config.installer');
    $sync_status = $config_installer->isSyncing();

Then you have $sync_status to use to decide if to throw an exception.

Tagging for our recurring sprint sessions, sorry for the noise

@Spokje @andypost this issue needs to move the code that works out whether the install is happening during a config install up to the top of the ModuleInstaller::install() function and then use this flag on the check. The ModuleInstaller should not trigger the obsolete exception. Doing that will break existing sites who use config install for CI / development / other purposes.

I'm not sure about this (moving back to needs review for more discussion).

The current equivalent to an obsolete module in core is Entity Reference. We have a hook_requirements() error on install to prevent new installations of the module.

Let's say we hadn't done this for entity_reference yet, and were just about to make it obsolete now.

#3111645: Uninstall entity_reference module and prevent it being enabled again, remove deprecated code added a post update to uninstall the module, alongside the hook_requirements.

So even a site using config install, I would expect the following to happen:

1. Update to Drupal 9.3.0
2. Post update runs to uninstall entity_reference
3. Developer exports the updated config and commits the changes.

If step 3 is missed, then entity_reference would be reinstalled by config install/config sync. However, this is a proper error condition, because when the site updates to Drupal 10, entity_reference will disappear from the code base with no further warnings except the status report - then you end up with a missing module, warnings all over the place, and potentially harder to fix.

So, as long as we always add an update to uninstall a module when we make it obsolete (which we have to), then I think the exception is fine here.

@catch that's a good point. I've changed my mind - you are correct. I wished we had made progress on #2628144: Ensure that ConfigImport is taking place against the same code base as that would make it easier to determine that the config being imported has been updated at the correct moment but that should not hold this up.

So, for the people not in possession of Big Brains *raises hand*, do I recap correctly that this open thread in the MR can now be resolved, leaving this thread the only one open?

@Spokje right on both counts I think, replied to the second thread.

tips hat @catch

Marked both open threads as resolved.
Updated MR with latest and greatest from 9.3.x.

@andypost If you feel strongly about the changing of the description in simpletest.info.yml I'm more than happy to un-change it to get this issue on RTBC. Just lemme know :)

I'm ++ for change simpletest and ER to Obsolete, I bet we can get rid of isset() and improve code comment.

Moreover probably it needs empty post update hook to clear cache, so after deploy it make sure that extension list will be repopulated with default value (stable) for info files

The change record looks up-to-date to me, so removing tag.

I think this is ready again

Committed 37bd376 and pushed to 9.3.x. Thanks!

Follow up #3225947: Remove redundant entity_reference.install and simpletest_install()