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.
Files: 
CommentFileSizeAuthor
#12 1934716-fast-404-default-settings-typo.patch1.2 KBhackwater
PASSED: [[SimpleTest]]: [MySQL] 56,199 pass(es).
[ View ]
#8 4-8-interdiff.txt1.58 KBalexpott
#8 1934716.fast_404.8.patch7.79 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 53,112 pass(es).
[ View ]
#4 1-4-interdiff.txt3.65 KBalexpott
#4 1934716.fast_404.4.patch7.69 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 52,599 pass(es).
[ View ]
#1 1934716.fast_404.1.patch4.63 KBalexpott
FAILED: [[SimpleTest]]: [MySQL] 52,492 pass(es), 52 fail(s), and 3 exception(s).
[ View ]

Comments

alexpott’s picture

StatusFileSize
new4.63 KB
FAILED: [[SimpleTest]]: [MySQL] 52,492 pass(es), 52 fail(s), and 3 exception(s).
[ View ]

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

StatusFileSize
new7.69 KB
PASSED: [[SimpleTest]]: [MySQL] 52,599 pass(es).
[ View ]
new3.65 KB
+++ 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
StatusFileSize
new7.79 KB
PASSED: [[SimpleTest]]: [MySQL] 53,112 pass(es).
[ View ]
new1.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
StatusFileSize
new1.2 KB
PASSED: [[SimpleTest]]: [MySQL] 56,199 pass(es).
[ View ]

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.