Problem/Motivation

In system_theme_suggestions_maintenance_page(), a template suggestion is provided for maintenance-page--offline.html.twig:

if ($offline) {
  $suggestions[] = 'maintenance_page__offline';
}

However, this template is not picked up when the system is offline. This results in a generic, unstyled exception page (i.e. a WSOD), creating a poor user experience. This is functionality that did exist and work in Drupal 7, and an attempt was made to have this functionality ported to Drupal 8, but appears to not have been completed/successful.

This is a major issue because it has no workaround and is a regression from Drupal 7.

Cause a significant admin- or developer-facing bug with no workaround.

Steps to reproduce

Scenario A (when core/includes/errors.inc is used):

  1. Set correct database credentials in settings.php
  2. Clear cache: drush cr
  3. Set $config['system.logging']['error_level'] = 'hide' in settings.php
  4. Set $settings['maintenance_theme'] = 'bartik' in settings.php
  5. Set incorrect database credentials in settings.php (for example invalid username)
  6. Refresh the page.
  7. There is a plain text message: "The website encountered an unexpected error. Please try again later."

Scenario B (when \Drupal\Core\EventSubscriber\FinalExceptionSubscriber is used):

  1. Make sure that $settings['hash_salt'] in set in settings.php
  2. Clear cache: drush cr
  3. Set $config['system.logging']['error_level'] = 'hide' in settings.php
  4. Set $settings['maintenance_theme'] = 'bartik' in settings.php
  5. Comment out $settings['hash_salt'] in settings file
  6. Refresh the page.
  7. There is a plain text message: "The website encountered an unexpected error. Please try again later."

Proposed resolution

Adds a way to render a template when in a fatal error context first by trying to get the maintenance theme in settings and then by the slower extension discovery if the that fails. Finally it falls back to a WSOD like fatal message.

Scenario A is addressed by calling the new static method in errors.inc.
Scenario B is addressed by calling the new static method in FinalExceptionSubscriber.

These scenarios are addressed in a new browser test.

Remaining tasks

User interface changes

Fatal errors will be attempted to be rendered using the maintenance theme "offline" template. This would cause previously displaying a WSOD to potentially show a themed page instead.

API changes

  • New public method Drupal\Core\Utility\Error::renderFatalError

Data model changes

N/A

Release notes snippet

This is a patch (bugfix) for a regression due to incomplete porting of functionality from Drupal 7 to Drupal 8 (and now Drupal 9). Drupal 7 allowed for the implementation of maintenance-page--offline.tpl.php, a template that to be shown when the site was offline and unable to connect to the database. The porting of this functionality to Drupal 8 and 9 was incomplete, and the equivalent Twig template, maintenance-page--ofline.html.twig, was not picked up, resulting in a generic, unstyled exception page (i.e. a WSOD) with a poor user experience. This but has now been fixed, and site themers can implement maintenance-page--ofline.html.twig on Drupal 8 and 9 sites.

Who this affects

This bugfix affects Drupal theme developers. Theme developers will now be able to create a template page to be displayed when Drupal is unable to reach the database, providing an improved user experience.

How to Implement

  1. Copy core/modules/system/templates/maintenance-page.html.twig to the theme to be used when your site is offline.
  2. (Optional) Edit the template to provide the HTML you would like visitors to see when the site is offline
  3. Edit settings.php, uncomment the line $settings['maintenance_theme'] = 'bartik';, and change the value from bartik to the machine name of the theme you chose in step one above.
  4. Clear your registry

How to test the template

After performing the steps above, the way to test that the template is working is to force a fatal error on your site. Warning: it is strongly advised to not test this on a production server. Fatal errors can be forced as follows:

  1. Set $config['system.logging']['error_level'] = 'hide'; in settings.php
  2. Set incorrect database credentials in settings.php (for example invalid username)
  3. Refresh the page
  4. Confirm that the template has been picked up
CommentFileSizeAuthor
#145 2720109-145.maintenance-page.patch22.34 KBhmdnawaz
#142 2720109-142.maintenance-page.patch22.39 KBjrb
#135 2720109-135.patch22.44 KBNigel Cunningham
#123 interdiff-2720109-121-123.txt1.03 KByogeshmpawar
#123 2720109-123.patch23.47 KByogeshmpawar
#121 2720109-121.patch22.41 KBranjith_kumar_k_u
#120 2720109-119.patch22.5 KBjoey-santiago
#102 interdiff_101_102.txt1.88 KBSpokje
#102 2720109-102.patch22.9 KBSpokje
#101 reroll_diff-95-101.txt5.61 KBSpokje
#101 2720109-101.patch22.76 KBSpokje
#95 2720109-95.patch22.66 KBkndr
#95 interdiff_83-95.txt5.21 KBkndr
#95 2720109-95-test-only.patch12.87 KBkndr
#93 2720109-83.patch22.67 KBquietone
#83 2720109-83.patch22.67 KBkndr
#83 interdiff_80-83.txt12.38 KBkndr
#83 2720109-83-test-only.patch12.81 KBkndr
#82 correct_test_only_scenario_a_should_fail.patch2.26 KBSpokje
#80 interdiff_76_80.txt2.07 KBSpokje
#80 test_only_scenario_a_should_fail.patch3 KBSpokje
#80 2720109-80.patch13.61 KBSpokje
#76 interdiff_75-76.txt981 byteskndr
#76 2720109-76.patch11.33 KBkndr
#75 interdiff_73-75.txt1.98 KBkndr
#75 2720109-75.patch10.47 KBkndr
#73 interdiff_67-73.txt9.39 KBkndr
#73 2720109-73.patch10.27 KBkndr
#67 2720109-67.patch8.53 KBmaacl
#67 interdiff_66-67.txt1.88 KBmaacl
#66 2720109-66.patch6.65 KBmaacl
#66 interdiff_60-66.txt3.45 KBmaacl
#60 2720109-60.patch7.53 KBsharma.amitt16
#57 2720109-57.patch6.74 KBsharma.amitt16
#54 2720109-54.patch7.49 KBsharma.amitt16
#52 2720109-52.patch6.72 KBsharma.amitt16
#47 2720109-47.patch6.75 KBsharma.amitt16
#43 2720109-43.patch6.4 KBsharma.amitt16
#30 2720109-20.patch3.6 KBNeslee Canil Pinto
#29 2720109-29.patch3.54 KBNeslee Canil Pinto
#21 theming-fatal-errors-2720109-21.patch4.82 KBfelribeiro
#20 theming-fatal-errors-2720109-20.patch4.59 KBfelribeiro
#12 theming-fatal-errors-2720109-12.patch4.84 KBzeuty

