API page: http://api.drupal.org/api/drupal/includes%21common.inc/function/drupal_g...

According to the IETF draft on the 308 status code the 307 code differs from the 302 in the sense that the HTTP method must not be changed. I fail to see the relationship with a 503. Can someone elaborate if that's not the case please? Thanks.

The comment is the same on the D6 and D8 versions also.

Files: 
CommentFileSizeAuthor
#50 1536750-comment-about-307code-50.patch1.65 KBsandipmkhairnar
PASSED: [[SimpleTest]]: [MySQL] 190 pass(es).
[ View ]
#40 drupal-drupal_goto-documentation-1536750-39.patch1.48 KBperusio
PASSED: [[SimpleTest]]: [MySQL] 190 pass(es).
[ View ]
#35 drupal-drupal_goto-documentation-1536750-35.patch1.43 KBperusio
PASSED: [[SimpleTest]]: [MySQL] 35,009 pass(es).
[ View ]
#31 drupal-drupal_goto-documentation-1536750-31.patch1.43 KBperusio
PASSED: [[SimpleTest]]: [MySQL] 35,006 pass(es).
[ View ]
#29 drupal-drupal_goto-documentation-1536750-29.patch1.43 KBperusio
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-drupal_goto-documentation-1536750-29.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#26 drupal-drupal_goto-documentation-1536750-26.patch6.75 KBperusio
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-drupal_goto-documentation-1536750-26.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#24 drupal-drupal_goto-documentation-1536750-24.patch1.44 KBperusio
PASSED: [[SimpleTest]]: [MySQL] 35,006 pass(es).
[ View ]
#20 drupal-drupal_goto-documentation-1536750-20.patch1.39 KBperusio
PASSED: [[SimpleTest]]: [MySQL] 34,999 pass(es).
[ View ]
#18 drupal-drupal_goto-documentation-1536750-18.patch1.22 KBperusio
PASSED: [[SimpleTest]]: [MySQL] 35,027 pass(es).
[ View ]
#16 drupal-drupal_goto-documentation-1536750-16.patch1.17 KBperusio
PASSED: [[SimpleTest]]: [MySQL] 35,358 pass(es).
[ View ]
#13 drupal-drupal_goto-documentation-1536750-13.patch1.13 KBperusio
PASSED: [[SimpleTest]]: [MySQL] 35,360 pass(es).
[ View ]

Comments

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.

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.

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.

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?

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.

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

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

I totally agree with #6.

How do we proceed? With a patch?

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

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,

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

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.

StatusFileSize
new1.13 KB
PASSED: [[SimpleTest]]: [MySQL] 35,360 pass(es).
[ View ]

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

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.

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.

Status:Needs work» Needs review
StatusFileSize
new1.17 KB
PASSED: [[SimpleTest]]: [MySQL] 35,358 pass(es).
[ View ]

New patch.

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.

StatusFileSize
new1.22 KB
PASSED: [[SimpleTest]]: [MySQL] 35,027 pass(es).
[ View ]

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

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

StatusFileSize
new1.39 KB
PASSED: [[SimpleTest]]: [MySQL] 34,999 pass(es).
[ View ]

Thanks for your help. Here it is.

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

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

OK. :)

StatusFileSize
new1.44 KB
PASSED: [[SimpleTest]]: [MySQL] 35,006 pass(es).
[ View ]

One more try.

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

StatusFileSize
new6.75 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-drupal_goto-documentation-1536750-26.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

I think it's this the one.

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.

Status:Needs work» Needs review
StatusFileSize
new1.43 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-drupal_goto-documentation-1536750-29.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Re-rolling.

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new1.43 KB
PASSED: [[SimpleTest]]: [MySQL] 35,006 pass(es).
[ View ]

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

Status:Needs review» Reviewed & tested by the community

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

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?

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.

Status:Needs work» Needs review
StatusFileSize
new1.43 KB
PASSED: [[SimpleTest]]: [MySQL] 35,009 pass(es).
[ View ]

Whitespace removed.

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

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.

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.

Status:Patch (to be ported)» Needs review
StatusFileSize
new1.48 KB
PASSED: [[SimpleTest]]: [MySQL] 190 pass(es).
[ View ]

Hello Jennifer,

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

Status:Needs review» Reviewed & tested by the community

Thanks!

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

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.

Issue summary:View changes
StatusFileSize
new1.65 KB
PASSED: [[SimpleTest]]: [MySQL] 190 pass(es).
[ View ]

Re-rolled Patch.

Status:Needs work» Needs review

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

Thanks!

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.