Problem

  • system.performance contains performance related configuration settings already.
  • When system.fast_404 is needed, system.performance is needed, too.

Proposed solution

  1. Merge system.fast_404 into system.performance.
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott’s picture

FileSize
4.63 KB

Patch merges the configs and adds an upgrade path test.

Actually using update_variables_to_config for these variable is interesting as in D7 they are declared in settings.php and D8 they can be in config... but must read original conversion issue to see if this was discussed :) #1778478: Convert fast_404 to CMI - doesn't appear so... but this issue should be for a follow up. Relatedly, I'm not sure why we still have the comments in defaults.settings.php (imho) this settings should just be added to the system performance ui.

alexpott’s picture

Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, 1934716.fast_404.1.patch, failed testing.

alexpott’s picture

+++ b/core/lib/Drupal/Core/ExceptionController.phpundefined
@@ -138,7 +138,7 @@ public function on404Html(FlattenException $exception, Request $request) {
     // Check for and return a fast 404 page if configured.
-    $config = config('system.fast_404');
+    $config = config('system.performance')->get('fast_404');
 
     $exclude_paths = $config->get('exclude_paths');

Doh! $config is no longer a config object... this ain't gonna work :)

Also forgot to update default.settings.php

alexpott’s picture

Status: Needs work » Needs review

Why do I always forget this :(

sun’s picture

Status: Needs review » Reviewed & tested by the community

Yay, awesome, thanks! :)

webchick’s picture

Status: Reviewed & tested by the community » Needs work
$ git apply --index 1934716.fast_404.4.patch 
error: patch failed: core/modules/system/tests/upgrade/drupal-7.system.database.php:119
error: core/modules/system/tests/upgrade/drupal-7.system.database.php: patch does not apply

Sorry, this is not applying for me.

alexpott’s picture

Status: Needs work » Needs review
FileSize
7.79 KB
1.58 KB

Needed reroll due to #1821420: Convert mail variables to cmi making unrelated whitespace changes to drupal-7.system.database.php

When green this should be back to rtbc

sun’s picture

Status: Needs review » Reviewed & tested by the community

In the hope it comes back green.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

Status: Fixed » Closed (fixed)

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

hackwater’s picture

Status: Closed (fixed) » Needs review
FileSize
1.2 KB

Patch in #8 had a typo in the default.settings.php file that affects all generated settings.php files; if the fast_404 option is enabled, the missing end quote in 'system.performance' leads to a PHP syntax error.

Status: Needs review » Needs work
Issue tags: -Configuration system, -Sunrise Sanity Cruise

The last submitted patch, 1934716-fast-404-default-settings-typo.patch, failed testing.

hackwater’s picture

Status: Needs work » Needs review
Issue tags: +Configuration system, +Sunrise Sanity Cruise
LinL’s picture

Status: Needs review » Reviewed & tested by the community

Looks good.

LinL’s picture

Title: Merge system.fast_404 config object into system.performance » [Followup] Merge system.fast_404 config object into system.performance
alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Nice catch... and oops :)

Committed c750885 and pushed to 8.x. Thanks!

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