Issue fork drupal-2720109

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

seppelM created an issue. See original summary.

kiwimind’s picture

Seeing the same thing here and haven't found a way to get it to work yet.

There's a suggestion in system_theme_suggestions_maintenance_page in core/modules/system/system.module, which looks like it should work, but doesn't.

NikLP’s picture

Yeah this does look like a bug of sorts, the folks in #drupal-uk tried all sorts of things and couldn't get a template working, even putting a forced template in core/modules/system/templates

seppelM’s picture

Like I wrote, fatal errors are directly output without any "rendering",
except within the installation process → install_display_output() which calls drupal_maintenance_theme()
(@see errors.inc ~ 251)

- Maybe a plain .html file, to get around non-catchable needed services like Twig or dependent ones?
- otherwise render the --offline.html.twig which is enriched with "static" twig template variables or $config (settings.php)?

seppelM’s picture

Any update on this issue?

jenny.cha’s picture

Hi there,

Has anyone found an alternative way to create a custom offline maintenance page? Since this bug has been reported, there has not been an update for 7 months, so I'm curious as to if anyone else found a solution.

Thanks!

jenny.cha’s picture

Has anyone found an alternative solution to create a custom offline maintenance page? There hasn't been an update on this issue for about 7 months now...

cilefen’s picture

Version: 8.1.1 » 8.3.x-dev

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Jaypan’s picture

Priority: Normal » Major

Changing the status to major, as the system outputs a WSOD in the case of a fatal error, which is horrible UI. This fits into the status description for major due to this:

Cause a significant admin- or developer-facing bug with no workaround.

This is a significant bug and there is no workaround at the moment.

ekl1773’s picture

zeuty’s picture

First of all, steps to reproduce the issue.
Situation A (when core/includes/errors.inc is used):
1. Set incorrect database credentials in settings file.
2. Refresh the page.

Situation B (when core/lib/Drupal/Core/EventSubscriber/FinalExceptionSubscriber is used):
1. Comment out $settings['hash_salt'] in settings file
2. Refresh the page.

The patch adds a possibility to have a twig template that styles fatal errors described in A and B.

zeuty’s picture

Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, 12: theming-fatal-errors-2720109-12.patch, failed testing. View results

Ivan Berezhnov’s picture

Issue tags: +CSKyiv18

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Jaypan’s picture

I tested the patch in #12, unfortunately it does not work with 8.8.

jungle’s picture

Version: 8.6.x-dev » 8.9.x-dev
felribeiro’s picture

Status: Needs work » Needs review
FileSize
4.59 KB

Patch to the 8.9.x-dev

felribeiro’s picture

Patch to the 8.9.x-dev

The last submitted patch, 20: theming-fatal-errors-2720109-20.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 21: theming-fatal-errors-2720109-21.patch, failed testing. View results

mradcliffe’s picture

I performed Novice Triage on this issue. I am leaving the Novice tag on this issue because I think that we still need to update the issue summary here to make it clear what the steps to reproduce. Also we sould run this through Major issue triage and update the issue accordingly that would be great. Particularly we should also analyze based on Allowed changes in major and minor releases.

freed_paris’s picture

Hello,
This patch doesn't work anymore with Drupal 8.8.4 (or maybe earlier after 8.7.x)
Any solution ?

Thanks

Jaypan’s picture

Title: "Theming" page--maintenance--offline - fatal errors » maintenance-page--offline.html.twig is not picked up when system is offline
Issue summary: View changes
Jaypan’s picture

Issue summary: View changes
Jaypan’s picture

Issue summary: View changes
Neslee Canil Pinto’s picture

Rerolled the patch, Please review

Neslee Canil Pinto’s picture

Status: Needs review » Needs work

The last submitted patch, 30: 2720109-20.patch, failed testing. View results

Jaypan’s picture

I tested the patch from #30 on Drupal 8.8.4, and it did not work.

dcam’s picture

Thank you for your contribution, @Neslee Canil Pinto, but your two patches were incomplete. My guess is that since the template file was new it didn't get added to the Git diff.

I'm adding the Needs Reroll tag back. Whoever makes another attempt at this, make sure you reroll #21, not #29 or #30. Also, make sure you add the template to the diff.

bthompson1’s picture

I installed a clean site with drupal 8.9.x-dev as well as 8.4.4. The maintenance page loads without a problem with bartik. I even have twig debug enabled and the source code is showing the correct template file being used. Is this an issue with other themes? It's not clear why a patch is needed at this point. I can help with this issue but I'm unable to reproduce the error originally noted based on the information provided. If anyone can provide more details that would be helpful. Thanks.

Jaypan’s picture

bthompson1

Did you follow the steps to reproduce?

bthompson1’s picture

Assigned: Unassigned » bthompson1
andrey.troeglazov’s picture

I have tested patch from #21 with 8.9.x-dev and 8.8.x-dev and it works for me.
So why does it need to be rerolled?

bthompson1’s picture

