I thought I'd take a stab at it since I saw #1775842: [meta] Convert all variables to state and/or config systems. I made some assumptions based on #1716920: $conf override example documentation needs updating. I took some liberties with naming things, which I will happily correct if given a little guidance.

Files: 
CommentFileSizeAuthor
#35 1778478-cmi_fast_404-35.patch5.59 KBACF
PASSED: [[SimpleTest]]: [MySQL] 49,351 pass(es).
[ View ]
#31 1778478-cmi_fast_404-31.patch5.59 KBcam8001
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/system/system.install.
[ View ]
#29 1778478-cmi_fast_404-29.patch5.06 KBcam8001
FAILED: [[SimpleTest]]: [MySQL] 48,756 pass(es), 17 fail(s), and 48 exception(s).
[ View ]
#26 1778478-fast-404-26.patch6.78 KBjapicoder
PASSED: [[SimpleTest]]: [MySQL] 46,472 pass(es).
[ View ]
#24 1778478-fast-404-24.patch6.5 KBdstol
PASSED: [[SimpleTest]]: [MySQL] 46,416 pass(es).
[ View ]
#23 1778478-fast-404-23.patch6.5 KBdstol
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1778478-fast-404-23.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#18 1778478-fast-404-18.patch3.65 KBdstol
PASSED: [[SimpleTest]]: [MySQL] 42,819 pass(es).
[ View ]
#16 fast_404_cmi-1778478-16.patch3.56 KBdstol
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch fast_404_cmi-1778478-16.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#15 fast_404_cmi-1778478-15.patch3.64 KBAlbert Volkman
PASSED: [[SimpleTest]]: [MySQL] 42,258 pass(es).
[ View ]
#15 interdiff.txt1.18 KBAlbert Volkman
#12 1778478-fast-404-11.patch2.97 KBdstol
PASSED: [[SimpleTest]]: [MySQL] 42,116 pass(es).
[ View ]
#10 1778478-fast-404-10.patch3.05 KBdstol
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1778478-fast-404-10.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#3 1778478-fast-404-3.patch3.01 KBdstol
PASSED: [[SimpleTest]]: [MySQL] 41,237 pass(es).
[ View ]
#1 1778478-fast-404-1.patch2.82 KBdstol
PASSED: [[SimpleTest]]: [MySQL] 41,240 pass(es).
[ View ]

Comments

dstol’s picture

Status:Active» Needs review
StatusFileSize
new2.82 KB
PASSED: [[SimpleTest]]: [MySQL] 41,240 pass(es).
[ View ]

Patch attached and setting status.

rbayliss’s picture

Status:Needs review» Needs work

Nice!

  *
  * Add leading hash signs if you would like to disable this functionality.
  */
-$conf['404_fast_paths_exclude'] = '/\/(?:styles)\//';
-$conf['404_fast_paths'] = '/\.(?:txt|png|gif|jpe?g|css|js|ico|swf|flv|cgi|bat|pl|dll|exe|asp)$/i';
-$conf['404_fast_html'] = '<!DOCTYPE html><head><title>404 Not Found</title></head><body><h1>Not Found</h1><p>The requested URL "@path" was not found on this server.</p></body></html>';
+$conf['system.fast_404']['paths_exclude'] = '/\/(?:styles)\//';
+$conf['system.fast_404']['paths'] = '/\.(?:txt|png|gif|jpe?g|css|js|ico|swf|flv|cgi|bat|pl|dll|exe|asp)$/i';

We can probably comment this out in default.settings.php now that it's stored in the config system, right? We'd just need to change the language around how to modify it.

19 days to next Drupal core point release.

dstol’s picture

Status:Needs work» Needs review
StatusFileSize
new3.01 KB
PASSED: [[SimpleTest]]: [MySQL] 41,237 pass(es).
[ View ]

Had the same thought myself.

rbayliss’s picture

Status:Needs review» Needs work

"Remove the leading hash signs to enable" is a bit misleading, since it's already on and these are the default settings. Maybe you could say something like: "Remove the leading hash signs to make changes to these defaults."

dstol’s picture

