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 swear I read it a few time and it's a pile of words that make no sense. The trick is that the documentation is actually the above section but this is utterly not trivial. Also " happen to match the pattern" is a very inadequate description because it needs to match the 404_fast_paths pattern and not match the 404_fast_paths_exclude pattern...
Comment | File | Size | Author |
---|---|---|---|
#8 | fast404-documentation-1553704-8.patch | 2.02 KB | eddie_c |
#6 | fast404-documentation-1553704-6.patch | 1.98 KB | eddie_c |
Comments
Comment #1
chx CreditAttribution: chx commentedI was talking about settings.php but the doxygen is really nice as well... I am too afraid to check git blame, did I do this :) ?
Comment #2
chx CreditAttribution: chx commentedI think given how short the function is, this might be a novice fix.
Comment #3
krishworks CreditAttribution: krishworks commentedwhich one are you referring to? The one in settings.php is
or the doc written for the drupal_fast_404 function as below?
Either 1 or 2, I think the doc is sufficiently good to understand what it does.
Comment #4
chx CreditAttribution: chx commentedThis.
Comment #5
krishworks CreditAttribution: krishworks commentedthanks chi for the text. It indeed is confusing. Cross linking a relevant discussion here: http://drupal.org/node/76824
Comment #6
eddie_c CreditAttribution: eddie_c commentedThis is hopefully an improvement. I'd really appreciate it if someone could review it.
Comment #7
jhodgdonThis looks like an improvement to me, thanks!
My only gripe is a small grammatical nitpick:
Could we change "ensure" to "make sure"? I don't think ensure is right here, and if it is it probably needs "that" after it.
Also, instead of "regex" can we spell out "regular expression"?
Comment #8
eddie_c CreditAttribution: eddie_c commentedThanks jhodgdon - thorough as always! I'd never thought about the difference between "ensure" and "make sure" until now. I've also removed the word 'pattern' after regular expression as I think this word is redundant, right?
Comment #9
jhodgdonThat looks good to me! I'll let it sit at RTBC for a day or two before committing so that the others following this issue have a chance to respond as to whether this is satisfactory.
Comment #10
xjmLooks great to me!
Comment #11
chx CreditAttribution: chx commentedWhile I love the new version -- thank you for fixing this -- I compared it to the old one and found one thing missing: " It will also prevent the Drupal system log entry". Can we add something about this -- of course reworded :) ? I think what this is about that on the admin // reports // page not found these won't get listed.
Comment #12
eddie_c CreditAttribution: eddie_c commentedThanks for the reviews everyone. chx, I did mention that returning the fast 404 early "prevents the 404 error from being logged in the Drupal system log". Do you think I need to expand on this?
Comment #13
eddie_c CreditAttribution: eddie_c commentedComment #14
jhodgdonRE #11/#12 - I agree with eddie_c that the wording is there and is sufficient. It says:
... This speeds up server response time when loading 404 error pages and prevents the 404 error from being logged in the Drupal system log. ...
I think that's sufficient... Let's set this back to RTBC again and let it sit for a few days.
Comment #15
webchickPassing to Jennifer for commit.
Comment #16
jhodgdonThanks! No objections after a few days... I committed the patch above to d7 and d8.