I agree, #21 works. #30 doesn't work as it's missing the template file.

piyuesh23’s picture

Issue tags: +DIACWApril2020
Jaypan’s picture

I just tested the patch in 21. It is an improvement on the existing UX, giving an error page, versus the default error message (or nothing) on the WSOD.

However, the template file is named incorrectly. The system is expecting the template file maintenance-page--offline.html.twig, as outlined in the opening post, but the patch is using fatal-error-page.html.twig. So this still needs some work.

Definitely better than the current core default though.

bthompson1’s picture

If a site is throwing fatal errors, which is what is happening in the steps to reproduce, then this really has nothing to do with maintenance mode in my opinion. I consider these separate issues and see a fatal error template as reasonable.

Jaypan’s picture

If a site is throwing fatal errors, which is what is happening in the steps to reproduce, then this really has nothing to do with maintenance mode in my opinion.

Maybe that's a separate issue then.
The current issue is about maintenance-mode--offline.html.twig not being picked up, functionality that was part of D7, whose implementation was only partially completed in D8.

I consider these separate issues and see a fatal error template as reasonable.

Maybe for that issue, but it does not solve the current issue.

sharma.amitt16’s picture

Assigned: bthompson1 » sharma.amitt16
Status: Needs work » Needs review
FileSize
6.4 KB

@Jaypan, I have modified patch #21. Removed dependency on Bartik's theme added in #21.
Now all the fatal errors are themable. Now anyone can override the maintenance offline template in their theme.
I hope this is what you are addressing in this issue.

@NOTE: Don't use this patch instead use patch #47.

sharma.amitt16’s picture

Issue tags: +drupal core
jungle’s picture

Cleanup tags, `drupal core` is unnecessary and IS updated, Rerolled.

Status: Needs review » Needs work

The last submitted patch, 43: 2720109-43.patch, failed testing. View results

sharma.amitt16’s picture

Status: Needs work » Needs review
FileSize
6.75 KB

Status: Needs review » Needs work

The last submitted patch, 47: 2720109-47.patch, failed testing. View results

bthompson1’s picture

If you look at the code for system_theme_suggestions_maintenance_page it checks a constant to determine if the site is in maintenance mode. This is when processes like update and authorize are running. Only then will the page template maintenance-mode--offline.html.twig ever be picked up.

The steps to reproduce the issue are not valid as they don't result in the site site being "offline". Instead they are fatal errors which have nothing to do with the MAINTENANCE_MODE indicator.

I'm not even sure why there's a need for an offline template. you need to put your site in maintenance mode for the offline template to ever be picked up. And it would seem that if you are running processes to update your site the maintenance page template should suffice.

Jaypan’s picture

You’ll have to go back and find out the rationale on it. Nevertheless it’s functionality that existed in D7, but the migration to D8 is incomplete leaving this functionality broken.

sharma.amitt16’s picture

Status: Needs work » Needs review

I agree with @bthompson1, it is advised to keep the site in maintenance mode before doing update activity.

But I believe @jaypan is reporting about the fatal error messages (like 500, website encountered an error with backtrace messages) which render as an unformatted text on the browser. He is talking about the templatizing of these error messages. Also, I think this is not related to offline mode. @jaypan, would you please confirm if I am not wrong.

If it is related to templatizing of error messages only then patch #47 is working fine for different fatal error messages that occurred on the site. Also after applying this patch, you can customize the template in your custom theme.

@NOTE: If it is not related to maintenance mode then the naming convention of template Is a point of discussion. What name should be given to the template?

sharma.amitt16’s picture

Modifications for test case failure.

Status: Needs review » Needs work

The last submitted patch, 52: 2720109-52.patch, failed testing. View results

sharma.amitt16’s picture

Status: Needs work » Needs review
FileSize
7.49 KB

Status: Needs review » Needs work

The last submitted patch, 54: 2720109-54.patch, failed testing. View results

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

sharma.amitt16’s picture

Modified the patch for test case failure.

@NOTE: Please review Patch #54, as it is not compatible.

Status: Needs review » Needs work

The last submitted patch, 57: 2720109-57.patch, failed testing. View results

sharma.amitt16’s picture

Status: Needs work » Needs review
sharma.amitt16’s picture

Status: Needs review » Needs work

The last submitted patch, 60: 2720109-60.patch, failed testing. View results

sharma.amitt16’s picture

Status: Needs work » Needs review
mradcliffe’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

It looks like there are test failures so the correct status is Needs work. We probably also need to add some tests so I added the Needs tests tag.

I am leaving the Novice tag on this issue because it is still fairly clear what the next steps are.

It may also help to clarify the intent of the issue in the issue summary based on the discussion in earlier comments.

sharma.amitt16’s picture

Assigned: sharma.amitt16 » Unassigned
mradcliffe’s picture

Adding Global2020 and Bug Smash Initiative tags to hopefully move the issue forward. See my previous comments.

maacl’s picture

I would like to propose to use the offline theme from the settings.php instead of trying to get the active theme. In my testing I always got "seven" as active theme, so this should be more reliable.

I also added escaping of the error message and removed deprecated code.

maacl’s picture

Fixing tests. BigBipe explicitly expects the error message to not contain a <body>-Tag. The Error does contain this tag after applying the patch. So I removed the assertion.

quietone’s picture

Status: Needs review » Needs work

There is still work to do here, see #63.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

shaktik’s picture

Status: Needs work » Needs review

woking same patch on 9.2.x. Tested on local.

mradcliffe’s picture

Status: Needs review » Needs work
Issue tags: +Europe2020, +Needs issue summary update

I performed Novice Triage on this issue. I am leaving the Novice tag on this issue because I think that we can work on this together to add a test to the latest patch in order to assert the regression.

Bump back to Needs work because we should add a test for the patch since this is a regression. @shaktik, you can add a test run manually to a patch that is in Needs work without needing to change it.

