Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jhodgdon’s picture

Version: 7.x-dev » 8.x-dev
Status: Active » Postponed (maintainer needs more info)
Issue tags: +Needs backport to D6, +Needs backport to D7

So what is incorrect there, and how do you think it should be changed? I don't see a problem with the current docs.

perusio’s picture

IMO it should read:

307: Temporary redirect with HTTP method constraining, i.e., the request that follows to the new location must preserve the method. This differs from the 302 status code, where there's no constraining of the HTTP method in follow up to the initial request. If a request was a GET in the original request, it can be a POST when following the redirect. With a 307 if initially a GET, it must necessarily be a GET, for example.

It's incorrect in the sense that is unclear and confusing. It doesn't state clearly the difference between a 302 and a 307 IMO.

jhodgdon’s picture

Status: Postponed (maintainer needs more info) » Active

Perhaps a better idea would be to stick with brief descriptions, and link to more complete information? This is web-wide standards, not Drupal-specific, and we don't need to document what all the status codes mean in detail.

perusio’s picture

Perhaps a better idea would be to stick with brief descriptions, and link to more complete information? This is web-wide standards, not Drupal-specific, and we don't need to document what all the status codes mean in detail.

Yes I agree, it should be concise and clear and not a mere filler. Here's my 2nd attempt at a better code documentation:

307: Temporary redirect that preserves the HTTP method (differs from 302 @see http://tools.ietf.org/html/draft-reschke-http-status-308-07#page-3).

How's that?

jhodgdon’s picture

What we need to do is this. In the first line of the @param, where it says:

Valid values for an actual "goto" as per RFC 2616 section 10.3 are:

This needs to be turned into a link to the real specs.

And then I think we can probably leave out the specific link that you suggested for the 307. I think we all know that the codes are different (otherwise, why would they have different numbers for them?), so I don't think we need to say "differs from 302" -- just a short statement about what each code does.

perusio’s picture

If that's so then we must also remove the confusing:

(alternative to "503 Site Down for Maintenance").

jhodgdon’s picture

I totally agree with #6.

perusio’s picture

How do we proceed? With a patch?

jhodgdon’s picture

Yes, a patch would be the next step. Thanks!

perusio’s picture

Can I get by with a simple git clone --depth 1 <repo> of the D6, D7 and D8 trees? I think it suffices. And then change the code and post the patch here.

Thanks,

jhodgdon’s picture

For information on how to use git & make a patch, start at
http://drupal.org/novice

perusio’s picture

I know how to make a patch, my question has to do if I need to get all the repo history or if I can get by with just HEAD. I don't think it's relevant, because this is not a pull request after all, I'll just post the patch here.

perusio’s picture

Here's the patch for 8.x. If ok I'll proceed for 7.x and 6.x.

perusio’s picture

Status: Active » Needs review

Changing status. Not sure if the closing remark:

* Note: Other values are defined by RFC 2616, but are rarely used and poorly
* supported.

Shouldn't be removed also. Seems confusing to me.

jhodgdon’s picture

Status: Needs review » Needs work

Thanks, good start!

There are a few problems with this patch that need to be addressed:

a) After @param, the parameter documentation needs to be indented two spaces, and it all needs to stay in one paragraph (no blank lines).

b) Don't use @see within the param docs. @see gets displayed on api.drupal.org in a separate See Also section at the bottom of the page. Instead, use @link -- see http://drupal.org/node/1354#links -- to turn "RFC 2616" and "draft for the new HTTP status codes" into links. Note that
@link ... @endlink
all needs to fit on one line, and if that would make the line longer than 80 characters, put it on its own line.

c) The line before the start of a list should end in :

d) Why is 308 code added to this patch? I don't think we need that in there.

e) I agree the note on other values can be removed.

perusio’s picture

Status: Needs work » Needs review
FileSize
1.17 KB

New patch.

jhodgdon’s picture

Status: Needs review » Needs work

Better, but still not quite right:

  * @param $http_response_code
- *   Valid values for an actual "goto" as per RFC 2616 section 10.3 are:
+ *   The valid values are defined in RFC 2616 and the draft for the new HTTP 
+ *   status codes:
+ *
+ *   @link http://www.w3.org/Protocols/rfc2616/rfc2616-sec10.html#sec10.3 @endlink
+ *   and 
+ *   @link http://tools.ietf.org/html/draft-reschke-http-status-308-07. @endlink
+ *
  *   - 301 Moved Permanently (the recommended value for most redirects)
...

a) No blank lines should be inside this parameter definition *at all*.

b) Read http://drupal.org/node/1354#links and please turn this text:

The valid values are defined in RFC 2616 and the draft for the new HTTP status codes:

into text with two @link...@endlink statements *embedded*, where the text "RFC 2616" is one link text, and the text "draft for the new HTTP status codes:" is the other link text.

perusio’s picture

Here's another shot. Posted a question on #drupal-docs. No reply :(

jhodgdon’s picture

Sorry if no one was in #drupal-docs. The #drupal-contribute channel is more active and is appropriate for questions concerning patches. :)

This is closer, but still not quite right:

a)

+ *   The valid values are defined in RFC 2616 and the draft for the new HTTP 
+ *   status codes:
+ *   @link http://www.w3.org/Protocols/rfc2616/rfc2616-sec10.html#sec10.3 RFC2616 @endlink

Again, please make RFC 2616 in the first line *into* the link, rather than having a separate link.

b)

+ *   and the @link http://tools.ietf.org/html/draft-reschke-http-status-308-07 
+ *   draft for the new HTTP status codes @endlink.
+ *

