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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

chx’s picture

Title: drupal_fast_404 documentation makes absolutely no sense » drupal_fast_404 settings.phpAl documentation makes absolutely no sense

I 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 :) ?

chx’s picture

Issue tags: +Novice

I think given how short the function is, this might be a novice fix.

krishworks’s picture

which one are you referring to? The one in settings.php is

/**
* Fast 404 pages:
*
* Drupal can generate fully themed 404 pages. However, some of these responses
* are for images or other resource files that are not displayed to the user.
* This can waste bandwidth, and also generate server load.
*
* The options below return a simple, fast 404 page for URLs matching a
* specific pattern:
* - 404_fast_paths_exclude: A regular expression to match paths to exclude,
* such as images generated by image styles, or dynamically-resized images.
* If you need to add more paths, you can add '|path' to the expression.
* - 404_fast_paths: A regular expression to match paths that should return a
* simple 404 page, rather than the fully themed 404 page. If you don't have
* any aliases ending in htm or html you can add '|s?html?' to the expression.
* - 404_fast_html: The html to return for simple 404 pages.
*
* Add leading hash signs if you would like to disable this functionality.
*/

or the doc written for the drupal_fast_404 function as below?

/**
* Returns a simple 404 Not Found page.
*
* If fast 404 pages are enabled, and this is a matching page then print a
* simple 404 page and exit.
*
* This function is called from drupal_deliver_html_page() at the time when a
* a normal 404 page is generated, but it can also optionally be called directly
* from settings.php to prevent a Drupal bootstrap on these pages. See
* documentation in settings.php for the benefits and drawbacks of using this.
*
* Paths to dynamically-generated content, such as image styles, should also be
* accounted for in this function.
*/ 

Either 1 or 2, I think the doc is sufficiently good to understand what it does.

chx’s picture

Title: drupal_fast_404 settings.phpAl documentation makes absolutely no sense » drupal_fast_404 settings.php documentation makes absolutely no sense
 * By default, fast 404s are returned as part of the normal page request
 * process, which will properly serve valid pages that happen to match and will
 * also log actual 404s to the Drupal log. Alternatively you can choose to
 * return a 404 now by uncommenting the following line. This will reduce server
 * load, but will cause even valid pages that happen to match the pattern to
 * return 404s, rather than the actual page. It will also prevent the Drupal
 * system log entry. Ensure you understand the effects of this before enabling.

This.

krishworks’s picture

thanks chi for the text. It indeed is confusing. Cross linking a relevant discussion here: http://drupal.org/node/76824

eddie_c’s picture

Status: Active » Needs review
FileSize
1.98 KB

This is hopefully an improvement. I'd really appreciate it if someone could review it.

jhodgdon’s picture

Status: Needs review » Needs work

This looks like an improvement to me, thanks!

My only gripe is a small grammatical nitpick:

+ * add them to the '404_fast_paths_exclude' regex above. Ensure you understand
+ * the effects of this feature before uncommenting the line below.

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"?

eddie_c’s picture

Assigned: Unassigned » eddie_c
Status: Needs work » Needs review
FileSize
2.02 KB

Thanks 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?

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

That 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.

xjm’s picture

Looks great to me!

chx’s picture

Status: Reviewed & tested by the community » Needs work

While 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.

eddie_c’s picture

Thanks 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?

eddie_c’s picture

Status: Needs work » Needs review
jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

RE #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.

webchick’s picture

Assigned: eddie_c » jhodgdon

Passing to Jennifer for commit.

jhodgdon’s picture

Assigned: jhodgdon » Unassigned
Status: Reviewed & tested by the community » Fixed

Thanks! No objections after a few days... I committed the patch above to d7 and d8.

Automatically closed -- issue fixed for 2 weeks with no activity.