Problem/Motivation

The buttons on update.php hide their text when Olivero is used as a frontend theme.

Steps to reproduce

  1. Install standard
  2. Add the following code to user.install
    /**
     * Tests olivero.
     */
    function user_update_9302() {
    }
    
  3. Login as user 1
  4. Visit /update.php

Screenshots

Update.php button covered in blue
The screenshot above shows the Apply pending updates button text completely hidden.

Mouse over Update.php button half-covered in blue
The screenshot above shows the Apply pending updates button with the mouseover state. The button is still partially covered in dark blue.

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

On update.php, $settings['maintenance_theme'] = FALSE now defaults to Claro or Seven if one of them is installed instead of the default theme. If Claro and Seven are not installed, default theme is still used.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott created an issue. See original summary.

alexpott’s picture

alexpott’s picture

Issue summary: View changes
alexpott’s picture

This swaps the theme to claro instead of seven (which is not installed so we fallback to olivero) for update.php when claro is installed. We still should fix the buttons in olivero for update.php and we also need to fix claro in update.php too... it does not look much better. See the screenshot below:

Claro update.php screen not looking great.

I toyed with replicating the logic in _drupal_maintenance_theme() to fake install seven so we can fallback to something we know looks good. But after discussion with @lauriii and @catch I'm pretty convinced that fake enabling a theme during the update process is going to lead to more problems than it fixes.

I think we should do the minimal theme work here to make claro and olivero look okay during update.php and fill follow-up issues to refine and make them look great.

lauriii’s picture

alexpott’s picture

Here's a small tweak to the current Claro maintenance css that makes it behave more like seven for now. I think we could do something like this as a minimum to unblock release and then refine further in issues like #3085219: Installer is not very usable in Claro.

This patch results in update.php looking like this:

Claro in update.php with a message looking okay

alexpott’s picture

+++ b/core/modules/system/src/Theme/DbUpdateNegotiator.php
@@ -40,10 +55,9 @@ public function applies(RouteMatchInterface $route_match) {
     if (!$custom_theme) {
-      $config = $this->configFactory->get('system.theme');
-      $custom_theme = $config->get('default');
+      $custom_theme = $this->themeHandler->themeExists('claro') ? 'claro' : 'seven';
     }

I think this code allows maintenance_theme to be set to FALSE. And then it'll use your frontend theme. It seems really odd functionality. And it is completely undocumented and not supported by _drupal_maintenance_theme(). I think this bit is odd copy and paste from that method where it does:

      $custom_theme = Settings::get('maintenance_theme', '');
      if (!$custom_theme) {
        $config = \Drupal::config('system.theme');
        $custom_theme = $config->get('default');
      }

But that only occurs when not in the installer or updating. And when we are we do $custom_theme = Settings::get('maintenance_theme', 'seven'); and don't do any check for falseness.

lauriii’s picture

Title: Buttons in update.php do not work as expected in Olivero » Standard install profile uses Olivero for update.php
Component: Olivero theme » system.module
Spokje’s picture

mherchel’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
58.46 KB

I tested the patch above in Chrome, Safari, and FF and at multiple viewports. It seems to work perfectly, and the CSS looks correct. I looked over the changes to DbUpdateNegotiator.php, and while I'm not familiar enough with the internals of Drupal's PHP to properly evaluate the code, it seems to be correct, and does the job properly.

RTBC pending tests passing!

mherchel’s picture

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 9: 3279640-8.patch, failed testing. View results

alexpott’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
573 bytes
4.36 KB
4.33 KB

Whoops forgot to inject the service. At least we've proved the deprecation works :)

Spokje’s picture

Now with extra valid (unpublished) CR-link.

Spokje’s picture

FileSize
1.18 KB
4.01 KB
Spokje’s picture

And a 10.0.x-dev patch without deprecation.

lauriii’s picture

Issue summary: View changes
Issue tags: +9.4.0 release notes
lauriii’s picture

Issue summary: View changes

  • lauriii committed 0f3e14a on 10.0.x
    Issue #3279640 by alexpott, Spokje, mherchel, lauriii, catch: Standard...

  • lauriii committed 1015c1b on 9.5.x
    Issue #3279640 by alexpott, Spokje, mherchel, lauriii, catch: Standard...

  • lauriii committed a585ce2 on 9.4.x
    Issue #3279640 by alexpott, Spokje, mherchel, lauriii, catch: Standard...

lauriii credited catch.

lauriii’s picture

Status: Reviewed & tested by the community » Fixed

Discussed with @catch and he asked for a release note on this. I added a draft release note to the IS.

Committed 0f3e14a and pushed to 10.0.x. Committed patch from #14 to 9.5.x and cherry-picked to 9.4.x. Thanks!

catch’s picture

Release note looks good - just in case someone gets confused by the theme change.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

Gábor Hojtsy’s picture

Reviewing this for the release notes of 9.4.0-beta1. The release note snippet says the default theme will still be used if neither Seven nor Claro are installed, but that is not what's in the code? It appears it would fall back on Seven regardless of installation status for Seven.

lauriii’s picture

The code is very confusing 😅 If you check \Drupal\Core\Theme\ThemeNegotiator::determineActiveTheme you can see that if the theme is not installed, \Drupal\system\Theme\DbUpdateNegotiator doesn't do anything.

quietone’s picture

Status: Closed (fixed) » Needs review

Hmm, lost my previous comment.

I added a CR for reference in the release notes, changing to NR for review of those CR.

xjm’s picture

Status: Needs review » Fixed

Thanks @quietone!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.