Files: 
CommentFileSizeAuthor
#37 maintenance-1829170-37.patch3.91 KBianthomas_uk
PASSED: [[SimpleTest]]: [MySQL] 58,638 pass(es). View
#37 1829170-interdiff-31-37.txt2.17 KBianthomas_uk
#31 interdiff.txt1.49 KBdawehner
#31 maintaince-1829170-31.patch3.46 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 58,705 pass(es). View
#29 drupal8.theme-system.1829170-29.patch3.09 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 58,512 pass(es), 2 fail(s), and 4,104 exception(s). View
#26 1829170-maintenance-theme-cmi-26.patch2.69 KBmtift
PASSED: [[SimpleTest]]: [MySQL] 58,836 pass(es). View
#26 interdiff.txt662 bytesmtift
#24 1829170-maintenance-theme-cmi-24.patch2.57 KBmtift
PASSED: [[SimpleTest]]: [MySQL] 58,303 pass(es). View
#24 interdiff.txt1.4 KBmtift
#18 1829170-maintenance-theme-cmi-18.patch2.47 KBLinL
PASSED: [[SimpleTest]]: [MySQL] 57,296 pass(es). View
#15 1829170-maintenance-theme-cmi-15.patch2.46 KBLinL
PASSED: [[SimpleTest]]: [MySQL] 57,563 pass(es). View
#15 interdiff-12-15.txt686 bytesLinL
#14 1829170-maintenance-theme-cmi-14.patch2.45 KBLinL
PASSED: [[SimpleTest]]: [MySQL] 58,751 pass(es). View
#14 interdiff.txt657 bytesLinL
#12 1829170-maintenance-theme-cmi-12.patch2.44 KBLittleCoding
PASSED: [[SimpleTest]]: [MySQL] 55,813 pass(es). View
#12 interdiff.txt665 bytesLittleCoding
#10 1829170-maintenance-theme-cmi-10.patch2.44 KBvijaycs85
PASSED: [[SimpleTest]]: [MySQL] 54,743 pass(es). View
#7 1829170-maintenance-theme-cmi-6.patch2.41 KBsocketwench
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1829170-maintenance-theme-cmi-6_0.patch. Unable to apply patch. See the log in the details link for more information. View
#6 1829170-maintenance-theme-cmi-6.patch2.41 KBsocketwench
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion. View
#4 1829170-maintenance-theme-cmi-4.patch2.33 KBvijaycs85
PASSED: [[SimpleTest]]: [MySQL] 52,258 pass(es). View

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
PASSED: [[SimpleTest]]: [MySQL] 52,258 pass(es). View

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

heyrocker’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

FileSize
2.41 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion. View

Reorganized comment to fit 80 character limit.

socketwench’s picture

FileSize
2.41 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1829170-maintenance-theme-cmi-6_0.patch. Unable to apply patch. See the log in the details link for more information. View

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
PASSED: [[SimpleTest]]: [MySQL] 54,743 pass(es). View

Re-rolling...

ACF’s picture

LittleCoding’s picture

FileSize
665 bytes
2.44 KB
PASSED: [[SimpleTest]]: [MySQL] 55,813 pass(es). View

Removing trailing whitespace.

vijaycs85’s picture

LinL’s picture

FileSize
657 bytes
2.45 KB
PASSED: [[SimpleTest]]: [MySQL] 58,751 pass(es). View

Updating for new Twig template location.

LinL’s picture

FileSize
686 bytes
2.46 KB
PASSED: [[SimpleTest]]: [MySQL] 57,563 pass(es). View

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

Cottser’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
PASSED: [[SimpleTest]]: [MySQL] 57,296 pass(es). View

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.

Cottser’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
PASSED: [[SimpleTest]]: [MySQL] 58,303 pass(es). View

@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
PASSED: [[SimpleTest]]: [MySQL] 58,836 pass(es). View

@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
FAILED: [[SimpleTest]]: [MySQL] 58,512 pass(es), 2 fail(s), and 4,104 exception(s). View

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
PASSED: [[SimpleTest]]: [MySQL] 58,705 pass(es). View
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

FileSize
2.17 KB
3.91 KB
PASSED: [[SimpleTest]]: [MySQL] 58,638 pass(es). View

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.