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

Assigned:pfrenssen» Unassigned
Status:Active» Postponed

Component:configuration system» theme system
Issue tags:+Configuration system

Title:Convert the Maintenance Theme variable to CMIConvert 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.

Status:Active» Needs review
StatusFileSize
new2.33 KB
PASSED: [[SimpleTest]]: [MySQL] 52,258 pass(es).
[ View ]

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

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

StatusFileSize
new2.41 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

Reorganized comment to fit 80 character limit.

StatusFileSize
new2.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.

Issue tags:-Configuration system

#7: 1829170-maintenance-theme-cmi-6.patch queued for re-testing.

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

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

Status:Needs work» Needs review
StatusFileSize
new2.44 KB
PASSED: [[SimpleTest]]: [MySQL] 54,743 pass(es).
[ View ]

Re-rolling...

StatusFileSize
new665 bytes
new2.44 KB
PASSED: [[SimpleTest]]: [MySQL] 55,813 pass(es).
[ View ]

Removing trailing whitespace.

StatusFileSize
new657 bytes
new2.45 KB
PASSED: [[SimpleTest]]: [MySQL] 58,751 pass(es).
[ View ]

Updating for new Twig template location.

StatusFileSize
new686 bytes
new2.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.

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!

Status:Needs work» Needs review
StatusFileSize
new2.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.

Status:Needs work» Needs review

Status:Needs review» Needs work

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

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

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 :)

Status:Needs work» Needs review
StatusFileSize
new1.4 KB
new2.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.

Status:Needs review» Needs work

With the patch applied the logic is really long:

<?php
   
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

Status:Needs work» Needs review
StatusFileSize
new662 bytes
new2.69 KB
PASSED: [[SimpleTest]]: [MySQL] 58,836 pass(es).
[ View ]

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

Status:Needs review» Reviewed & tested by the community

This is indeed way easier :)

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.

Status:Needs work» Needs review
StatusFileSize
new3.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.

Status:Needs work» Needs review
StatusFileSize
new3.46 KB
PASSED: [[SimpleTest]]: [MySQL] 58,705 pass(es).
[ View ]
new1.49 KB

Yeah the required logic is kind of odd.

Makes sense!

Status:Needs review» Reviewed & tested by the community

Back to rtbc. looks ok to me...

+1 for RTBC.

Priority:Normal» Major

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

Status:Reviewed & tested by the community» Needs review

Sounds like a "needs review"

StatusFileSize
new2.17 KB
new3.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.

Status:Needs review» Reviewed & tested by the community

Looks much better to me.

Status:Reviewed & tested by the community» Fixed

Committed/pushed to 8.x, thanks!

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