Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
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.
Comment | File | Size | Author |
---|---|---|---|
#35 | 1778478-cmi_fast_404-35.patch | 5.59 KB | ACF |
#31 | 1778478-cmi_fast_404-31.patch | 5.59 KB | Cameron Tod |
#29 | 1778478-cmi_fast_404-29.patch | 5.06 KB | Cameron Tod |
#26 | 1778478-fast-404-26.patch | 6.78 KB | japicoder |
#24 | 1778478-fast-404-24.patch | 6.5 KB | dstol |
Comments
Comment #1
dstolPatch attached and setting status.
Comment #2
rbayliss CreditAttribution: rbayliss commentedNice!
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.
Comment #3
dstolHad the same thought myself.
Comment #4
rbayliss CreditAttribution: rbayliss commented"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."
Comment #5
dstol"Remove the leading hash signs to enable" is the same language used throughout default.settings.php.
Comment #6
dagmartagging
Comment #7
dstol[Edit: whoops network hiccup]
Comment #8
dstolUpdating status per my comment in #5.
Comment #9
gddSeems like 'exclude_paths' would be a better name for this config key.
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.
Comment #10
dstolThanks, updated.
Comment #12
dstolComment #13
gddThanks for the help!
Comment #14
webchickThis needs an upgrade path too, if I'm not mistaken.
Comment #15
Albert Volkman CreditAttribution: Albert Volkman commentedUpgrade path added.
Comment #16
dstolSupposed to be exclude_paths per heyrocker in #9, fixed in attached..
Comment #18
dstolCatching up against 8.x
Comment #19
dstoland status update.
Comment #20
aspilicious CreditAttribution: aspilicious commentedIs 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)
Comment #21
gddHmm 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.
Comment #22
dstolThere 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'].Comment #23
dstolOh hell, forgot the patch.
Comment #24
dstolLong day, epic fail in #23.
Comment #25
japicoder CreditAttribution: japicoder commentedComment #26
japicoder CreditAttribution: japicoder commentedIf 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
Comment #27
aspilicious CreditAttribution: aspilicious commentedIs 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.
Comment #28
Cameron Tod CreditAttribution: Cameron Tod commentedThis now needs to be updated due to the changes made in #1831364: Inline drupal_fast_404() function.
Comment #29
Cameron Tod CreditAttribution: Cameron Tod commentedRebased 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.
Comment #30
aspilicious CreditAttribution: aspilicious commentedForgot the .yml file.
Comment #31
Cameron Tod CreditAttribution: Cameron Tod commentedWoops, here we go.
Comment #33
ACF CreditAttribution: ACF commented#31: 1778478-cmi_fast_404-31.patch queued for re-testing.
Comment #35
ACF CreditAttribution: ACF commentedupdated the update number.
Comment #36
japicoder CreditAttribution: japicoder commentedLooks RTBC for me
Comment #37
penyaskitoThis comment should be available elsewhere (maybe system.fast_404.yml?) or stay at settings.php. I don't think we want to remove it.
Comment #38
Cameron Tod CreditAttribution: Cameron Tod commentedFast 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.
Comment #39
penyaskitoRTBCing then?
Comment #40
Dries CreditAttribution: Dries commentedCommitted to 8.x. Thanks.
Comment #41.0
(not verified) CreditAttribution: commentedUpdated issue summary.