Again, *everything* between @link and @endlink needs to be on one line, even if this goes past 80 characters. Also, again, remove this blank line at the end. There should not be any blank lines within the @param section.

c) One thing I didn't mention before:

 *   - 301 Moved Permanently (the recommended value for most redirects)
  *   - 302 Found (default in Drupal and PHP, sometimes used for spamming search
  *         engines)
  *   - 303 See Other
  *   - 304 Not Modified
  *   - 305 Use Proxy
- *   - 307 Temporary Redirect (alternative to "503 Site Down for Maintenance")
- *   Note: Other values are defined by RFC 2616, but are rarely used and poorly
- *   supported.
+ *   - 307 Temporary Redirect

This whole list should use our list formatting syntax -- should be a : after each integer value. See
http://drupal.org/node/1354#lists

perusio’s picture

Thanks for your help. Here it is.

jhodgdon’s picture

Much better! This is 95% there. Still to fix:

a) Move long @link lines so that only @link ... @endlink is on the line. We try as much as possible to not go over 80 characters, so if the @link...@endlink (with the * ) is over 80 by itself, it needs to go on its own line.

b) List indenting:

+ *   - 302: Found (default in Drupal and PHP, sometimes used for spamming search
+ *          engines)

the second line here should only be indented 2 spaces, see http://drupal.org/node/1354#lists

c) List formatting: each list item should end with . (see http://drupal.org/node/1354#lists)

d) I just noticed that the text now says "The valid values...:" and then lists only about 5 values. Obviously, many more than 5 valid values exist for HTTP status codes.... So maybe we should (after the second @endlink) say something like: "such as" or "including"?

perusio’s picture

They're the valid values for Redirection codes, i.e., codes of the form 3xx. It needs clarification.

jhodgdon’s picture

OK. :)

perusio’s picture

jhodgdon’s picture

When you upload a patch, set the status to "needs review" so that both human and testbot followers of the issue can know there is a patch to review. :)

This latest patch is 99% there! To fix:

a) Indentation of subsequent lines of list items should be 2 spaces. Yours is 4:

+ *   - 302: Found (default in Drupal and PHP, sometimes used for spamming search
+ *       engines).

b) redirections -> redirection in this line:

+ *   The valid values for 3xx redirections status codes are defined in 

c) In other places, we've put the : into the link text (before the @endlink), and I think we should do that here (I'm not sure it will parse properly by the API module after the @endlink):

+ *   @link http://tools.ietf.org/html/draft-reschke-http-status-308-07 draft for the new HTTP status codes @endlink:

In other words, the end of that line should say codes: @endlink

perusio’s picture

I think it's this the one.

perusio’s picture

Status: Needs work » Needs review

Changing status for testbot.

Status: Needs review » Needs work

The last submitted patch, drupal-drupal_goto-documentation-1536750-26.patch, failed testing.

perusio’s picture

Status: Needs work » Needs review
FileSize
1.43 KB

Re-rolling.

Status: Needs review » Needs work

The last submitted patch, drupal-drupal_goto-documentation-1536750-29.patch, failed testing.

perusio’s picture

Status: Needs work » Needs review
FileSize
1.43 KB

Re-rolling again. Somewhere patching practices changed. Now it includes git prefixes.

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

Excellent -- that patch looks good. I'll get it committed soon. Thanks!

tim.plunkett’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/includes/common.incundefined
@@ -651,16 +651,17 @@ function drupal_encode_path($path) {
+ *   The valid values for 3xx redirection status codes are defined in ¶
+ *   @link http://www.w3.org/Protocols/rfc2616/rfc2616-sec10.html#sec10.3 RFC 2616 @endlink
+ *   and the ¶

The first and third lines here have trailing whitepace

+++ b/core/includes/common.incundefined
@@ -651,16 +651,17 @@ function drupal_encode_path($path) {
+ *   @link http://tools.ietf.org/html/draft-reschke-http-status-308-07 draft for the new HTTP status codes: @endlink

This is over 80 characters, is that allowed because of the @link?

jhodgdon’s picture

Dang, I missed the trailing spaces. Thanks for catching that tim.plunkett!

Regarding the 80 characters, yes: links have to be on one line, so it's OK to exceed 80 characters if they are alone on a line.

perusio’s picture

Status: Needs work » Needs review
FileSize
1.43 KB

Whitespace removed.

perusio’s picture

Question: doesn't this needs to be backported to D6 and D7?

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

Yes, and the issue is already tagged "needs backport..." to indicate this. Please read http://drupal.org/node/1319154#multiple-versions for sequence of events...

Anyway, the new patch is fine (this time the whitespace is correct). Thanks! I'll get it committed to 8.x soon.

jhodgdon’s picture

Version: 8.x-dev » 6.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

The patch in #35 applied to 8.x and 7.x both, so I committed it to both. Thanks! Now needs backport to 6.x.

perusio’s picture

Status: Patch (to be ported) » Needs review
FileSize
1.48 KB

Hello Jennifer,

Here's the first shot at a patch for D6. Pretty much copy & paste from D8.

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

Thanks!

dixon_’s picture

Setting back title after spam attack.
/me goes reporting to the webmasters.

jhodgdon’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Sorry for the long delay in getting this committed... the patch above needs a re-roll for D6 apparently, since it doesn't currently apply.

sandipmkhairnar’s picture

Issue summary: View changes
FileSize
1.65 KB

Re-rolled Patch.

sandipmkhairnar’s picture

Status: Needs work » Needs review
jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs reroll

Thanks!

jhodgdon’s picture

Status: Reviewed & tested by the community » Fixed

Thanks again everyone! Committed to 6.x. Only two years later, this one is done. :)

Status: Fixed » Closed (fixed)

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