Fast 404s are slower than regular 404s

that means an extra config call on every request, no? should we convert fast 404 to settings?

Did you profile the difference?

I'm a bit confused by this. regular 404 also happens as part of 404 exception handling, as long as this is first, generating a fixed predefined response string should be faster than for example rendering a node? Maybe you tested with the default 404 controller, which is probably faster, but I'd expect it to be slower if you have a 404 node or so, which is pretty common to have a custom and nice page with content on it.

I also don't understand how this would work as a middleware. we _need_ routing for fast 404 extensions, for private files, generating image styles and so on, as the failing tests are showing?

I can confirm that regular 404 is faster, but that's because that's a cached page. The correct fix here is to return a cacheable response I think. Of course, that does mean we need to consider cache invalidation here, but the default 404 stuff *probably* has that covered. To test, we need to access a certain private file path, then create said file, then access again.

If you disable page cache, you get a very different picture, blackfire numbers, so a bit slower overall:

cached 404: 4ms
fast 404: 10ms
non-cached 404: 20ms

so fast 404 is twice as fast, you only get the cache benefit in case of repeated requests on the same URL.

should we convert fast 404 to settings?

Yes we should have done that when we introduced $settings...

I also don't understand how this would work as a middleware. we _need_ routing for fast 404 extensions, for private files, generating image styles and so on, as the failing tests are showing?

# $config['system.performance']['fast_404']['exclude_paths'] = '/\/(?:styles)|(?:system\/files)\//';

The idea is that 'exclude paths' don't get fast_404s, so that image styles, private files (and eventually asset aggregates if that patch ever lands) are ignored. Maybe we should consider converting this particular setting to container parameters so that the relevant subsystems can add entries themselves. From the test failures it seems that logic isn't working, but conceptually it should work and it used to in 7.x

Worth referencing the original issue where this was introduced: #76824: Drupal should not handle 404 for certain files.

Yes we should have done that when we introduced $settings...

#1833516: Add a new top-level global for settings.php - move things out of $conf two choice quotes

The goal of this issue is to get the new concept/helper function in, including a few example conversions, to see how it works, we can't convert everything yet, it's way too much.

Every single thing that is currently $conf in settings.php should either be moved to $settings or deleted from the file.

Aside from two examples for the Configuration overrides, this issue is the only one left in settings.php which hasn't been converted.

no routing, no queries for fast 404s as the config is now stored on the container.

This is good but looking at the patch I'm wondering if we'd be better off moving it to $settings.

I guess there is a question about whether this is worth it.

If we always set a cacheable header regardless of page cache settings maybe that'd be a good trade-off to simplify things a bit.

I also wonder whether all this complexity is worth it. If you want even faster 404s that match a regex, you can do it directly in the webserver configuration so the requests never even reach PHP.

