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

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

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.

vijaycs85’s picture

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.

vijaycs85’s picture

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

Re-rolling...

ACF’s picture

LittleCoding’s picture

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

Removing trailing whitespace.

vijaycs85’s picture

LinL’s picture

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

Updating for new Twig template location.

LinL’s picture

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.

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

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

dawehner’s picture

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

mtift’s picture

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.

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

dawehner’s picture

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.

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

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.

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.