Also I added the Needs issue summary tag because as it took a while to (re-)familiarize myself with what has happened so far in the issue. I think having a proposed resolution section to describe what we have done (code-wise) to resolve the issue.

kndr’s picture

Issue summary: View changes

I've changed "Steps to reproduce" slightly since there are two "main players" we should consider: error_level and maintenance_theme. In the test cases I suggest to focus on 'error_level = hide' because we should examine system behavior from a user point of view above all.

kndr’s picture

My review and a new patch proposal

  1. #67 patch fails at a case A from "Steps to reproduce". A html page with "Service unavailable" is expected but we get a plain text "The website encountered an unexpected error. Please try again later.".  It is caused by the fact that on a very early stage during fatal error handling we don't have an access to the container. It is not created because of inactive database. So we cannot use such functions like drupal_get_path().
  2. Final fatal errors are rendered as a html page now  so we need change comments in a FinalExceptionSubscriber.php
  3. Function  Error::renderFatalError() can throw its own exception so we shouldn't display this error (an exception message and a backtrace) if the fatal error is not displayable.
  4. If we can use 'maintenance-page--offline.html.twig' template it should be described in a default.settings.php
  5. If we don't check 'body' tag in a BigPipeTest.php a comment should be changed slightly
  6. Function drupal_get_path() has been deprecated (https://www.drupal.org/node/2940438) and we should use \Drupal::service('extension.list.theme')->getPath($theme)
  7. If error_level is set to verbose (case A from "Steps to reproduce") there is no empty line between the content and the error message
  8. We still need automated tests. In my opinion there are two existing test classes we may consider to change a little:  UncaughtExceptionTest and ErrorHandlerTest
kndr’s picture

I can't resist thoughts that this issue is just a 'feature request'. We don't get WSOD in 9.2.x any longer. All proposed changes lead to replace plain text message with a Twig template.

kndr’s picture

Let's try this patch. FinalExceptionSubscriber sends a html response if a request format is html and a plain text otherwise.

kndr’s picture

A small fix to pass a testExpectedScaffoldFiles.

kndr’s picture

Status: Needs work » Needs review

#76 patch is green so review is appreciated.

Jaypan’s picture

Status: Needs review » Reviewed & tested by the community

Tested and confirmed - the template is correctly picked up for both the test cases outlined in the issue outline. Thanks for the nice work!

Spokje’s picture

Assigned: Unassigned » Spokje
Status: Reviewed & tested by the community » Needs work
Issue tags: -Vienna2017, -CSKyiv18, -midcamp2020, -DIACWApril2020, -Global2020, -Europe2020, -Needs issue summary update

Tag clean-up and back to NW for Needs Tests (#63)

(IS was updated in #45)

Lemme see if I can make a failing test for both scenarios that passes with patch #76

Spokje’s picture

So I've managed to create a test for Scenario A in the IS (Change DB credentials.

I've attached that to patch#76 and as a test_only_should_fail patch.

For scenario B I've tried something similar:

  /**
   * Tests if the maintenance offline page is served when settings.php
   * originally did have a hash_salt defined, which is emptied out afterwards.
   */
  public function testRemovedHashSalt() {
    $this->drupalGet('');
    $this->assertSession()->statusCodeEquals(200);
    $this->assertSession()->pageTextContains('Log in');

    $settings = [];

    // Set $settings['hash_salt'] = '' in settings.php
    $settings['settings']['hash_salt'] = (object) [
      'value' => '',
      'required' => TRUE,
    ];
    $this->writeSettings($settings);

//    drupal_flush_all_caches();

    $this->drupalGet('');
    $this->assertSession()->statusCodeEquals(500);
    $this->assertSession()->pageTextContains('Service unavailable');
  }

but that doesn't work for TestBot.

Since reading the salt_hash happens very early in the bootstrap phase, changing it afterwards has an effect when doing actual request, but TestBot doesn't notice the change.

I've tried uncommenting drupal_flush_all_caches(); in the example above, but that throws an uncaught RuntimeException, which is an automatic fail for TestBot :/

Somebody with a bigger (Test Writing) brain than mine has to look at this.

Spokje’s picture

Assigned: Spokje » Unassigned

Leaving at "Needs work" because there's still no test for Scenario B as described in the Issue Summary.

Spokje’s picture

Please ignore the test_only patch from #80, this is the correct one.

kndr’s picture

Status: Needs work » Needs review
FileSize
12.81 KB
12.38 KB
22.67 KB

@Spokje, Thanks for your tests. They inspired me a lot. I tried hard to figured it out how to write an automated test when there is no 'hash_salt' value in a settings.php file. It turned out that you were really close to solve it. Function drupal_flush_all_caches() should be called before rewriting settings. The same situation is described in "Steps to reproduce". We should clear a cache first and than refresh a page. Besides this, I've added additional test cases because we should check if a maintenance theme's template is picked up when it exists in a 'template' directory. I hope it is OK now but review is appreciated.

The last submitted patch, 83: 2720109-83-test-only.patch, failed testing. View results

Jaypan’s picture

Was 2720109-83-test-only.patch supposed to fail?

kndr’s picture

Yes, 2720109-83-test-only.patch should fail. It replicates "Steps to reproduce" and proves that system without a 2720109-83.patch has a bug.

kndr’s picture

The idea of 'test-only' patches is explained at a step #10 on a page https://www.drupal.org/contributor-tasks/write-tests

Spokje’s picture

Issue tags: -Needs tests

Removed `Needs Tests`-tag since tests are here (thanks @kndr!🙏).

Since I myself worked on this issue I can't make it RTBC, but the quick scan I did on the code looks promising.

Jaypan’s picture

The idea of 'test-only' patches is explained at a step #10 on a page https://www.drupal.org/contributor-tasks/write-tests

Thank you, appreciated.
I don't have a testing system set up, or the time to do so, to be able to test it myself. When I do, I will, if no one has.

kndr’s picture

@Jaypan Can you mark this issue RTBC? There is a nice cheat sheet and a sample comment http://www.kristen.org/content/my-drupal-issue-queue-rtbc-cheat-sheet Thanks.

Jaypan’s picture

Status: Needs review » Reviewed & tested by the community

I'm matching RTBC as:
* Patch applies cleanly.
* Tests pass.
* Test-only patch fails as expected.
* No coding standard problems were found by the test bot.
* Code is clear and addresses the issue from the issue summary.
* New test code tests the specific issue from the issue summary.
* Feedback from previous comments has been addressed.
* Issue title is clear and relevant.
* The issue summary properly clarifies the issue
* Issue metadata looks okay.
* Manually tested and patch works as expected.

The last submitted patch, 83: 2720109-83-test-only.patch, failed testing. View results

quietone’s picture

Reuploading the patch in #83 because the fail patch is being tested every two days.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/assets/scaffold/files/default.settings.php
    @@ -572,10 +572,15 @@
    + * This applies also when the database is inactive due to an error or when a fatal
    + * error is thrown.
    + * The template file should also be copied into the theme. It is located inside
    + * 'core/modules/system/templates/maintenance-page--offline.html.twig'.
    
    +++ b/sites/default/default.settings.php
    @@ -572,10 +572,15 @@
    + * This applies also when the database is inactive due to an error or when a fatal
    + * error is thrown.
    + * The template file should also be copied into the theme. It is located inside
    + * 'core/modules/system/templates/maintenance-page--offline.html.twig'.
    

    The comment shoudl wrap at 80 chars/

  2. +++ b/core/lib/Drupal/Core/Utility/Error.php
    @@ -100,6 +106,71 @@ public static function renderExceptionSafe($exception) {
    +      // The maintenance theme is set but the container doesn't exist
    +      // since the database is inactive. Hence there are no services available
    +      // to retrieve a maintenance theme path. The path can be obtained by using
    +      // ExtensionDiscovery::scan() but it calls \Drupal::installProfile()
    +      // so a minimal mocked container has to be created earlier.
    +      $container = new ContainerBuilder();
    +      $container->setParameter('install_profile', NULL);
    +      \Drupal::setContainer($container);
    

    This can be removed if we call
    $listing->setProfileDirectories([]); after constructing the $listing object and before scanning.

  3. +++ b/core/modules/system/tests/src/Functional/System/MaintenancePageOfflineTest.php
    @@ -0,0 +1,212 @@
    +        'value' => $this->randomMachineName(),
    

    This has the potential to be randomly correct. I would take the current username and add a longer random string to it. Or something like that.

  4. +++ b/core/modules/system/tests/src/Functional/System/MaintenancePageOfflineTest.php
    @@ -0,0 +1,212 @@
    +    $this->assertSession()->pageTextContains('Log in');
    +
    +    drupal_flush_all_caches();
    +
    +    // CASE 1 - a maintenance theme's template is picked up
    +    // and no errors are displayed.
    ...
    +    $this->assertSession()->pageTextContains('Log in');
    +
    +    drupal_flush_all_caches();
    +
    +    // CASE 1 - a maintenance theme's template is picked up
    +    // and no errors are displayed.
    

    If the drupal_flush_all_caches() is necessary it need a comment as to why. I'm not sure it is necessary.

kndr’s picture

@alexpott, thank you for the review. You are absolutely right so I've made changes you suggested. As for the point 3 I decided to change the user password since it is impossible to set it correctly in this way. It turned out that drupal_flush_all_caches() was not necessary in an automated test but flushing the cache is necessary during manual testing 'hash_salt'. BTW, is it still valid to attach a test-only patch? If it is being tested every two days as @quietone said at #93 maybe test-only patch should be omitted?

The last submitted patch, 95: 2720109-95-test-only.patch, failed testing. View results

renatog’s picture

Status: Needs review » Needs work

In this case:

* The template file should also be copied into the theme. It is located inside
* 'core/modules/system/templates/maintenance-page--offline.html.twig'.

Please could you use "inside of"? E.g.:

* The template file should also be copied into the theme. It's located inside of
* 'core/modules/system/templates/maintenance-page--offline.html.twig'.

Brian C made their first commit to this issue’s fork.

Jaypan’s picture

If we're doing wording, I think "located at" is natural.

renatog’s picture

I agree

Spokje’s picture

Spokje’s picture

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Spokje’s picture

Used 2720109-102.patch as base for the MR against 9.3.x/code>

quietone’s picture

Came to do a review.

I started reading the Issue Summary. It is really nice to have steps to reproduce, A and B. However, there are no remaining tasks or proposed resolution. Without a proposed resolution it is not possible to confirm that the patch here is doing what it should be. Note that #71 also asked for a proposed resolution section to be added. Tagging for an issue summary update and keeping the novice tag, I think updating the IS for this issue is a suitable novice task.

Then I read the issue. And then I got very confused about when the maintenance mode template is to be used. See #34, #41 and #49, all of which did not, in my opinion receive an explanation. Perhaps the reason is in the IS where is says that Drupal 7 behaved this way. I don't know if that is true, but if it is it is a regression. If I understand correctly, this is using the maintenance mode twig file when the site is deliberately placed in maintenance mode and when a fatal error occurs. The IS just says that this is to use the twig file when the site is offline and when I think 'offline' I think maintenance mode not fatal. If that is correct the IS can state that more clearly. I am not doing that in case I am wrong.

This issue also should have before and after screenshots.

No time left for a code review.

Edit: s/the/then/

kndr’s picture

As I mentioned at #74 I see this issue more as "feature request" than "bug report". The issue priority is too high as well. I can agree that maintenance mode has nothing in common with handling fatal errors. The whole work that was done in this issue was focused on rendering fatal error page from twig template. That's all. I vote for changing the category to "feature request", priority to "normal" and topic to "Render fatal error page from twig template" or so. The last patch can handle this new feature but it uses a little confusing template's name (maintenance-page--offline.html.twig) in the new context.

quietone’s picture

Title: maintenance-page--offline.html.twig is not picked up when system is offline » Render fatal error page from twig template
Category: Bug report » Feature request
Priority: Major » Normal

@kndr, thank you! I was so focused on getting my head around this issue that I didn't even think about the issue category and priority.

I agree with kndr and have implemented the suggestions in #107.

Setting to NW to change the template name to something that reflects what the template is for.

Jaypan’s picture

Category: Feature request » Bug report
Priority: Normal » Major

This is a bug. It's behavior from Drupal 7 that was not ported to Drupal 8, containing documentation that indicates the template will be picked up and it is not. As this bug causes a fatal error with a WSOD, it is a major bug.

kndr’s picture

@jaypan Are you sure that it causes a fatal error with WSOD? Neither "Situation A" nor "Situation B" described in "Steps to reproduce" ends with WSOD. As a result we have a plain text message: "The website encountered an unexpected error. Please try again later." so it works as designed. Can you check this out manually in your environment? Maybe we missed something that causing WSOD. If so I can agree that this is a "bug report". Otherwise I'm pretty sure that this is "feature request".

Jaypan’s picture

Replace WSOD with 'fatal error' then. My point still stands that it's a bug, and that it shuts down your site with no control over the site builder to theme the page. This is a major bug.

we have a plain text message: "The website encountered an unexpected error. Please try again later." so it works as designed.

This is where I disagree. As the original post lays out, it was designed to pick up a TWIG template named maintenance-page--offline.html.twig. Drupal 7 allowed for maintenance-page--offline.tpl.php, however this functionality was not properly implemented in Drupal 8, and the corresponding twig template is not picked up, as it was designed to. Hence this thread is a bug report, not a feature request.

Jaypan’s picture

Title: Render fatal error page from twig template » maintenance-page--offline.html.twig is not picked up when system is offline
jwilson3’s picture

This is definitely a bug, not a feature request. The maintenance_page__offline template suggestion is not the only thing impacted by the broken render pipeline when a fatal issue is encountered.

There is also a Twig variable db_offline available in the html.html.twig template supplied by core, but there is no practical way to make use of this Twig variable.

mradcliffe’s picture

I performed Novice Triage on this issue. I am leaving the Novice tag on this issue because I think there is a good contribution opportunity for manual testing without a local environment to provide before/after screenshots that @quietone requested above. You should be able to do both Scenario A and Scenario B using DrupalPod.

I reviewed the current merge request and took a stab at providing the current proposed resolution and removed the Needs issue summary update tag.

I also added the Needs release note because this is a Major bug fix that site owners should be aware of. I'm not sure if that also means it needs a Change record or not.

Jaypan’s picture

Issue summary: View changes
Issue tags: -Needs release note

I've added a release note. I personally don't think a changelog is necessary, as this isn't a change, it's a bugfix. That said, I don't know the criteria for determining whether a changelog should be written, so I may be out of line on that.

Jaypan’s picture

Issue summary: View changes

I'm wondering if we should add an item to remaining tasks for writing a documentation page equivalent to Theming the Drupal maintenance page. The linked page contains the D7 documentation for using maintenance-page--offline.tpl.php.

Jaypan’s picture

Issue summary: View changes
Jaypan’s picture

Issue summary: View changes

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

joey-santiago’s picture

I did my best to reroll the patch on 9.3.x and here's the result.

I think there's something a little bit off with the way the template is looked up maybe? If i set my custom theme as maintenance in settings.php:

$settings['maintenance_theme'] = 'mytheme';

and i put my template in: web/themes/contrib/mytheme/templates/system/maintenance-page--offline.html.twig
it is not picked up. It only gets picked up if i put it in: web/themes/contrib/mytheme/templates/maintenance-page--offline.html.twig

This is not a huge deal, but it threw me off, because i have my "normal" maintenance page template in web/themes/contrib/mytheme/templates/system/maintenance-page.html.twig and that works correctly.

ranjith_kumar_k_u’s picture

Re-rolled #120 for 9.4

Status: Needs review » Needs work

The last submitted patch, 121: 2720109-121.patch, failed testing. View results

yogeshmpawar’s picture

Updated patch will fix the test failure.

Status: Needs review » Needs work

The last submitted patch, 123: 2720109-123.patch, failed testing. View results

yogeshmpawar’s picture

Status: Needs work » Needs review
Shashwat Purav’s picture

The patch in #123 applied successfully to 9.4.x branch.

Jaypan’s picture

Status: Needs review » Reviewed & tested by the community
tonytheferg’s picture

Patch works well. I used #121 because #123 would not apply to 9.3.9 via composer, and I didn't fell like requiring dev. Thanks!

hmdnawaz’s picture

I applied the patch, and it works.

But I am unable to override the maintenance-page--offline.html.twig template in my theme.

I copied the file to /themes/custom/mytheme/templates/system/maintenance-page--offline.html.twig, cleared the cache, but Drupal still displays the template from the system module instead of my theme.

Updated:
It is working in /themes/custom/mytheme/templates/maintenance-page--offline.html.twig

joey-santiago’s picture

Have you set your theme as maintenance mode theme in settings.php @hmdnawaz?

$settings['maintenance_theme'] = 'yourtheme';

hmdnawaz’s picture

@joey-santiago

I checked it with with and without that setting,

But now I place the template file directly in the templates directory without a system directory and it works without that setting
in the settings.php.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/lib/Drupal/Core/Utility/Error.php
    @@ -100,6 +105,69 @@ public static function renderExceptionSafe($exception) {
    +   * @param array $context
    +   *   Variables to be passed to twig template.
    

    I think this should detail the expected keys. Ie. title, content and displayable and say what they are. Or I wonder if instead of $context we should have 3 parameters.

  2. I wonder if we the title context should be mandatory - we're always providing it but we provide a default in the twig template.
  3. I also wonder if we're going to all the trouble of using a twig template here we should then also go the other way and stop putting html in the non html responses and only output clean text. But perhaps that's a separate issue. That said this does influence how we should create the template - ie. potentially we should do more in the template and do less html munging in the code.
hmdnawaz’s picture

There is no way to override or add new variables to the template.

I have tried with mytheme_preprocess_maintenance_page(&$variables) but it is not picked up.

function mytheme_preprocess_maintenance_page(&$variables) {
  $variables['title'] = 'new title';
  $variables['new_var'] = 'new_value';
}

It is still taking the variable values from the function _drupal_log_error

$html = Error::renderFatalError([
        'title' => 'Service unavailable',
        'content' => $message,
        'displayable' => error_displayable($error),
      ]);

Here when I add my custom variable like this:

$html = Error::renderFatalError([
        'title' => 'Service unavailable',
        'content' => $message,
        'displayable' => error_displayable($error),
        'custom_var' => 'custom value',
      ]);

and print it in the template as {{ custom_var }} it is working, but I think mytheme_preprocess_maintenance_page(&$variables) is not a preprocess function for this offline template.

Is there any other preprocess function available for this template? How can I override the existing variables of the template and add new variables?

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Nigel Cunningham’s picture

I would suggest that the test in FinalExceptionSubscriber not display the offline message if the error level is verbose. (Change the content type test to:

 if ($content_type == 'text/html' && !$this->isErrorLevelVerbose()) {

I've adjusted the patch from 121 in this attachment.

DrDam’s picture

#135 works against 9.4.x and 9.5.x

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

dariemlazaro’s picture

This should be taken more seriously as a matter of system reliability in the event of errors. After applying patch #135 in core 9.4.8 and copying the maintenance-page--offline.html.twig template to my theme templates directory (themes/custom/mytheme/templates/maintenance-page--offline.html.twig), the template renders correctly but it is not possible to modify any of its variables from my .theme file, it does not receive any CSS, nor JS, nor does it accept any preprocessing.

Analyzing the patch and trying to add new variables from errors.inc these worked in the template. To make the solution as generic as possible I made the following changes to errors.inc on line 277:

 $base_url = base_url(); //get the base URL of the site
      $base_dir = str_replace(basename($_SERVER['SCRIPT_NAME']), '', $_SERVER['SCRIPT_NAME']); //get the root dir
      $public_dir = $base_dir . 'sites/default/files/'; //get the public dir
      $public_dir_url = $base_url . 'sites/default/files/'; //get the public dir URL

      //Check for a favicon in public files directory with svg or ico mimetype(prefers svg)
       if (file_exists($public_dir . 'favicon.svg')) {
        $favicon = $public_dir_url . 'favicon.svg';
        }else if (file_exists($public_dir . 'favicon.ico')){
         $favicon = $public_dir_url . 'favicon.ico';
       }

      //Check for a logo in public files directory with svg or png mimetype(prefers svg)CHECK more mimes
      if (file_exists($public_dir . 'logo.svg')) {
        $site_logo = $public_dir_url . 'logo.svg';
      }else if (file_exists($public_dir . 'logo.png')){
        $site_logo = $public_dir_url . 'logo.png';
      }

      //Check for a css in public files directory(this fail)
//      if (file_exists($public_dir . 'maintenance/maintenance.css')) {
//        $styles = $public_dir_url . 'maintenance/maintenance.css';
//      }
      //force the load of the css
      $styles = $public_dir_url . 'maintenance/maintenance.css';

      //Check for a js in public files directory(this fail)
//      if (file_exists($public_dir . 'maintenance/maintenance.js')) {
//        $scripts = $public_dir_url . 'maintenance/maintenance.js';
//      }
      //force the load of the js
      $scripts = $public_dir_url . 'maintenance/maintenance.js';

      $html = Error::renderFatalError([
        'title' => 'Service unavailable',
        'content' => $message,
        'displayable' => error_displayable($error),
        'base_url' => $base_url,
        'favicon' => $favicon,
        'site_logo' => $site_logo,
        'styles' => $styles,
        'scripts' => $scripts,
      ]);

And this function at the end of the file:


if (!function_exists('base_url')) {
  function base_url($atRoot=FALSE, $atCore=FALSE, $parse=FALSE){
    if (isset($_SERVER['HTTP_HOST'])) {
      $http = isset($_SERVER['HTTPS']) && strtolower($_SERVER['HTTPS']) !== 'off' ? 'https' : 'http';
      $hostname = $_SERVER['HTTP_HOST'];
      $dir =  str_replace(basename($_SERVER['SCRIPT_NAME']), '', $_SERVER['SCRIPT_NAME']);

      $core = preg_split('@/@', str_replace($_SERVER['DOCUMENT_ROOT'], '', realpath(dirname(__FILE__))), NULL, PREG_SPLIT_NO_EMPTY);
      $core = $core[0];

      $tmplt = $atRoot ? ($atCore ? "%s://%s/%s/" : "%s://%s/") : ($atCore ? "%s://%s/%s/" : "%s://%s%s");
      $end = $atRoot ? ($atCore ? $core : $hostname) : ($atCore ? $core : $dir);
      $base_url = sprintf( $tmplt, $http, $hostname, $end );
    }
    else $base_url = 'http://localhost/';

    if ($parse) {
      $base_url = parse_url($base_url);
      if (isset($base_url['path'])) if ($base_url['path'] == '/') $base_url['path'] = '';
    }

    return $base_url;
  }
}

Then modify the maintenance-page--offline.html.twig template:

{#
/**
 * @file
 * Default theme implementation to display a single Drupal page while offline.
 *
 * @see template_preprocess_maintenance_page()
 *
 * @ingroup themeable
 */
#}
<!DOCTYPE html>
<html lang="en">
<head>
  <meta charset="utf-8">
  <meta name="viewport" content="width-device-width, initial-scale-1">
  <title>
{#    {% if title %}#}
{#      {{ title }}#}
{#    {% else %}#}
      Service unavailable
{#    {% endif %}#}
  </title>
  <link rel="shortcut icon" type="image/x-icon" href="{{ favicon }}">
  <link rel="stylesheet" href="{{ styles }}">
</head>

<body class="maintenance-page layout-no-sidebars path-frontpage">
<div class="container contenido-mantenimiento">
  <div class="caja" data-aos="fade-up" data-aos-duration="700" data-aos-delay="1000" data-aos-once="true">
    <header role="banner" data-aos="zoom-in" data-aos-duration="500" data-aos-delay="1300"
            data-aos-once="true">
      <img src="{{ site_logo }}" alt="Site name"/>
    </header>
    <main class="texto-mantenimiento" role="main">

      <h2 data-aos="fade-up" data-aos-duration="600" data-aos-delay="1600"
          data-aos-once="true">
{#        {{ title }}#}
        Your site name or something else
      </h2>

      <div data-aos="fade-up" data-aos-duration="600" data-aos-delay="1900" data-aos-once="true">
{#        {{ page.content }}#}
        Your personalized text explaining the error.
        <br><br>
        Some more text.
      </div>
    </main>
  </div>
</div>
<script src="{{ scripts }}"></script>
</body>
</html>

In this template I also decided to add animations to make it not so flat.

Then in the public files directory (/sites/default/files) add a favicon.svg, a logo.svg and a maintenance folder to put CSS, JS and images files.
Optional (for animations): place the AOS library in an aos folder inside maintenance.

This is my CSS in maintenance.css, note that the '@import' is for loading the aos library from the CSS itself:

@import "aos/aosnext.css";
body.maintenance-page {
  font-family: "Verdana", sans-serif;
  font-size: 14px;
  margin: 0;
  background-image: url("images/mantenimiento.jpg");
  background-attachment: fixed;
  background-position: center center;
  background-repeat: no-repeat;
  background-size: cover; }
body.maintenance-page .contenido-mantenimiento {
  display: flex;
  flex-direction: column;
  align-items: center;
  justify-content: center;
  height: 100vh; }
body.maintenance-page .contenido-mantenimiento .caja {
  padding: 1.5rem;
  text-align: justify;
  color: #000;
  background-color: rgba(255, 255, 255, 0.8); }
body.maintenance-page .contenido-mantenimiento header {
  display: flex;
  flex-direction: column;
  align-items: center;
  justify-content: center;
  margin-bottom: 1rem; }
body.maintenance-page .contenido-mantenimiento header img {
  width: 200px; }
body.maintenance-page .contenido-mantenimiento .texto-mantenimiento {
  display: flex;
  flex-direction: column;
  align-items: center;
  justify-content: center;
  text-align: center;
  font-size: 1rem; }
body.maintenance-page .contenido-mantenimiento .texto-mantenimiento h2 {
  text-align: center;
  margin-bottom: 1rem; }

This is my JS in maintenance.js, the function to concatenate the JS fails to load because of some access restriction, so I manually concatenated jQuery and the AOS library and then put my custom code:

// function include(filePath) {
//   const scriptTag = document.createElement("script");
//   scriptTag.src = filePath;
//   document.body.appendChild(scriptTag);
// }
// include("js/jquery.min.js");
// include("aos/aosnext.js");

//Jquery
/*! jQuery v3.6.0 | (c) OpenJS Foundation and other contributors | jquery.org/license */
<Full jQuery JS content>(I am not copying it here for obvious reasons)

//AOS
<Full AOS JS content>(I am not copying it here for obvious reasons)

//My code
jQuery(document).ready(function () {
  setTimeout(() => {
    AOS.init();
  }, 120);
});

After all this "superhuman" effort I managed to create an offline maintenance page with a similar style and behavior (animations) to the online maintenance page (just change the texts). Of this last one it would be necessary a way to access to those variables of the site and to show them in the template so that it is not necessary to put the plain text.

Please someone review these modifications to see what can be improved and please update the patch with these changes that are a temporary solution to this problem, it is not the best but it solves. I don't know how to update the patch on my own or make patches. Thanks in advance and I hope this solution will be helpful.

Translated with www.DeepL.com/Translator (free version)

newaytech’s picture

Thanks for the patch #151 - this applied for me on Drupal 9.5 - and I was able to change the offline page within a minimal theme I have created just for offline pages across all my multisites.

The issues I cannot fathom - is how to elegantly alter the response code of the error page. If we have a database issue - I need a 504 - such that my upstream proxy serves an interim cached site page - any thoughts around this? Previously, we could achieve that using a preprocess hook - but as we're bypassing the drupal ecosystem and going direct via twig - I can't see how to alter the response code.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

tonytheferg’s picture

Can't get 135 to apply on 10.1.1 with composer.

jrb’s picture

Here's the patch in #135 re-rolled for 10.1.

tonytheferg’s picture

142 applies, but it's not picking up my maintenance-page--offline.html.twig file.

tonytheferg’s picture

Ahh... I was missing this step:

Edit settings.php, uncomment the line $settings['maintenance_theme'] = 'bartik';, and change the value from bartik to the machine name of the theme you chose in step one above.

hmdnawaz’s picture

Re-rolled the patch for 10.2