+++ b/core/lib/Drupal/Core/EventSubscriber/Fast404ExceptionHtmlSubscriber.php
@@ -74,10 +78,32 @@ public function on404(ExceptionEvent $event) {
+    if ($saved_config->getName() == 'system.performance' && $event->isChanged('fast_404')) {

In practice what is going to trigger this if there is no UI?

I also think that with the patch attached here we can remove the system.performance:fast_404.exclude_paths setting.

I think we could do that anyway - untested but you should be able to use negative lookahead in the paths setting.

The fix is nice and clean and the test looks comprehensive.

+++ b/core/lib/Drupal/Core/EventSubscriber/Fast404ExceptionHtmlSubscriber.php
@@ -74,10 +78,32 @@ public function on404(ExceptionEvent $event) {
+      Cache::invalidateTags(['4xx-response']);

Worth injecting cache_tags.invalidator instead? If you think not, then this is RTBC.

Looks good to me.

Do we also need to do a deprecation dance in the constructor?

Looks great. I love the irony of this issue! ;) Nice job, everyone.

I can only spot a few pedantic nits. Please ignore, if you prefer.

1. +++ b/core/lib/Drupal/Core/EventSubscriber/Fast404ExceptionHtmlSubscriber.php
   @@ -2,11 +2,14 @@
   +use Drupal\Core\Cache\CacheTagsInvalidatorInterface;
   +use Drupal\Core\Config\ConfigCrudEvent;
   +use Drupal\Core\Config\ConfigEvents;
    use Drupal\Core\Config\ConfigFactoryInterface;
    use Drupal\Component\Utility\Html;
   +use Drupal\Core\Render\HtmlResponse;
   

   Nit: These aren't exactly in order now. /shrug

2. +++ b/core/lib/Drupal/Core/EventSubscriber/Fast404ExceptionHtmlSubscriber.php
   @@ -74,10 +77,32 @@ public function on404(ExceptionEvent $event) {
   +    if ($saved_config->getName() == 'system.performance' && $event->isChanged('fast_404')) {
   

   Nit: ===

3. +++ b/core/tests/Drupal/FunctionalTests/EventSubscriber/Fast404Test.php
   @@ -0,0 +1,72 @@
   +  public function testFast404() {
   

   Nit: : void?

+++ b/core/tests/Drupal/FunctionalTests/EventSubscriber/Fast404Test.php
@@ -0,0 +1,72 @@
+    $this->assertSession()->statusCodeEquals(404);

Another minor nit, do we not want to save $this->assertSession() to a local variable to save setting it up on every line?

Also might be nice to abstract $this->assertSession()->responseNotContains('modules/system/css/'); into a function like assertNotFast404 or something similar to make it easier to improve/refactor in future?

This patch fixes all nits from #25. I didn't touch core.services.yml so I'm not sure why it's appearing in the interdiff.

Re: #26.1: Core isn't consistent about this and there's no standard. Many tests do it either way. If anything, it looks like not having a local var is the more common approach. So I didn't want to change that here.

Re: #26.2: Thing is, we do both responseContains() and responseNotContains() in this test, so we'd need 2 new custom assert methods (or 1 method that takes a param). In some ways, the custom assert method makes it harder to know what the test is doing. I'm a little worried about YAGNI for 2 1-line functions that are only invoked a handful of times for the sake of "easier to improve/refactor in future?".

So for now, no changes for either suggestion in #26, but I'd be happy to do either of them if a core committer prefers...

Thanks!
-Derek

Changes in #27 look great, they fix the remakrs in #25, and I agree that the refactor in #26 is not yet needed.

Back to rtbc.

@alexpott not class props, just a variable, i.e $assert = $this->assertSession()

+    // Changing settings using an override means we need to rebuild everything.
+    $this->rebuildAll();

what does this mean for a settings.php edit, will drush cr pick up the change or an explicit call to the tag invalidator is needed? If the latter, we might want to add a note to settings.php to do drush php '\Drupal::service("cache_tags.invalidator")->invalidateTags(["4xx-response"]);' after editing.

+++ b/core/lib/Drupal/Core/EventSubscriber/Fast404ExceptionHtmlSubscriber.php
@@ -74,10 +77,32 @@ public function on404(ExceptionEvent $event) {
+   *   The configuration event.
+   */
+  public function onConfigSave(ConfigCrudEvent $event) {
+    $saved_config = $event->getConfig();
+    if ($saved_config->getName() === 'system.performance' && $event->isChanged('fast_404')) {
+      $this->cacheTagsInvalidator->invalidateTags(['4xx-response']);
+    }
+  }

The reason we originally added settings.php configuration for this was because it could break actual Drupal 404 errors due to running before routing.

If we're letting routing happen, then do we need the configuration at all?

There are some very specific use cases for not showing a minimal 404 for files (like an image hosting site), but for those there's the option of removing the event subscriber altogether.

Could be done in a follow-up but feels worth looking at.

If we leave this in config, I think we should take the lines out of default.settings.php and put some comment documentation in system.performance.yml instead. Or actually move it to $settings altogether.

I think we should still take the $config lines out of settings.php in this issue - it's not doing anything useful, and to the extent that it would, the config listeners here won't work.

+++ b/core/lib/Drupal/Core/EventSubscriber/Fast404ExceptionHtmlSubscriber.php
@@ -74,10 +90,32 @@ public function on404(ExceptionEvent $event) {
+  public function onConfigSave(ConfigCrudEvent $event) {

Add a void return type? Otherwise this looks good to go, all review points have been addressed.

Thanks - I think it is easiest to add return type everywhere for now, otherwise we have to come up with a set of explicit rules.

Committed/pushed to 10.1.x, cherry-picked to 10.0.x and 9.5.x

I published CR but it doesn't mention that this settings no longer in settings.php file

Looks like this introduced a critical regression: #3312198: Regression concerning the cache of private files.

If anyone else is using the Search 404 module, I think this change should be considered over there. If it's no more recommended to use fast_404 in settings.php it might mean that by removing these settings, all those 404's are handled by Search 404, resulting in many database queries for the missing files.

That might be a heavy performance trap? See #3360782: Exclude fast_404 paths and file extensions by default (Performance) and same for Redirect 404: #3360799: Redirect 404: Exclude file paths by default (Performance)