Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dstol’s picture

Status: Active » Needs review
FileSize
2.82 KB

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
FileSize
3.01 KB

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.

gdd’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
FileSize
3.05 KB

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
FileSize
2.97 KB
gdd’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
FileSize
1.18 KB
3.64 KB

Upgrade path added.

dstol’s picture

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

FileSize
3.65 KB

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)

gdd’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

FileSize
6.5 KB

Oh hell, forgot the patch.

dstol’s picture

FileSize
6.5 KB

Long day, epic fail in #23.

japicoder’s picture

Assigned: Unassigned » japicoder
japicoder’s picture

FileSize
6.78 KB

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.

Cameron Tod’s picture

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

Cameron Tod’s picture

Status: Needs work » Needs review
FileSize
5.06 KB

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.

Cameron Tod’s picture

Status: Needs work » Needs review
FileSize
5.59 KB

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
FileSize
5.59 KB

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.

Cameron Tod’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.