In D9/10 the fast 404 processor no long runs prior to routing so the concept of excluded paths is unnecessary. If you want proper 404s for image files for example then it is best to disable fast 404s altogether.
Remove the config key and write an update function to do that.
From #3292908-36: Fast 404s are slower than regular 404s: Decide wether to keep this in config or move it to settings. The exclude_paths config is meaningless in the current implementation. So deciding what to do with that and whether all the things should be config or settings feels all intimately related
None
None
system.performance:fast_404.excluded_paths is removed.
@todo
| Comment | File | Size | Author |
|---|
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
compare
Comments
Comment #2
ajv009 commentedComment #3
ajv009 commentedComment #5
kunal_singh commentedModified the test cases.
Comment #6
njim commentedThe change was implemented as described. Imagecache is no longer in the excluded_path attribute of the yaml file. Related tests were also updated. This should have no impact on current sites as it removed references to something that no longer exists. Code review is complete and everything looks good.
Comment #7
alexpottThis will only affect new sites. As we're changing install configuration. I guess there is a question about whether or not we should update existing sites config value if it is still equal to
'/\/(?:styles|imagecache)\//'. It's a bit tricky - could go either way. Thoughts anyone?Comment #8
longwaveNot sure it is worth updating existing config. As fast 404 config is also listed in settings.php I was under the impression that most sites that change fast 404 config do it there rather than using
drush csetor similar, as there is no UI for this feature. Speaking of which: the default in settings.php also includessystem/files, should we make the same change here?This was changed in #2455057: Fix fast 404 settings for private image files
Comment #9
njim commentedMy impression is that this does not have any negative consequences if left within active configuration -- it's simply a relic of the old functionality. But I assume there is always a risk in rolling out a change to active configs on existing sites. So FWIW, I tend to leave these things alone unless I have complete control of the application.
Regarding the fast_404 change in #8, I prefer to create new tickets unless it makes sense to bundle work under a bigger epic.
Comment #11
ajaypratapsingh commented#5 Patch is applied Successfully on Drupal 9.3.x local machine
Comment #13
rinku jacob 13 commentedRe-rolled patch for 9.5.x. Adding screenshots for the reference. Thanks @kunal_singh
Comment #14
longwaveRTBC assuming bot agrees.
Comment #15
alexpottSo imagecache getting there was my fault completely. See #1934716: [Followup] Merge system.fast_404 config object into system.performance which added it. Oops.
I think we should consider removing the override section from settings.php. It's weird that this is there. As far as I know there's no special reason to override this there as opposed to deploy it properly. I also think that we should consider adding system/files to the default exclude_paths if it is not there as this makes sense. I think we should do this here because we're changing the default value and rather than changing it twice - I think we should do that once.
I propose that we re-purpose this issue to: "Provide a better default for 'excluded_paths' system performance.yml and remove duplication in default.settings.php". That way we can get to a better place with respect to fast 404 settings in a single issue.
Comment #16
alexpottActually: re default.settings.php I think this is there because there is no UI so this is one way to configured this. However I'd bet a lot of D8/9 sites just use the default configuration so #2455057: Fix fast 404 settings for private image files should have changed the default config too. Hmmm... how do we prevent this getting out-of-sync in the future. The current state of HEAD is a bit of a mess.
Comment #17
longwaveMaybe we should just build an admin UI for it? I never understood why it didn't have one.
Comment #18
alexpott@longwave because regexes are awful to validate?
Comment #19
rakhi soni commentedKindly review patch for version 9.5x,,
Comment #20
Munavijayalakshmi commentedComment #21
Munavijayalakshmi commentedBoth #13 and # 19 same.
Both patches applied successfully.
Comment #22
alexpottSo this setting is completely pointless in D8/9/10 so we should remove it. Let's wait for #3292908: Fast 404s are slower than regular 404s and re-focus this issue into cleaning up the fast 404 config.
Comment #23
cilefen commented@ajaypratapsingh, @Rinku Jacob 13 and @Munavijayalakshmi: Thank you for helping out. One thing I had been wanting to mention is that screenshots depicting patches applying or screenshots of code are unnecessary, are not contributions as such, and do not move code review forward, as there are automated systems that confirm diffs. There are a few review guides like this one which contain lots of tips on reviewing code. You can make more meaningful contributions by using those techniques.
Comment #24
alexpottUpdated IS to be about what the current issue should do - now that #3292908: Fast 404s are slower than regular 404s has landed.
Comment #25
Munavijayalakshmi commentedComment #26
Munavijayalakshmi commentedNeeds work
Comment #27
eric_a commentedFixing metadata to catch up with the repurposing by @alexpott. See #24.
Comment #28
jwilson3Catch pointed me to this issue, which I was unaware of. I have a kind of a sister issue that takes a different approach for lowercase "faster" 404s from image api here.
I'm not certain if there is overlap, or if my issue is obviated by this one.
#3362929: Add minimal 4xx responses for invalid image derivative requests