Allow marking themes as experimental

Hi @webchick,

Here is a quick patch I've made to implement the proposed solution. Instead of grouping experimental themes in a new group, I propose to simply add the note "experimental theme" in front of the theme name.
An experimental theme must have the property experimental set to true in the .info.yml file :

experimental: true

Here are some screenshots of the result :

Theme display

Confirmation form

Theme display in admin themes combobox

Warning display for experimental themes in status report page

Here is a new patch that tries to fix failed tests.

Great work here, @kamalzairig! I have a lot of remarks, but most of it is small stuff, so it'll be easy to address 😊

1. +++ b/core/modules/system/src/Controller/SystemController.php
   @@ -197,6 +197,7 @@ public function themesPage() {
   +      $theme->is_experimental = isset($theme->info['experimental']) && $theme->info['experimental'];
   

   🤔 This code is different from that for modules because SystemController::themesPage() was never truly modernized like the modules page was, whose code now lives at \Drupal\system\Form\ModulesListForm().

2. +++ b/core/modules/system/src/Controller/SystemController.php
   @@ -287,7 +288,7 @@ public function themesPage() {
   -      // Add notes to default and administration theme.
   +      // Add notes to default, administration and experimental theme.
   

   🤓 Übernit: There can only be one default theme, and only one administration theme. There can be many experimental themes. The language of the comment kinda doesn't work.

3. +++ b/core/modules/system/src/Controller/ThemeController.php
   @@ -117,6 +117,12 @@ public function install(Request $request) {
   +      $allThemes = $this->themeHandler->rebuildThemeData();
   
   @@ -168,6 +174,11 @@ public function setDefaultTheme(Request $request) {
   +      $allThemes = $this->themeHandler->rebuildThemeData();
   

   🤓🐫 Nit: s/$allThemes/$all_themes/

4. +++ b/core/modules/system/src/Controller/ThemeController.php
   @@ -117,6 +117,12 @@ public function install(Request $request) {
   +      if(isset($allThemes[$theme]) && isset($allThemes[$theme]->info['experimental']) && $allThemes[$theme]->info['experimental']) {
   
   @@ -168,6 +174,11 @@ public function setDefaultTheme(Request $request) {
   +      if (isset($allThemes[$theme]) && isset($allThemes[$theme]->info['experimental']) && $allThemes[$theme]->info['experimental'] && $allThemes[$theme]->status == 0) {
   

   🤓 Missing space: if( → if (

5. +++ b/core/modules/system/src/Controller/ThemeController.php
   @@ -117,6 +117,12 @@ public function install(Request $request) {
   +        return $this->formBuilder()->getForm('Drupal\system\Form\ThemeExperimentalConfirmForm', $theme);
   
   @@ -168,6 +174,11 @@ public function setDefaultTheme(Request $request) {
   +        return $this->formBuilder()->getForm('Drupal\system\Form\ThemeExperimentalConfirmForm', $theme, TRUE);
   

   🤓 Let's use FQCN::class instead of hardcoding a string?

6. +++ b/core/modules/system/src/Form/ThemeExperimentalConfirmForm.php
   @@ -0,0 +1,155 @@
   +    return $this->t('Are you sure you wish to install experimental theme');
   

   👎 Missing s? at the end — then the English will be unbroken and it will match \Drupal\system\Form\ModulesListExperimentalConfirmForm::getQuestion exactly.

7. +++ b/core/modules/system/src/Form/ThemeExperimentalConfirmForm.php
   @@ -0,0 +1,155 @@
   +    $theme = $form_state->getBuildInfo()['args'][0] ?? NULL;
   

   👎 AFAIK we're not yet using PHP 7-only syntax in Drupal 8.8, to simplify backports to Drupal 8.7.

8. +++ b/core/modules/system/src/Form/ThemeExperimentalConfirmForm.php
   @@ -0,0 +1,155 @@
   +    $setDefault = $form_state->getBuildInfo()['args'][1] ?? FALSE;
   

   🐫

9. +++ b/core/modules/system/src/Form/ThemeExperimentalConfirmForm.php
   @@ -0,0 +1,155 @@
   +          // The status message depends on whether an admin theme is currently in
   

   🤓 80 cols violation.

10. +++ b/core/modules/system/src/Form/ThemeExperimentalConfirmForm.php
    @@ -0,0 +1,155 @@
    +            $this->messenger()
    +              ->addStatus($this->t('Please note that the administration theme is still set to the %admin_theme theme; consequently, the theme on this page remains unchanged. All non-administrative sections of the site, however, will show the selected %selected_theme theme by default.', [
    +                  '%admin_theme' => $themes[$admin_theme]->info['name'],
    +                  '%selected_theme' => $themes[$theme]->info['name'],
    +            ]));
    

    🤓 Indentation issues.

11. +++ b/core/modules/system/src/Form/ThemeExperimentalConfirmForm.php
    --- a/core/modules/system/system.install
    +++ b/core/modules/system/system.install
    

    👍 The changes here make perfect sense. Take what exists for modules, repeat the exact same code pattern for themes.

Hello, I've fixed some of the points.

3. Fixed
4. Fixed
6. Fixed - I have added the 's?' at the end, but It's not exactly the same with the documentation). Is it okay to stay like this?
9. Fixed
10. Fixed

I think adding a text/label is a very good solution that gives us room to come up with a design for Claro integrating it into the cards component and doesn't take too much space. The rest of the behaviors follow the current patterns, so +1 for this.

Hello,

Thanks @wim-leers for the remarks and @tinko for the fixes.

Here is a complete patch fixing all the remarks listed by Wim Leers.

For point 6, I added the question mark without the plural "s". Because unlike modules, themes can be installed only one at a time. What do you think?

Thanks!

1. +++ b/core/modules/system/src/Controller/SystemController.php
   @@ -288,7 +288,7 @@ public function themesPage() {
   -      // Add notes to default, administration and experimental theme.
   +      // Add notes to default theme, administration theme and experimental themes.
   

   Nit: 80 cols violation.

2. +++ b/core/modules/system/src/Form/ThemeExperimentalConfirmForm.php
   @@ -57,7 +57,7 @@ public static function create(ContainerInterface $container) {
   -    return $this->t('Are you sure you wish to install experimental theme');
   +    return $this->t('Are you sure you wish to install experimental theme?');
   

   +1 to going with singular like you wrote in #11, but then we need to say "a": […] install an experimental theme?

3. +++ b/core/modules/system/src/Form/ThemeExperimentalConfirmForm.php
   @@ -115,15 +115,15 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
                ]));
   

   Nit: the indentation of this is still wrong.

Thanks for the quick reply!

Here is a new patch addressing those issues :)

1. +++ b/core/modules/system/src/Form/ThemeExperimentalConfirmForm.php
   @@ -37,10 +37,12 @@ class ThemeExperimentalConfirmForm extends ConfirmFormBase {
   -  public function __construct(ThemeHandlerInterface $themeHandler, ThemeInstallerInterface $theme_installer) {
   +  public function __construct(ThemeHandlerInterface $themeHandler, ThemeInstallerInterface $themeInstaller) {
   

   Actually, $theme_installer was correct. We need to rename $themeHandler to $theme_handler too. I missed that in my earlier reviews! 😅

2. +++ b/core/modules/system/src/Form/ThemeExperimentalConfirmForm.php
   @@ -0,0 +1,162 @@
   +    $setDefault = $form_state->getBuildInfo()['args'][1] ? $form_state->getBuildInfo()['args'][1] : FALSE;
   

   Should be $set_default.

3. +++ b/core/modules/system/src/Form/ThemeExperimentalConfirmForm.php
   @@ -0,0 +1,162 @@
   +          // in use: a value of 0 means the admin theme is set to be the default
   

   That is no longer true as of #2587119: Form sets system.theme:admin to '0' breaking Quick Edit and making no sense :)

4. +++ b/core/modules/system/src/Form/ThemeExperimentalConfirmForm.php
   @@ -0,0 +1,162 @@
   +          if (!empty($admin_theme) && $admin_theme != $theme) {
   

   Can we use a strict comparison here?

5. +++ b/core/modules/system/src/Form/ThemeExperimentalConfirmForm.php
   @@ -0,0 +1,162 @@
   +      $this->messenger()->addError(
   +        $this->formatPlural(
   +          count($config_objects),
   +          'Unable to install @extension, %config_names already exists in active configuration.',
   +          'Unable to install @extension, %config_names already exist in active configuration.',
   +          [
   +            '%config_names' => implode(', ', $config_objects),
   +            '@extension' => $theme,
   +          ])
   +          );
   

   The last reroll broke the indentation on this.

Hi Wim. Here is a new patch fixing those issues.

No more remarks about the code :) Only structural remarks.

1. +++ b/core/modules/system/src/Controller/SystemController.php
   @@ -197,6 +197,7 @@ public function themesPage() {
   +      $theme->is_experimental = isset($theme->info['experimental']) && $theme->info['experimental'];
   

   This requires a line like

   experimental: true
   

   to be added to a theme. That's different from how it works for experimental modules, where you have to do

   package: Core (Experimental)
   

   Why that difference?

2. This is missing a test, including a test theme, where we confirm this behaves as expected.
3. Once point 1 is addressed, this will need a change record.

Test coverage and test theme added, based on \Drupal\Tests\system\Functional\Module\ExperimentalModuleTest, which was added by #2663796: Use a confirmation form when enabling experimental modules. To prove and demonstrate I tried to stay as close as possible to the pattern established in that issue, I included similarity-comparison.txt _{(which was created using git diff -C -C -M20%)}.

This should still fail because this is only adding test coverage, and the logic in #15's patch doesn't yet handle the case of a non-experimental subtheme that uses an experimental base theme.

#18 should fail like this:

1) Drupal\Tests\system\Functional\Module\ExperimentalThemeTest::testExperimentalConfirmForm
Exception: Notice: Undefined offset: 1
Drupal\system\Form\ThemeExperimentalConfirmForm->submitForm()() (Line: 137)

Which points to a bug in the patch in #15. Fixing that first.

And finally, this fixes the problem that #18 already alluded to:

This should still fail because this is only adding test coverage, and the logic in #15's patch doesn't yet handle the case of a non-experimental subtheme that uses an experimental base theme.

So, to address my own review in #16:

1. There are at least two reasons to go with this approach:
   1. The choice to go with package: Core (Experimental) was not really conscious: it was pragmatic. We never got around to improving it.
   2. Using a key is clearer, more explicit and it even allows contrib themes to also mark themselves as experimental.
2. #18 + #19 + #20 addressed this. → Removing Needs tests
3. Change record created: https://www.drupal.org/node/3086531. → Removing Needs change record.

Fixing coding standards violations.

The test failure is sadly rather unhelpful — will dig in!

01:01:30.396 PHP Fatal error:  Cannot declare class Drupal\Tests\system\Functional\Module\ExperimentalThemeTest, because the name is already in use in /var/www/html/core/modules/system/tests/src/Functional/Theme/ExperimentalThemeTest.php on line 0

😳🤦‍♂️

Hi Wim,

Thank you for implementing the tests.
+1 for using experimental key (#21, 1st point). I think that that it will be useful to mark contrib themes as experimental.

+++ b/core/modules/system/src/Controller/ThemeController.php
@@ -117,6 +118,16 @@ public function install(Request $request) {
+      $all_themes = $this->themeHandler->rebuildThemeData();
...
@@ -168,6 +179,11 @@ public function setDefaultTheme(Request $request) {
+      $all_themes = $this->themeHandler->rebuildThemeData();
...
+++ b/core/modules/system/src/Form/ThemeExperimentalConfirmForm.php
@@ -0,0 +1,189 @@
+    $all_themes = $this->themeHandler->rebuildThemeData();
...
+    $themes = $this->themeHandler->rebuildThemeData();

Are all these rebuilds necessary? The ones in the confirmation form seem unnecessary but relatively harmless if they only run when you're trying to install an experimental theme. However, the one in install(), and worse, the one in setDefaultTheme(), looks like they would run even when you're installing / setting-to-default a non-experimental theme.

+++ b/core/modules/system/tests/src/Functional/Theme/ExperimentalThemeTest.php
@@ -0,0 +1,87 @@
+  /**
+   * Tests installing experimental themes and dependencies in the UI.
+   */
+  public function testExperimentalConfirmForm() {

This is nice test coverage. But it looks to be missing coverage for setting an already installed theme to the default. In particular, when doing that, what is the expected behavior for a theme that isn't experimental itself but depends on an experimental theme?

Also, looks like there's no test coverage for setting an experimental theme as the admin theme (e.g., verifying that its label has the "(experimental)" suffix) or verifying that "experimental theme" appears as a note in the Appearance page.

+++ b/core/modules/system/system.install
@@ -65,18 +65,33 @@ function system_requirements($phase) {
+    // Warn if any experimental themes are installed.
...
+    if (!empty($experimental_themes)) {
+      $requirements['experimental_themes'] = [
+        'title' => t('Experimental themes enabled'),
+        'value' => t('Experimental themes found: %theme_list. Experimental themes are provided for testing purposes only. Use at your own risk.', ['%theme_list' => implode(', ', $experimental_themes)]),
         'severity' => REQUIREMENT_WARNING,
       ];
     }

This also needs test coverage.

+++ b/core/modules/system/system.install
@@ -65,18 +65,33 @@ function system_requirements($phase) {
-        'value' => t('Experimental modules found: %module_list. <a href=":url">Experimental modules</a> are provided for testing purposes only. Use at your own risk.', ['%module_list' => implode(', ', $experimental), ':url' => 'https://www.drupal.org/core/experimental']),
+        'value' => t('Experimental modules found: %module_list. <a href=":url">Experimental modules</a> are provided for testing purposes only. Use at your own risk.', ['%module_list' => implode(', ', $experimental_modules), ':url' => 'https://www.drupal.org/core/experimental']),
...
+        'value' => t('Experimental themes found: %theme_list. Experimental themes are provided for testing purposes only. Use at your own risk.', ['%theme_list' => implode(', ', $experimental_themes)]),

Let's also open a followup to create documentation that we can link "Experimental themes" to, to match how "Experimental modules" is a link to documentation.

#29: great question. I should've dug deeper into that in my own review prior to starting the work on this. I don't think this matters a great deal though because installing extensions is a rare operation. Nevertheless: fixed.

(I think @kamalzairig simply chose to use the service that was available to him already 🙂)

#30:

This is nice test coverage.

It better be, because it's identical to the test coverage in \Drupal\Tests\system\Functional\Module\ExperimentalModuleTest, like I described in #18!

But it looks to be missing coverage for setting an already installed theme to the default.

Good catch! 👍 Added this test coverage.

In particular, when doing that, what is the expected behavior for a theme that isn't experimental itself but depends on an experimental theme?

The patch already contains test coverage for when installing a non-experimental theme that depends on an experimental theme. It does not yet contain test coverage for installing and setting as the default at the same time in that scenario. And in fact, there's a bug in there, which causes that missing test to fail — I'm sure that's why you asked specifically about that 🧐👍😊

Also, looks like there's no test coverage for setting an experimental theme as the admin theme (e.g., verifying that its label has the "(experimental)" suffix) or verifying that "experimental theme" appears as a note in the Appearance page.

I didn't add that because \Drupal\Tests\system\Functional\Module\ExperimentalModuleTest doesn't test that either. Of course, the module listing page is different than the theme listing page. Plus, experimental modules are all in that one package, and hence are implicitly labeled already, unlike themes. So: 👍, added that test coverage too.

• #31: I'm not sure that's worth it. #2560597: Mark Migrate* modules as Experimental added the status report entry for experimental modules, and also didn't come with test coverage.
• #32: Done: #3086761: Document the concept, purpose and requirements for experimental themes. Initially I just wanted to update https://www.drupal.org/core/experimental. But the word "module" appeared all over that page. So yeah, easier said than done. The way that is structured is pretty much a release manager decision, because this handbook page is such an important reference point.

Updated issue summary.

I made Claro an experimental theme and here's what I could find:

1. 
   👍Claro is now listed as an experimental theme in the theme list.
2. 
   👎I started installing the theme and I could see that the text doesn't read well if the experimental theme depends on multiple base themes.
3. 

   👍 When I have an admin theme set to something else than the default option and I try to install and set the theme as the default theme, I got this helpful message.

4. 

   👍 After enabling the theme, I can see a warning in the status report that I have an experimental theme enabled on my site.

I'm setting this back to needs work to address point 2.

Exciting feature! Glad we are going to support experimental themes

1. +++ b/core/modules/system/src/Controller/ThemeController.php
   @@ -149,6 +167,24 @@ public function install(Request $request) {
   +    $all_themes = $this->themeList->getList();
   ...
   +    $is_experimental = function ($theme) use ($all_themes) {
   +      return isset($all_themes[$theme]) && isset($all_themes[$theme]->info['experimental']) && $all_themes[$theme]->info['experimental'] && $all_themes[$theme]->status == 0;
   +    };
   +    return $is_experimental($theme) || array_sum(array_map($is_experimental, $dependencies)) > 0;
   

   This seems overly complicated and hard to read for me. It doesn't seem we actually need to make $is_experimental a function.

   Why not just first make an array of $themes_to_enable which would be $all_themes[$theme]->requires and $theme.

   Then loop through $themes_to_enable and check the condition we set up in $is_experimental now. First one that matches returns true. If no matches after the loop return false.

   I guess this personal preference but with creating the function variable, the array_map, array_sum and the || which means are calling $is_experimental in 2 different spots it just seem overly complicated.

   Also if $theme is put as the first item in the array this way would technically be more efficient because now if $theme is not experimental then $is_experimental is called on all elements in $depencies whereas the way I am suggesting it only needs to test items till it finds the first experimental. I know $all_themes[$theme]->requires is likely to have 1 or 2 elements at most but still.

2. +++ b/core/modules/system/src/Controller/ThemeController.php
   @@ -149,6 +167,24 @@ public function install(Request $request) {
   +      return isset($all_themes[$theme]) && isset($all_themes[$theme]->info['experimental']) && $all_themes[$theme]->info['experimental'] && $all_themes[$theme]->status == 0;
   

   Use === for checking status

   \Drupal\Core\Extension\ThemeExtensionList::doList() explicitly sets as an int.

3. +++ b/core/modules/system/src/Controller/ThemeController.php
   @@ -117,6 +130,11 @@ public function install(Request $request) {
   +      if ($this->willInstallExperimentalTheme($theme)) {
   +        return $this->formBuilder()->getForm(ThemeExperimentalConfirmForm::class, $theme);
   +      }
   

   We need to update the @return for this function. It now can return as well as the existing RedirectResponse

4. +++ b/core/modules/system/src/Form/ThemeExperimentalConfirmForm.php
   @@ -0,0 +1,189 @@
   +              ->addStatus($this->t('Please note that the administration theme is still set to the %admin_theme theme; consequently, the theme on this page remains unchanged. All non-administrative sections of the site, however, will show the selected %selected_theme theme by default.', [
   +                '%admin_theme' => $themes[$admin_theme]->info['name'],
   +                '%selected_theme' => $themes[$theme]->info['name'],
   +              ]));
   ...
   +          'Unable to install @extension, %config_names already exists in active configuration.',
   +          'Unable to install @extension, %config_names already exist in active configuration.',
   

   I noticed these are places we are duplicating messages from existing code and neither this or the existing code as test coverage for these message. Should a do a minor follow to test all possible messages when enabling themes?

5. +++ b/core/modules/system/tests/src/Functional/Theme/ExperimentalThemeTest.php
   @@ -0,0 +1,128 @@
   +    // Reinstall the same experimental theme, but this time immediately set it
   +    // as the default. This should again trigger a confirmation form with an
   +    // experimental warning.
   +    $this->drupalGet('admin/appearance');
   +    $this->cssSelect('a[title="Install Experimental dependency test as default theme"]')[0]->click();
   +    $this->assertText('Experimental themes are provided for testing purposes only. Use at your own risk.');
   

   The comment here seems off. We not installing the experimental theme. We are installing the theme that depends on the experimental theme.

I updated the draft CR: https://www.drupal.org/node/3086531/revisions/view/11594225/11596547.

#38.2 I agree this message looks awkward, but it is a direct copy of the wording when installing an experimental module, we should open a follow-up to improve the wording in both places.

#39.1 I updated this per your suggestion, I can see why a function would be a good choice since it's checking the same condition on the theme and the dependencies, but I agree that a simpler loop is more clear.

#39.2 Used strict equality here, and updated the comment in setDefaultTheme since I was confused at first why this would be necessary.

#39.3 Added "array" as a possible return value and updated the comment.

#39.4 Another followup here would be good too.

#39.5 Changed the wording in the comment

@zrpnr thanks for the updates. Just a couple more things and the 2 follows need to be created.

1. #38.2 I agree with @zrpnr that we should keep the text same for modules and themes and open up a follow-up if we want to change it.

2. +++ b/core/modules/system/src/Controller/ThemeController.php
   @@ -119,8 +119,9 @@
   +   * @return \Symfony\Component\HttpFoundation\RedirectResponse|array
   +   *   Redirects back to the appearance admin page or the confirmation form
   +   *   if an experimental theme will be installed.
   

   This looks good. Sorry didn't notice \Drupal\system\Controller\ThemeController::setDefaultTheme() has the same issue and needs its @return updated. Probably can just copy all of this there.

3. +++ b/core/modules/system/src/Controller/ThemeController.php
   @@ -179,10 +180,15 @@
   -    $is_experimental = function ($theme) use ($all_themes) {
   -      return isset($all_themes[$theme]) && isset($all_themes[$theme]->info['experimental']) && $all_themes[$theme]->info['experimental'] && $all_themes[$theme]->status == 0;
   -    };
   -    return $is_experimental($theme) || array_sum(array_map($is_experimental, $dependencies)) > 0;
   +    $themes_to_enable = array_merge([$theme], $dependencies);
   +
   +    foreach ($themes_to_enable as $name) {
   +      if ($all_themes[$name]->status === 0 && isset($all_themes[$name]->info['experimental']) && $all_themes[$name]->info['experimental']) {
   

   I think we still need to check isset($all_themes[$name]) as the first conditioned to be evaluated . incase it is somehow is not set the other conditions don't get evaluated.

   But actually...

   Could we shorten this whole thing by doing
   if (!empty($all_themes[$name]->info['experimental']) && $all_themes[$name]->status === 0) {
   Because this way the second condition will be checked if isset($all_themes[$name]) !== TRUE(as a side effect of the first check).

   That way we can cut the conditions for the original 4 to 2.

#42.2 Fixed, good catch!
#42.3 That is much more succinct and readable

Looks good to me! 🎉

Thanks for pushing this across the finish line @zrpnr & @tedbow! 🙏

Adding reviewer credit.

Pushed #43 to 8.8.x and published the CR.

However, tagging it for needing followups identified in #41 and #42. Also, it occurs to me that part of what makes #38.2 worse (in the case of Claro) is that...

+++ b/core/modules/system/src/Form/ThemeExperimentalConfirmForm.php
@@ -0,0 +1,189 @@
+    $dependencies = array_keys($all_themes[$theme]->requires);
...
+    if (!empty($dependencies)) {
+      // Display a list of required themes that have to be installed as well.
+      $items[] = $this->formatPlural(count($dependencies), 'You must enable the @required theme to install @theme.', 'You must enable the @required themes to install @theme.', [
+        '@theme' => $get_label($theme),
+        // It is safe to implode this because theme names are not translated
+        // markup and so will not be double-escaped.
+        '@required' => implode(', ', array_map($get_label, $dependencies)),
+      ]);
+    }

This lists all dependencies, including ones already "installed". E.g., in the case of enabling Claro, if you already have Bartik installed, then you already have Stable and Classy "installed" as well, so we don't really need to include them in the confirmation message. ModulesListConfirmForm doesn't, because it iterates on a $dependencies variable that only includes dependencies not yet installed. So let's also open a follow-up to clean that up for themes too. Because this message only appears when you're installing an experimental theme, and I don't think having it include extra base themes in that list is too terrible, I decided to not hold up commit on it.

Not sure why d.o. hasn't added the commit comment to this issue yet. So here's the commit: https://git.drupalcode.org/project/drupal/commit/20faabf.

Created the #38.2 follow-up that both #41 and #42 asked for: #3086941: Improve experimental theme/module installation warning.

Also created the follow-up that #47 asked for. Well-spotted. That would not have occurred if theme extensions had undergone the same improvements that module extensions have gone through. I agree we should not hold up improving this string on that though. See #3086942: Improve experimental theme confirmation warning to not list required base themes that are already installed.

Yay! 🔥🔥🔥 Thank you everyone! 🙏