"Remove the leading hash signs to enable" is the same language used throughout default.settings.php.

dagmar’s picture

Issue tags:+Configuration system

tagging

dstol’s picture

Status:Needs review» Needs work

[Edit: whoops network hiccup]

dstol’s picture

Status:Needs work» Needs review

Updating status per my comment in #5.

heyrocker’s picture

+++ b/core/includes/bootstrap.incundefined
@@ -2587,12 +2587,14 @@ function drupal_maintenance_theme() {
+  $exclude_paths = $config->get('paths_exclude');

Seems like 'exclude_paths' would be a better name for this config key.

+++ b/sites/default/default.settings.phpundefined
@@ -509,11 +509,11 @@ ini_set('session.cookie_lifetime', 2000000);
- * Add leading hash signs if you would like to disable this functionality.
+ * Remove the leading hash signs to enable.

I don't have a problem with 'Remove the leading hash signs' but I do like the more extended comment in the old version. So I'd prefer 'Remove the leading hash signs if you would like to enable this functionality.'

Otherwise this seems pretty straightforward. It is probably a slight performance ping but I highly doubt its enough to be worth worrying about.

dstol’s picture

Status:Needs work» Needs review
StatusFileSize
new3.05 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1778478-fast-404-10.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Thanks, updated.

Status:Needs review» Needs work

The last submitted patch, 1778478-fast-404-10.patch, failed testing.

dstol’s picture

Status:Needs work» Needs review
StatusFileSize
new2.97 KB
PASSED: [[SimpleTest]]: [MySQL] 42,116 pass(es).
[ View ]
heyrocker’s picture

Status:Needs review» Reviewed & tested by the community

Thanks for the help!

webchick’s picture

Status:Reviewed & tested by the community» Needs work

This needs an upgrade path too, if I'm not mistaken.

Albert Volkman’s picture

Status:Needs work» Needs review
StatusFileSize
new1.18 KB
new3.64 KB
PASSED: [[SimpleTest]]: [MySQL] 42,258 pass(es).
[ View ]

Upgrade path added.

dstol’s picture

StatusFileSize
new3.56 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch fast_404_cmi-1778478-16.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
+++ b/core/modules/system/system.installundefined
@@ -2188,6 +2188,19 @@ function system_update_8030() {
+    '404_fast_paths_exclude' => 'paths_exclude',

Supposed to be exclude_paths per heyrocker in #9, fixed in attached..

Status:Needs review» Needs work

The last submitted patch, fast_404_cmi-1778478-16.patch, failed testing.

dstol’s picture

StatusFileSize
new3.65 KB
PASSED: [[SimpleTest]]: [MySQL] 42,819 pass(es).
[ View ]

Catching up against 8.x

dstol’s picture

Status:Needs work» Needs review

and status update.

aspilicious’s picture

+++ b/sites/default/default.settings.phpundefined
@@ -511,11 +511,11 @@
- * Add leading hash signs if you would like to disable this functionality.
+ * Remove the leading hash signs if you would like to enable this functionality.

Is it me or is this functionality always enabled now. It seems we need an onf/off switch in the config and this block should say that we can overwrite the settings this way. (and it probably needs the on/off example to)

heyrocker’s picture

Status:Needs review» Needs work
+++ b/core/includes/bootstrap.incundefined
@@ -2590,12 +2590,14 @@ function drupal_maintenance_theme() {
-  $exclude_paths = variable_get('404_fast_paths_exclude', FALSE);

Hmm you're right. It used to be that if '404_fast_paths_exclude' was commented out in $conf, then this line would return FALSE. However we don't really have an equivalent to that anymore since the default config always exists. So I guess we do need to add an enable/disable flag.

dstol’s picture

Status:Needs work» Needs review

There was way more work here in reality. drupal_fast_404() in settings.php was removed, since the config system isn't even available that early in bootstrap. If that's a feature that needs to be maintained, that's probably another ticket. In it's stead, there is now a $conf['system.fast_404']['enabled'].

dstol’s picture

StatusFileSize
new6.5 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1778478-fast-404-23.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Oh hell, forgot the patch.

dstol’s picture

StatusFileSize
new6.5 KB
PASSED: [[SimpleTest]]: [MySQL] 46,416 pass(es).
[ View ]

Long day, epic fail in #23.

japicoder’s picture

Assigned:Unassigned» japicoder
japicoder’s picture

StatusFileSize
new6.78 KB
PASSED: [[SimpleTest]]: [MySQL] 46,472 pass(es).
[ View ]

If I uncomment the line for drupal_fast_404() call at settings.php, I get an WSOF, caused because it doesn't know the function "config" at that point.

I submit a patch for it

aspilicious’s picture

Status:Needs review» Needs work
+++ b/core/includes/bootstrap.incundefined
@@ -2593,21 +2593,27 @@ function drupal_maintenance_theme() {
+  // At this stage of bootstraping we don't have still the config functions
+  // or the classloader, so let's help this function.
+  drupal_classloader();
+
+  // Load the procedural configuration system helper functions.
+  require_once DRUPAL_ROOT . '/core/includes/config.inc';

Is this still needed? I don't want to introduce these kinda changes here :s.
And this needs a reroll for the increased system update functions.

cam8001’s picture

This now needs to be updated due to the changes made in #1831364: Inline drupal_fast_404() function.

cam8001’s picture

Status:Needs work» Needs review
StatusFileSize
new5.06 KB
FAILED: [[SimpleTest]]: [MySQL] 48,756 pass(es), 17 fail(s), and 48 exception(s).
[ View ]

Rebased and updated the fast_404 stuff to work with the Drupal\Core\ExceptionController added in #1831364: Inline drupal_fast_404() function. Are there tests for fast_404? I couldn't see anything in Drupal\system\Tests\System\PageNotFoundTest.

aspilicious’s picture

Status:Needs review» Needs work

Forgot the .yml file.

cam8001’s picture

Status:Needs work» Needs review
StatusFileSize
new5.59 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/system/system.install.
[ View ]

Woops, here we go.

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

The last submitted patch, 1778478-cmi_fast_404-31.patch, failed testing.

ACF’s picture

Status:Needs work» Needs review

#31: 1778478-cmi_fast_404-31.patch queued for re-testing.

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

The last submitted patch, 1778478-cmi_fast_404-31.patch, failed testing.

ACF’s picture

Status:Needs work» Needs review
StatusFileSize
new5.59 KB
PASSED: [[SimpleTest]]: [MySQL] 49,351 pass(es).
[ View ]

updated the update number.

japicoder’s picture

Looks RTBC for me

penyaskito’s picture

Status:Needs review» Needs work
+++ b/sites/default/default.settings.phpundefined
@@ -484,37 +484,19 @@
-/**
- * By default the page request process will return a fast 404 page for missing
- * files if they match the regular expression set in '404_fast_paths' and not
- * '404_fast_paths_exclude' above. 404 errors will simultaneously be logged in
- * the Drupal system log.
- *
- * You can choose to return a fast 404 page earlier for missing pages (as soon
- * as settings.php is loaded) by uncommenting the line below. This speeds up
- * server response time when loading 404 error pages and prevents the 404 error
- * from being logged in the Drupal system log. In order to prevent valid pages
- * such as image styles and other generated content that may match the
- * '404_fast_html' regular expression from returning 404 errors, it is necessary
- * to add them to the '404_fast_paths_exclude' regular expression above. Make
- * sure that you understand the effects of this feature before uncommenting the
- * line below.
- */
-# drupal_fast_404();

This comment should be available elsewhere (maybe system.fast_404.yml?) or stay at settings.php. I don't think we want to remove it.

cam8001’s picture

Fast 404s are handled by Drupal/Core/ExceptionContoller.php now, and are enabled by default, so the stuff around commenting/uncommenting that function doesn't make sense for D8.

The other comments around configuring Fast 404s are already covered off in the comment block above, so I don't think we need that one anymore.

penyaskito’s picture

Status:Needs work» Reviewed & tested by the community

RTBCing then?

Dries’s picture

Status:Reviewed & tested by the community» Fixed

Committed to 8.x. Thanks.

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

Anonymous’s picture

Issue summary:View changes

Updated issue summary.