Support from Acquia helps fund testing for Drupal Acquia logo

Comments

pfrenssen’s picture

Assigned: pfrenssen » Unassigned
Status: Active » Postponed
sun’s picture

Component: configuration system » theme system
Issue tags: +Configuration system
catch’s picture

Title: Convert the Maintenance Theme variable to CMI » Convert the Maintenance Theme variable to settings
Status: Postponed » Active

No longer blocked.

This has to work when the database is down so it should be in settings rather than CMI.

vijaycs85’s picture

Status: Active » Needs review
FileSize
2.33 KB

Updating as $conf as we got $conf['system.theme']['default']

gdd’s picture

catch do you have a problem with leaving this in $conf? I'd kind of prefer it myself.

+++ b/sites/default/default.settings.phpundefined
@@ -543,11 +543,11 @@
+ * $conf['system.theme']['maintenance']. The template file should also be copied into the

Nitpick: this comment is over 80 characters

socketwench’s picture

Reorganized comment to fit 80 character limit.

socketwench’s picture

Reorganized comment to fit 80 character limit.

vijaycs85’s picture

Issue tags: -Configuration system

Status: Needs review » Needs work
Issue tags: +Configuration system

The last submitted patch, 1829170-maintenance-theme-cmi-6.patch, failed testing.

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
2.44 KB

Re-rolling...

ACF’s picture

LittleCoding’s picture

Removing trailing whitespace.

vijaycs85’s picture

LinL’s picture

Updating for new Twig template location.

LinL’s picture

Updated to Drupal::config() as per #2028149: The config() function should be deprecated in favour of Drupal::config()

Also took out my change in #14 as that is probably scope creep.

vijaycs85’s picture

star-szr’s picture

Status: Needs review » Needs work

The change in #14 is fair game since we are already changing that line. If we are changing the line we should fix it!

LinL’s picture

Status: Needs work » Needs review
FileSize
2.47 KB

Thanks Cottser, never sure about ones like that :)

Status: Needs review » Needs work
Issue tags: -Configuration system

The last submitted patch, 1829170-maintenance-theme-cmi-18.patch, failed testing.

LinL’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 1829170-maintenance-theme-cmi-18.patch, failed testing.

star-szr’s picture

Status: Needs work » Needs review
Issue tags: +Configuration system
aspilicious’s picture

Status: Needs review » Needs work
+++ b/core/includes/theme.maintenance.inc
@@ -34,7 +34,7 @@ function _drupal_maintenance_theme() {
+    $custom_theme = isset($conf['system.theme']['maintenance']) ?: 'seven';

I can be wrong but this will return a boolean in stead of the maintenance theme. The ternary shorthand with isset is dangerous :)

mtift’s picture

Status: Needs work » Needs review
FileSize
1.4 KB
2.57 KB

@aspilicious you are correct. An updated patch is attached that also includes a minor change to the text.

dawehner’s picture

Status: Needs review » Needs work

With the patch applied the logic is really long:

    if (isset($conf['system.theme']['maintenance'])) {
      $custom_theme = $conf['system.theme']['maintenance'];
    }
    elseif (Drupal::config('system.theme')->get('default')) {
      $custom_theme = Drupal::config('system.theme')->get('default');
    }
    else {
      $custom_theme = 'bartik';
    }

    if (!$custom_theme)  {
      $config = Drupal::config('system.theme');
      // A broken install might not return an object.
      if (is_object($config)) {
        $custom_theme = $config->get('default');
      }
    }
    if (!$custom_theme)  {
      $custom_theme = 'bartik';
    }

This seems to be really duplicated information

mtift’s picture

Status: Needs work » Needs review
FileSize
662 bytes
2.69 KB

@dawehner: thanks for the review. I agree. Updated patch is attached.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

This is indeed way easier :)

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

So I don't think we should be using CMI here at all we should be using the Settings object since this should only be configured in settings.php as it is used when the database is unavailable.

andypost’s picture

Status: Needs work » Needs review
FileSize
3.09 KB

CMI could use a kind of caching and needs directories configured so let's use $settings. (#3 catch and #28 alexpott suggest that)

There's a bit of confusion because the same settings used for install-update but defaults to 'seven', probably this should be preserved for custom profile installers

Status: Needs review » Needs work

The last submitted patch, drupal8.theme-system.1829170-29.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
3.46 KB
1.49 KB

Yeah the required logic is kind of odd.

andypost’s picture

Makes sense!

aspilicious’s picture

Status: Needs review » Reviewed & tested by the community

Back to rtbc. looks ok to me...

vijaycs85’s picture

+1 for RTBC.

catch’s picture

Priority: Normal » Major

Rather than checking for $config being true and an object, should we be doing try/catch there instead?

webchick’s picture

Status: Reviewed & tested by the community » Needs review

Sounds like a "needs review"

ianthomas_uk’s picture

Here is a patch using try/catch.

I've also reworked the comments and split the assignment inside the if into two separate lines, to help readability.

catch’s picture

Status: Needs review » Reviewed & tested by the community

Looks much better to me.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

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