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.
Comments
Comment #1
jhodgdonSo what is incorrect there, and how do you think it should be changed? I don't see a problem with the current docs.
Comment #2
perusio CreditAttribution: perusio commentedIMO it should read:
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.
Comment #3
jhodgdonPerhaps 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.
Comment #4
perusio CreditAttribution: perusio commentedYes I agree, it should be concise and clear and not a mere filler. Here's my 2nd attempt at a better code documentation:
How's that?
Comment #5
jhodgdonWhat we need to do is this. In the first line of the @param, where it says:
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.
Comment #6
perusio CreditAttribution: perusio commentedIf that's so then we must also remove the confusing:
Comment #7
jhodgdonI totally agree with #6.
Comment #8
perusio CreditAttribution: perusio commentedHow do we proceed? With a patch?
Comment #9
jhodgdonYes, a patch would be the next step. Thanks!
Comment #10
perusio CreditAttribution: perusio commentedCan 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,
Comment #11
jhodgdonFor information on how to use git & make a patch, start at
http://drupal.org/novice
Comment #12
perusio CreditAttribution: perusio commentedI 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.
Comment #13
perusio CreditAttribution: perusio commentedHere's the patch for 8.x. If ok I'll proceed for 7.x and 6.x.
Comment #14
perusio CreditAttribution: perusio commentedChanging status. Not sure if the closing remark:
Shouldn't be removed also. Seems confusing to me.
Comment #15
jhodgdonThanks, 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.
Comment #16
perusio CreditAttribution: perusio commentedNew patch.
Comment #17
jhodgdonBetter, but still not quite right:
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.
Comment #18
perusio CreditAttribution: perusio commentedHere's another shot. Posted a question on #drupal-docs. No reply :(
Comment #19
jhodgdonSorry 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)
Again, please make RFC 2616 in the first line *into* the link, rather than having a separate link.
b)
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:
This whole list should use our list formatting syntax -- should be a : after each integer value. See
http://drupal.org/node/1354#lists
Comment #20
perusio CreditAttribution: perusio commentedThanks for your help. Here it is.
Comment #21
jhodgdonMuch 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:
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"?
Comment #22
perusio CreditAttribution: perusio commentedThey're the valid values for Redirection codes, i.e., codes of the form
3xx
. It needs clarification.Comment #23
jhodgdonOK. :)
Comment #24
perusio CreditAttribution: perusio commentedOne more try.
Comment #25
jhodgdonWhen 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:
b) redirections -> redirection in this line:
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):
In other words, the end of that line should say
codes: @endlink
Comment #26
perusio CreditAttribution: perusio commentedI think it's this the one.
Comment #27
perusio CreditAttribution: perusio commentedChanging status for testbot.
Comment #29
perusio CreditAttribution: perusio commentedRe-rolling.
Comment #31
perusio CreditAttribution: perusio commentedRe-rolling again. Somewhere patching practices changed. Now it includes git prefixes.
Comment #32
jhodgdonExcellent -- that patch looks good. I'll get it committed soon. Thanks!
Comment #33
tim.plunkettThe first and third lines here have trailing whitepace
This is over 80 characters, is that allowed because of the @link?
Comment #34
jhodgdonDang, 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.
Comment #35
perusio CreditAttribution: perusio commentedWhitespace removed.
Comment #36
perusio CreditAttribution: perusio commentedQuestion: doesn't this needs to be backported to D6 and D7?
Comment #37
jhodgdonYes, 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.
Comment #38
jhodgdonThe patch in #35 applied to 8.x and 7.x both, so I committed it to both. Thanks! Now needs backport to 6.x.
Comment #40
perusio CreditAttribution: perusio commentedHello Jennifer,
Here's the first shot at a patch for D6. Pretty much copy & paste from D8.
Comment #41
jhodgdonThanks!
Comment #48
dixon_Setting back title after spam attack.
/me goes reporting to the webmasters.
Comment #49
jhodgdonSorry for the long delay in getting this committed... the patch above needs a re-roll for D6 apparently, since it doesn't currently apply.
Comment #50
sandipmkhairnar CreditAttribution: sandipmkhairnar commentedRe-rolled Patch.
Comment #51
sandipmkhairnar CreditAttribution: sandipmkhairnar commentedComment #52
jhodgdonThanks!
Comment #53
jhodgdonThanks again everyone! Committed to 6.x. Only two years later, this one is done. :)