Fix constructor deprecation in Drupal\Core\Theme\Registry::__construct()

@catch, I couldn't find the $theme_name parameter at the ThemeRegistry::__construct() (link). Am I looking on the wrong place?

So I tried to create a new deprecation message for the Theme\Registry::__construct() and change the order of the parameters.
I also changed the order at D10 (without adding the deprecation this time).

@andregp: Thank you for working on this issue. Both patches look good.

For the D9.5 patch:

+++ b/core/lib/Drupal/Core/Theme/Registry.php
@@ -178,19 +178,25 @@ class Registry implements DestructableInterface {
+    if ($runtime_cache instanceof string || $module_list instanceof CacheBackendInterface || $theme_name instanceof ModuleExtensionList) {
+      @trigger_error('Calling Registry::__construct() with the $module_list as the antepenultimate argument is deprecated in drupal:9.5.0 and is removed in drupal:10.0.0. See https://www.drupal.org/node/3285131', E_USER_DEPRECATED);

Can we change this to:

if (!($runtime_cache instanceof CacheBackendInterface) || !($module_list instanceof ModuleExtensionList)) {
  @trigger_error('Calling Registry::__construct() without the $runtime_cache as an instance of CacheBackendInterface or the $module_list as an instance of ModuleExtensionList is deprecated in drupal:9.5.0 and is removed in drupal:10.0.0. See https://www.drupal.org/node/3285131', E_USER_DEPRECATED);

For the D10 patch:

+++ b/core/lib/Drupal/Core/Theme/Registry.php
@@ -174,19 +174,18 @@ class Registry implements DestructableInterface {
+  public function __construct($root, CacheBackendInterface $cache, LockBackendInterface $lock, ModuleHandlerInterface $module_handler, ThemeHandlerInterface $theme_handler, ThemeInitializationInterface $theme_initialization, CacheBackendInterface $runtime_cache = NULL, ModuleExtensionList $module_list = NULL, $theme_name = NULL) {

The $runtime_cache and the $module_list are not optional, therefor they should not have a default value.

All right, thanks for the feedback @daffie. I'll fix it in a moment.

+++ b/core/lib/Drupal/Core/Theme/Registry.php
@@ -178,19 +178,25 @@ class Registry implements DestructableInterface {
+      @trigger_error('Calling Registry::__construct() with the $module_list as the antepenultimate argument is deprecated in drupal:9.5.0 and is removed in drupal:10.0.0. See https://www.drupal.org/node/3285131', E_USER_DEPRECATED);

Even as a native English speaker the word "antepenultimate" is not familiar to me, I suggest we use "last but one" or something similar instead.

@daffie, I changed the code accordingly and tested it by running the related tests (the ones that appear on the patch) without changing them to see if they would throw the deprecation messages correctly. All tests worked (showed the deprecation messages) as expected, except for core/tests/Drupal/KernelTests/Core/Theme/RegistryLegacyTest.php which, even without changing it, worked without triggering the deprecation. I don't know why.

Another problem with !($runtime_cache instanceof CacheBackendInterface) || !($module_list instanceof ModuleExtensionList) is that some tests actually provide a NULL value for $runtime_cache and/or $module_list which triggers the deprecation even when they are provided on the right order (which reorders the parameters and makes the test fail). See for example Drupal\KernelTests\Core\Theme\RegistryTest::testMultipleSubThemes or Drupal\KernelTests\Core\Theme/RegistryLegacyTest::testMultipleSubThemes().

The same happens for the D10 patch, removing the NULL option makes the tests fail as they don't provide all the parameters.

Should I modify the tests to always include $runtime_cache and $module_list, or do I just keep these parameters nullable?

@longwave sure. I'm sorry for that. I used Google translator to find this term...
I'll wait the response for #8 before fixing it, so I can fix both the wording and the tests on the same patch.

Should I modify the tests to always include $runtime_cache and $module_list, or do I just keep these parameters nullable?

The tests need to be updated, change it where you set $runtime_cache and $module_list to a real value. That is what we are fixing in this issue.

@daffie Okay, thanks for the answer.
I'll do it and send a patch here soon.

This patch addresses the comments #5 and #10. I didn't address #7 as the message text was changed completely.

Thanks for working on this.

1. +++ b/core/lib/Drupal/Core/Theme/Registry.php
   @@ -178,19 +178,25 @@ class Registry implements DestructableInterface {
   +      @trigger_error('Calling Registry::__construct() without the $runtime_cache as an instance of CacheBackendInterface or the $module_list as an instance of ModuleExtensionList is deprecated in drupal:9.5.0 and is removed in drupal:10.0.0. See https://www.drupal.org/node/3285131', E_USER_DEPRECATED);
   

   Should be "is required" instead of "is removed" I think?

   Also this deprecation needs a test, as there are now three ways of instantiating this object and it's quite confusing to follow.

2. +++ b/core/lib/Drupal/Core/Theme/Registry.php
   @@ -178,19 +178,25 @@ class Registry implements DestructableInterface {
   +      $temp = $runtime_cache;
   +      $runtime_cache = $module_list;
   +      $module_list = $theme_name;
   +      $theme_name = $temp;
   

   This can be simplified to

   [$runtime_cache, $module_list, $theme_name] = [$module_list, $theme_name, $runtime_cache];
   

Thanks for reviewing @longwave. I'll take a look into this.
Edit.: I didn't know about the #2 syntax, TIL! It's clever and helpful :D

Addressing comment #13.

@daffie and @longwave, I was wondering about 2 things:
1 - If we removed the type hints on the last 3 parameters, shouldn't the deprecation be able to handle any order different from the one expected? Something more or less like this:

if (($runtime_cache !== NULL && !($runtime_cache instanceof CacheBackendInterface))
 || ($module_list !== NULL && !($module_list instanceof ModuleExtensionList))
 || ($theme_name !== NULL && !($theme_name instanceof string))) {
  @trigger_error('Calling ...', E_USER_DEPRECATED);
  $list = [$runtime_cache, $module_list, $theme_name];
  foreach ($list as $param) {
    if ($param instanceof CacheBackendInterface) {
      $runtime_cache = $param;
    }
    if ($param instanceof ModuleExtensionList) {
      $module_list = $param;
    }
    if ($param instanceof string) {
      $theme_name = $param;
    }
  }
}

2 - If we are going to remove the "optionality" of the $runtime_cache and $module_list parameters, shouldn't we add a deprecation specific to that (which checks and say they can't be null)?

Edit: It could be probably 3 deprecations: wrong order, wrong class, and missing (null) parameter. (or I might be just overshooting the complexity)

The D9.5 patch is for me RTBC. We also need a patch for D10.

+++ b/core/lib/Drupal/Core/Theme/Registry.php
@@ -178,19 +178,22 @@ class Registry implements DestructableInterface {
+  public function __construct($root, CacheBackendInterface $cache, LockBackendInterface $lock, ModuleHandlerInterface $module_handler, ThemeHandlerInterface $theme_handler, ThemeInitializationInterface $theme_initialization, $runtime_cache = NULL, $module_list = NULL, $theme_name = NULL) {

For the D10 patch we need to add the parameter typehints for $runtime_cache and $module_list. Both parameters do not need a default value. In D10 they are required and no longer optional.

@daffie The patch I sent on #12 for D10 already removes the default null values and adds back the type hints https://www.drupal.org/files/issues/2022-06-14/3285131-12_10.0.patch

All code changes look good to me.
Both patches are RTBC for me.
All review points have been addressed.