Introduce new Theme extension object and properly deprecate REGIONS_VISIBLE and REGIONS_ALL

That looks great and much cleaner

Maybe instead of new Translatable($label) extension api could provide that

+++ b/core/modules/block/src/BlockListBuilder.php
@@ -404,6 +418,7 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
   protected function systemRegionList($theme, $show = REGIONS_ALL) {
+    @trigger_error('@todo', E_USER_DEPRECATED);

+++ b/core/modules/block_place/src/Plugin/DisplayVariant/PlaceBlockPageVariant.php
@@ -133,7 +143,8 @@ public function build() {
   protected function getVisibleRegionNames($theme) {
...
+    @trigger_error('@todo', E_USER_DEPRECATED);

+++ b/core/modules/system/system.module
@@ -1062,25 +1064,18 @@ function system_rebuild_module_data() {
 function system_region_list($theme, $show = REGIONS_ALL) {
...
+  @trigger_error('@todo', E_USER_DEPRECATED);

@@ -1127,10 +1122,12 @@ function system_system_info_alter(&$info, Extension $file, $type) {
 function system_default_region($theme) {
...
+  @trigger_error('@todo', E_USER_DEPRECATED);

also needs testing for deprecated BC + exception

Added regions_hidden to theme init
As all of this methods getters - then let's call them not "list"
A bit of clean-up

Clean-up renaming

Deletegated in theme init, hidden is not used in active theme so every usage should use theme handler

make translation inline with core

the other option is to have a ThemeRegion service. It would depend on the theme_handler and string_translation services.

This sounds preferable, and also avoiding a BC break by not expanding ThemeHandlerInterface.

Tim do you have any thoughts about how to swap it later for #2924058: Discuss using Layout Builder to control full site layout and replace Block UI

@alexpott what if we add extension getFeatures() which will return specific extension info.yml parsing and validation object - surely followup

On my side I'd like to split theme info which is already dumplicated in activetheme class object

Making it a new service could probably help with that (as Layout Builder could swap out the service), but that issue isn't a current focus of the Layout Initiative

1. +++ b/core/lib/Drupal/Core/Extension/ExtensionList.php
   @@ -311,7 +311,8 @@ protected function doList() {
   +      $extension = $this->decorateExtension($extension);
   +      $extensions[$extension_name] = $extension;
    
   

   Nit: Why split this into two lines?

2. +++ b/core/lib/Drupal/Core/Extension/ExtensionList.php
   @@ -540,25 +541,27 @@ public function getPath($extension_name) {
   +   * extensions info.yml information. This maybe be overridden to return an
   

   Should be "This may be overridden..."

3. +++ b/core/lib/Drupal/Core/Extension/Theme.php
   @@ -0,0 +1,167 @@
   +   * Whether the Theme is installed or not.
   

   Nit: "Theme" should be "theme".

4. +++ b/core/lib/Drupal/Core/Extension/Theme.php
   @@ -0,0 +1,167 @@
   +   * @todo Remove all code that relies on the $status property.
   +   *
   +   * @var int
   +   */
   +  public $status;
   

   To facilitate the removal of the property, maybe this would be a good time to add an isEnabled() method to replace it?

5. +++ b/core/lib/Drupal/Core/Extension/Theme.php
   @@ -0,0 +1,167 @@
   +  /**
   +   * The info array based on the theme's .info.yml file.
   +   *
   +   * @var array
   +   */
   +  public $info;
   

   Should this default to an empty array?

6. +++ b/core/lib/Drupal/Core/Extension/Theme.php
   @@ -0,0 +1,167 @@
   +  /**
   +   * The relative path to the theme engine extension file.
   +   *
   +   * @var string
   +   */
   +  public $owner;
   

   "Owner" seems like a strange name for this property. Maybe we should take this opportunity to rename it and add a magic __get() wrapper around "owner", if external code relies on it.

7. +++ b/core/lib/Drupal/Core/Extension/Theme.php
   @@ -0,0 +1,167 @@
   +  /**
   +   * The internal name of the theme engine extension
   +   *
   +   * @var string
   +   */
   +  public $prefix;
   

   Same here.

8. +++ b/core/lib/Drupal/Core/Extension/Theme.php
   @@ -0,0 +1,167 @@
   +   * @param int $status
   +   *   The theme's installation status.
   

   Should this be a boolean?

9. +++ b/core/lib/Drupal/Core/Extension/Theme.php
   @@ -0,0 +1,167 @@
   +  public function __construct($root, Extension $extension, array $info, $status) {
   

   Passing an Extension object to this constructor seems strange. It almost feels like it should be a static createFromExtension() method...

10. +++ b/core/lib/Drupal/Core/Extension/Theme.php
    @@ -0,0 +1,167 @@
    +    if ($this->info['base theme'] === FALSE) {
    +      unset($this->info['base theme']);
    +    }
    +    else {
    +      // Add the base theme as a proper dependency.
    +      $this->info['dependencies'][] = $this->info['base theme'];
    +    }
    

    What if 'base theme' is an empty string, or null? Maybe this should be an empty() check instead.

11. +++ b/core/lib/Drupal/Core/Extension/Theme.php
    @@ -0,0 +1,167 @@
    +  public function getDefaultRegion() {
    +    return (string) key($this->listVisibleRegions());
    +  }
    

    Why does the return value need to be casted to a string?

12. +++ b/core/lib/Drupal/Core/Extension/ThemeExtensionList.php
    @@ -104,19 +67,39 @@ public function __construct($root, $type, CacheBackendInterface $cache, InfoPars
    +    $engines = $this->engineList->getList();
    

    Where is this used?

13. +++ b/core/lib/Drupal/Core/Extension/ThemeExtensionList.php
    @@ -245,24 +226,10 @@ protected function doGetBaseThemes(array $themes, $theme, array $used_themes = [
    +    $status = (int) isset($this->installedThemes[$extension->getName()]);
    

    Isn't there a method to retrieve the list of installed themes?

A little overlap from #42:

1. +++ b/core/lib/Drupal/Core/Extension/Theme.php
   @@ -0,0 +1,167 @@
   +  /**
   +   * Whether the Theme is installed or not.
   +   *
   +   * @todo Remove all code that relies on the $status property.
   +   *
   +   * @var int
   +   */
   +  public $status;
   +
   +  /**
   +   * The info array based on the theme's .info.yml file.
   +   *
   +   * @var array
   +   */
   +  public $info;
   

   These should really be a part of Extension, they're not theme specific: http://cgit.drupalcode.org/drupal/tree/core/includes/common.inc?id=8.6.5...

2. +++ b/core/lib/Drupal/Core/Extension/Theme.php
   @@ -0,0 +1,167 @@
   +  public function __construct($root, Extension $extension, array $info, $status) {
   +    if ($extension->getType() !== 'theme') {
   +      throw new \InvalidArgumentException('Constructing a Theme object using an extension that is not a theme makes no sense');
   +    }
   +    parent::__construct($root, 'theme', $extension->getPathname(), $extension->getExtensionFilename());
   +    $this->info = $info + static::getInfoDefaults();
   

   This is really weird and feels like a huge anti-pattern.

   The Theme object extends from Extension, it shouldn't need an Extension object to create a Theme object.

3. +++ b/core/lib/Drupal/Core/Extension/ThemeExtensionList.php
   @@ -245,24 +226,10 @@ protected function doGetBaseThemes(array $themes, $theme, array $used_themes = [
   +  protected function decorateExtension(Extension $extension) {
   +    $info = $this->infoParser->parse($extension->getPathname());
   +    $status = (int) isset($this->installedThemes[$extension->getName()]);
   +    return new Theme($this->root, $extension, $info, $status);
   

   Decorating something shouldn't return a whole new object.

I'm also wondering if maybe this shouldn't be postponed on #2941757: Extension System, Part IV: Properly register all installed extensions/namespaces during container generation and #3023131: [PP-1] Extension System, Part IV: ExtensionHandler and ExtensionHandlerInterface. These issues will clear up a lot of how extensions are constructed/handled by ExtensionList and eliminate some of the weird hackery that's going on currently and the extended proposed changes that only add to the nightmare.

#43.1 There is nothing to stop us moving them up a class in a follow-up and I agree but issue scope.

Not entirely sure how this is issue scope when it's introducing official properties that belong to the parent class in the first place.

#43.2 I think you like the words "anti pattern" too much. But anyway the principle of taking an object and decorating it is not one.

I don't use it that much lol I wasn't aware that you were attempting to turn the Extension objects into decorated classes, which is why it looked quite odd for a sub-class to require a parent class. I tend to think of Extensions as stand-alone objects; able to be created using scalar values.

#43.3 Yes it should.

Please forgive my ignorance, I'm still not all that familiar with decorators as they aren't as prevalent on the FE (yet). Regardless, if this were to be a decorator, I think it'd need that name appended to the class to clarify what it's supposed to be doing. Given that it wasn't, I think that's what confused me the most.

For me this shouldn't be postponed on #2941757: Extension System, Part IV: Properly register all installed extensions/namespaces during container generation that is make extensive changes...

True, I was thinking to postpone more on #3023131: [PP-1] Extension System, Part IV: ExtensionHandler and ExtensionHandlerInterface as that will likely change a lot of how Extensions are listed/handled as a whole.

edit: separating this out because I think it got overlooked.

Perhaps instead of postponing we should mark Theme as @internal temporarily?

I suspect this object will undergo quite a lot of changes over several issues once it's introduced.

My only real concern is introducing something that isn't yet fully flushed out for the sake of expediency.

To be honest decoration is a really weird word. We do composition here and the extension has a clear limited scope. Inheritance isn't the only way how you can architect your data structures :)

I'm not sure what the name should be. Personally, I don't believe replacing createExtensionInfo is necessarily the right move here.

In fact, $info['mtime'] = $extension->getMTime(); is missing from the ThemeExtensionList implementation.

Maybe it should be its own new method that invokes createExtensionInfo?

I know this is out of scope, but food for thought... ExtensionInfo from #2186491: [meta] D8 Extension System: Discovery/Listing/Info was pretty much sidelined and merged with ExtensionList (which is already doing way too much IMO). It may be beneficial to rethink introducing this in another issue?

---

I think we should temporarily mark Theme as @internal.

I suspect this object will undergo quite a lot of changes over several issues once it's introduced.

My only real concern is introducing something that isn't yet fully flushed out for the sake of expediency.

I think we should temporarily mark Theme as @internal.

+1 for this. I can't imagine too many people will extend or muck around with it, but we should at least make it as "sealed" and private as we realistically can, at least for now.

About naming field api using notion of "massage" to extend fields to widgets

Not it's not - see Theme::__construct()

That isn't ThemeExtensionList. I was referring to the code snippets in #50 and how it's a little confusing that it splits the responsibility of populating the info array between ThemeExtensionList and Theme.

Wrt to marking it @internal - the problem with that is that is not really what I think you mean. People will interact with these objects.

People already interact with ExtensionList and it's marked @internal because we haven't yet finalized the service and for good reason. I don't see why we can't do that here as well. I don't see a reason why backing us into a corner is necessary just yet.

+++ b/core/lib/Drupal/Core/Extension/ThemeHandler.php
@@ -112,7 +112,7 @@ public function listInfo() {
-  public function addTheme(Extension $theme) {
+  public function addTheme(Theme $theme) {

+++ b/core/lib/Drupal/Core/Extension/ThemeHandlerInterface.php
@@ -95,10 +95,10 @@ public function listInfo();
-  public function addTheme(Extension $theme);
+  public function addTheme(Theme $theme);

Ins't this technically a BC break (changing existing interfaces)?

@alexpott, @andypost

Do you think that the actual theme_get_setting() could be moved as a method in the new Theme object, as it has been suggested in #3035288-7: Deprecate theme_get_setting()?

@claudiu.cristea I think it is because of encapsulation of this logic related exactly to theme - and Mark already explained it in #3035288-11: Deprecate theme_get_setting()
So makes sense to figure #2024043: Add Module, Theme, Profile, and Extension value objects

I contrib to this cause rerolling.

Fix CS issues

Here is a re-roll of patch #63.

Hm, so in #3111942: Remove all remaining @deprecated code from system module I attempted to remove the region constants. They were deprecated with a suggestion to use \Drupal\block\BlockRepositoryInterface:: instead. So if we are adding this new API in 8.9 that changes what the resolution for the deprecation is. How is that compatible with defining deprecations up until Drupal 8.8? Tagging for release manager review.

Yes, this missed the deadline and therefore the solution is likely to undeprecate that constant or change it to a D10 removal.

Moving to Drupal 9.1 then :)

Re-rolled for 9.1.x

Removed needs re-roll tag.

The patch fails the testbot.

for 9.2.0

Re-roll against 9.2.x. Please review.

+ * @expectedDeprecation system_region_list() is deprecated in drupal:8.8.0 and is removed from drupal:9.0.0. Use

Wrong reroll

The testbot is not happy.

Let's start by re-rolling the patch from #80...

The patch is failing the testbot. See: https://dispatcher.drupalci.org/job/drupal_patches/93500/.

Rerolled #82
No interdiff.

re-roll went wrong

Since #63 patch lost some important parts, another rerolling based on #63

Fix CS

Fixed missing parts

Addressed #92

Moved optional construct argument into the end of the list

Tests passed. Ready for the review

1. +++ b/core/lib/Drupal/Core/Extension/Theme.php
   @@ -0,0 +1,152 @@
   +   * Gets a list of all regions for the theme.
   ...
   +  public function listAllRegions(): array {
   

   s/Gets/Returns

2. +++ b/core/lib/Drupal/Core/Extension/Theme.php
   @@ -0,0 +1,152 @@
   +   * Gets a list of visible regions for the theme.
   ...
   +  public function listVisibleRegions(): array {
   

   s/Gets/Returns

3. +++ b/core/lib/Drupal/Core/Extension/Theme.php
   @@ -0,0 +1,152 @@
   +   * Gets the name of the default region for the theme.
   

   s/Gets/Returns

4. +++ b/core/modules/block/block.module
   @@ -123,8 +123,10 @@ function block_theme_initialize($theme) {
   +    /** @var \Drupal\Core\Extension\Theme $theme_extension */
   +    $theme_extension = \Drupal::service('theme_handler')->getTheme($theme);
        // Apply only to new theme's visible regions.
   -    $regions = system_region_list($theme, REGIONS_VISIBLE);
   +    $regions = $theme_extension->listVisibleRegions();
   
   @@ -166,9 +168,10 @@ function block_modules_installed($modules) {
   -  foreach (\Drupal::service('theme_handler')->listInfo() as $theme => $data) {
   -    if ($data->status) {
   ...
   +  foreach (\Drupal::service('theme_handler')->listInfo() as $theme => $theme_extension) {
   +    if ($theme_extension->status) {
   +      $regions = $theme_extension->listAllRegions();
   
   +++ b/core/modules/block/src/BlockForm.php
   @@ -186,7 +186,7 @@ public function form(array $form, FormStateInterface $form_state) {
   -      '#options' => system_region_list($theme, REGIONS_VISIBLE),
   +      '#options' => $this->themeHandler->getTheme($theme)->listVisibleRegions(),
   
   @@ -198,7 +198,7 @@ public function form(array $form, FormStateInterface $form_state) {
   -    $form['region']['#options'] = system_region_list($form_state->getValue('theme'), REGIONS_VISIBLE);
   +    $form['region']['#options'] = $this->themeHandler->getTheme($form_state->getValue('theme'))->listVisibleRegions();
   
   +++ b/core/modules/block/src/BlockListBuilder.php
   @@ -197,7 +209,7 @@ protected function buildBlocksForm() {
   -    $regions = $this->systemRegionList($this->getThemeName(), REGIONS_VISIBLE);
   +    $regions = $this->themeHandler->getTheme($this->getThemeName())->listVisibleRegions();
   
   +++ b/core/modules/block/src/Controller/BlockController.php
   @@ -110,9 +110,15 @@ public function demo($theme) {
   +    return $this->themeHandler->getTheme($theme)->listVisibleRegions();
   
   +++ b/core/modules/block/src/Entity/Block.php
   @@ -346,10 +347,20 @@ public function preSave(EntityStorageInterface $storage) {
   +      $theme_extension = \Drupal::service('theme_handler')->getTheme($this->theme);
   

   wondering why theme handler used to access extension instead of \Drupal\Core\Extension\ExtensionList::get(), it may not need decoration and could use built-in caching. It reminds me abou #2941155: ModuleHandler should not maintain list of installed modules now that ModuleExtensionList exists

5. +++ b/core/modules/block/src/Controller/BlockController.php
   @@ -110,9 +110,15 @@ public function demo($theme) {
   +   * @deprecated in drupal:8.8.0 and is removed from drupal:9.0.0. Use
   +   *   $this->themeHandler->getTheme()->listVisibleRegions() instead.
   ...
   +    @trigger_error(__CLASS__ . '::getVisibleRegionNames() is deprecated in drupal:8.8.0 and is removed from drupal:9.0.0. Use $this->themeHandler->getTheme()->listVisibleRegions() instead. See https://www.drupal.org/node/3015925', E_USER_DEPRECATED);
   

   should use 9.4.0, not clear why 8.8.0 used

+++ b/core/lib/Drupal/Core/Extension/Theme.php
@@ -0,0 +1,152 @@
+   * Whether the theme is installed or not.
...
+   * @var int
...
+  public $status;
...
+   * The info array based on the theme's .info.yml file.
...
+   * @var array
...
+  public $info = [
+    'engine' => 'twig',
+    'regions' => [
+      'sidebar_first' => 'Left sidebar',
+      'sidebar_second' => 'Right sidebar',
+      'content' => 'Content',
+      'header' => 'Header',
+      'primary_menu' => 'Primary menu',
+      'secondary_menu' => 'Secondary menu',
+      'footer' => 'Footer',
+      'highlighted' => 'Highlighted',
+      'help' => 'Help',
+      'page_top' => 'Page top',
+      'page_bottom' => 'Page bottom',
+      'breadcrumb' => 'Breadcrumb',
...
+   * The relative path to the theme engine extension file.
...
+   * @var string
...
+  public $owner;
...
+   * The internal name of the theme engine extension.
...
+   * @var string
...
+  public $prefix;

not sure this properties should remain public

Addressed #96 .1 .2 .5

re-roll for 9.5

the failed test does not work with stark somehow but classy is missing

+++ b/core/modules/system/system.module
@@ -877,25 +888,35 @@ function system_check_directory($form_element, FormStateInterface $form_state) {
  *
  * @return
  *   An array of regions in the form $region['name'] = 'description'.
+ *
+ * @deprecated in drupal:9.5.0 and is removed from drupal:10.0.0. Use
+ *   \Drupal::service('theme_handler')->getTheme()->listAllRegions() or
+ *   \Drupal::service('theme_handler')->getTheme()->listVisibleRegions()
+ *   instead.
+ *
+ * @see https://www.drupal.org/node/3015925
  */
 function system_region_list($theme, $show = REGIONS_ALL) {
-  if (!$theme instanceof Extension) {
-    $themes = \Drupal::service('theme_handler')->listInfo();

9.5 is closed to new code deprecations (modules and themes are still fair game) to give contrib a stable API to port against.

So I think this needs to be targeted at 10.1.x now.

Also I think this is probably more in 'major task' territory than 'major bug', seems like the 'bug' aspect of this is just a docs issue.

rerolled for drupal:10.1.x

Rerolling so I can work on https://www.drupal.org/project/drupal/issues/3035288

systemRegionList($theme should be deprecated as well

NW as of #109

Rerolled but for #109 can you provide more detail? I see that systemRegionList is deprecated.

+++ b/core/lib/Drupal/Core/Extension/ExtensionList.php
@@ -542,25 +542,27 @@ public function getPath($extension_name) {
     // Add the info file modification time, so it becomes available for
     // contributed extensions to use for ordering extension lists.
     $info['mtime'] = $extension->getMTime();

+++ b/core/lib/Drupal/Core/Extension/Theme.php
@@ -0,0 +1,152 @@
+    $this->info['mtime'] = $this->getMTime();

somehow phpstan giving false positive here as the method executed via magic \Drupal\Core\Extension\Extension::__call()

  /**
   * Re-routes method calls to SplFileInfo.
   *
   * Offers all SplFileInfo methods to consumers; e.g., $extension->getMTime().
   */
  public function __call($method, array $args) {
    if (!isset($this->splFileInfo)) {
      $this->splFileInfo = new \SplFileInfo($this->root . '/' . $this->pathname);
    }
    return call_user_func_array([$this->splFileInfo, $method], $args);
  }

So this a bug with something else?

Let's see if this will allow to pass, thanks to @mglaman

Sorry for assigning, the ref is https://github.com/guzzle/guzzle/blob/6.5/src/Client.php#L13

Fix CS

Fixed failing test.

I think it's ready for RM review

+++ b/core/lib/Drupal/Core/Extension/Extension.php
@@ -8,6 +8,8 @@
+ * @method int|false getMTime()

There's issue for that #2959989: Deprecate Extension::__call() magic

Added deprecation tests for protected methods from the Block module

Fixed the typo in the test method name

Sorry folks, the CR has an @todo. Can someone attend to that?

And I think the deprecation notices for the constants needs to be changed.

+++ b/core/modules/block/src/BlockRepositoryInterface.php
@@ -7,14 +7,25 @@ interface BlockRepositoryInterface {
+   * @deprecated in drupal:10.1.0 and is removed from drupal:11.0.0.
+   *   It will not exist in Drupal 11, use
+   *   \Drupal\Core\Extension\Theme::listVisibleRegions()
+   *   instead.
+   *
+   * @see https://www.drupal.org/node/3015925
+   * @see \Drupal\Core\Extension\Theme::listVisibleRegions()
...
+   * @deprecated in drupal:10.1.0 and is removed from drupal:11.0.0.
+   *   It will not exist in Drupal 11, use
+   *   \Drupal\Core\Extension\Theme::listAllRegions() instead.
+   *
+   * @see https://www.drupal.org/node/3015925
+   * @see \Drupal\Core\Extension\Theme::listAllRegions()

While I do not see a specific case for deprecating constants, I also see no reason that this can't be same as for deprecating method parameters. That would mean the second @see in each case is removed as is "it will not exist in Drupal 1".

Took the description of this ticket for the CR.

Updated the comments too based on #122

+++ b/core/lib/Drupal/Core/Extension/Extension.php
@@ -8,6 +8,8 @@
+ * @method int|false getMTime()

could be removed as already commited #2959989: Deprecate Extension::__call() magic

+++ b/core/lib/Drupal/Core/Extension/Theme.php
@@ -0,0 +1,152 @@
+    $this->info['mtime'] = $this->getMTime();

needs fix

Updated Patch and address #124

Sorry for added wrong in #125
when applied patch #123 on 10.1.x it throwing some error added screen shot as well

Worked 3015812-123.patch addressing comments on #124
Main difference (after #2959989: Deprecate Extension::__call() magic):
$this->info['mtime'] = $this->getMTime();
Becomes
$this->info['mtime'] = $this->getFileInfo()->getMTime();

Checked last patch and looks like it's ready

Sorry folks. The patch is fail commit code checks.

#127 was not getting applied, I have made changes and hopefully, it should work. Couldn't generate the interdiff here because of "Whitespace damage detected in input"

This is tagged for a release manager review. In #67 a question was asked regarding the compatibility of the deprecation being added with the one added in the previous minor version. At that time that this was adding a deprecation to 8.9. Those concerns are no longer relevant because there isn't a supported version of Drupal with those deprecations. I discussed this issue with xjm and we agree.

I am removing the 'Needs release manager review' tag.

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.

Introduced MR which contains #130 patch.

btw when core will require PHP 8.5 we can deprecate constants https://wiki.php.net/rfc/attributes-on-constants

For the RM review mentioned in #132.

Updated and rebased, still running tests, but I think in a state where it can be reviewed.

I think the const replacing enum is missing

Enum is not missing. Issue summary has an overview of all the changes here. Still expecting some test fails, but the worst one should hopefully be fixed now.

Got some comments on the MR, I need to think about this approach a bit,

It is nice to remove the need entirely of these constants.

In not sure about the decorating pattern, but it seems to make sense.

I think the main remaining thing for me is the decorate naming/concept. This has been discussed quite a bit already, in #46/#47 for example. I don't think this is a decorator. Yes, a decorator does return a new object, but it's IMHO one that adheres to the original interface. That's not the case there. Theme is a child class that adds its own methods and concepts that do not exist on the parent. Decorator specifically allows to decorate the same object multiple times.

@dawehner in #47 also talks that we use composition and not inheritance, but that's not true, neither then nor now. Theme extends Extension, so it is inheritance. We're working with value objects and not interfaces, as far as I see it's impossible to use composition here if we want Theme objects to be used as more generic Extension objects too.

The only idea I have right now is to use a less opinionated, more generic term for this method, possibly just alterExtension(), which would kind of match the system_info_alter hook. Possibly wrap, *if* it were actually composition. Is there a verb that we could use that translates to "Use specific subclass"?

I'm also wondering if we should split the info parsing from the altering/subclassing. I'm not really convinced of the move of the defaults and mtime from ThemeExtensionList to the Theme constructor. I think we could keep it where it is and then we have to change fewer things. This started before ThemeExtensionList existed, so the context here changed quite a bit.

I think my proposal would be something like this:

foreach ($extensions as $extension) {
      $extension->info = $this->createExtensionInfo($extension);
      $extension = $this->alterExtension($extension);
}

I thought about using $info and passing that around as a second argument, but that's no longer an alter then and it's doing multiple things again.

Speaking of mtime, I was confused about that one too, because I don't see any usage of this in core outside of tests. This was added 14 years ago in #1355526: Add a way to determine the date a module was added so the modules page can use it for sort, even then it was just verified in tests. I'd assume that mtime is rarely useful with composer and modern deployment processes. And package manager and stuff is going to deal with "enable a recently enabled module" in its own way.

I implemented my suggested change now, essentially restoring createExtensionInfo() and introducing subClassExtension() per suggestion from @alexpott This significantly simplifies the required changes in the Extension component.

There is more that could be done in ThemeExtensionList. This moves the status and only the status from doList() to subClassExtension() as that was in decorate before as well. We could:

a) further reduce the required changes here by restoring how status is set now and deal with that later.
b) keep it as is.
c) Move more logic from doList and set them through the constructor/methods. There are still several dynamic properties being set there, like module_dependencies, base_themes and sub_themes. But we can't move everything as parts depend on having all themes and then adding that information.

I think c) is a rabbit hole that will blow this up considerably. I don't care much between a) or b). What I would suggest is that we mark Theme as either @final or real final, to make it clear that we do not support subclassing this. We want to change the constructor, add methods and properties later without having to worry about subclasses.

There are possibly even more options. We could do the subclass in doList() and not introduce subClassExtension() at all, then the parent would not need any changes, but I think this is fine as an extension point for other extension types later on.

Yeah a or b make sense.

To be honest this feels manageable at this point so leaving it as is is probably fine.

If we want to be extra careful rolling that bit back it's the way to go.

@final makes sense too, I'm strongly against real final.

@alexpott, see updates, is that what you had in mind? I also added a @final, I agree with you, but I'm doing my best to avoid any holy wars around final, either is fine for me.

I think this is ready for the next steps, I took another in depth look at the code, and it's come together very nicely!

I also edited the CR pretty heavily, it was targeted towards 8.7!!!

NW for merge conflict.

Rebased, it was a comment on a deprecated method that was removed in the deprecation removal bunch of issues.

The phpstan baseline needs a rebase. I also added comments to the MR.

Also, I know the "decorate" or "subclassExtension" terminology has already been much discussed, so I don't want to block this, but I have an alternate idea that could be done in a follow up if people think it's worthwhile. Instead of copying an Extension object to a subclass object, we could do this:

Add an optional "$extensionClass" constructor parameter and class property to ExtensionDiscovery, so that it looks like this:

  public function __construct(string $root, $use_info_parser = TRUE, ?array $profile_directories = NULL, ?string $site_path = NULL, protected string $extensionClass = Extension::class) {
    $this->root = $root;
    $this->profileDirectories = $profile_directories;
    $this->sitePath = $site_path;
    $this->infoParser = $use_info_parser ? new InfoParser($root) : NULL;
  }

And in ExtensionDiscovery::scanDirectory() change
new Extension($this->root, $type, $pathname, $filename)
to new $this->extensionClass($this->root, $type, $pathname, $filename).
The constructor of Theme would need to be changed to match Extension, so there'd be some brittleness there. The alternative would be to put it in a createExtension() method, but then every ExtensionList would need a corresponding discovery class with createExtension overridden.

In ThemeExtensionList::getExtensionDiscovery():