/**
964 * Send the HTTP response headers previously set using drupal_add_http_header().
965 * Add default headers, unless they have been replaced or unset using
966 * drupal_add_http_header().
967 *
968 * @param $default_headers
969 * An array of headers as name/value pairs.
970 * @param $single
971 * If TRUE and headers have already be sent, send only the specified header.
972 */
973 function drupal_send_headers($default_headers = array(), $only_default = FALSE) {
974 $headers_sent = &drupal_static(__FUNCTION__, FALSE);
975 $headers = drupal_get_http_header();
976 if ($only_default && $headers_sent) {
977 $headers = array();
978 }
979 $headers_sent = TRUE;

As the title, there may be a typo in the bootstrap.inc file. We believe that the second param should be unified with the real param.
So i suggest they are both "$default_single".

Files: 
CommentFileSizeAuthor
#12 drupal-typos_in_docs_for_drupal_8-1853050-12.patch1.26 KBJerenus
PASSED: [[SimpleTest]]: [MySQL] 48,920 pass(es).
[ View ]
#10 drupal-typos_in_docs_for_drupal_8-1853050-10.patch1.17 KBJerenus
PASSED: [[SimpleTest]]: [MySQL] 48,900 pass(es).
[ View ]
#9 drupal-typos_in_docs_for_drupal_8-1853050-9.patch1.01 KBJerenus
PASSED: [[SimpleTest]]: [MySQL] 48,898 pass(es).
[ View ]
#7 drupal-typos_in_docs_for_drupal_8-1853050-7.patch1023 bytesJerenus
PASSED: [[SimpleTest]]: [MySQL] 48,898 pass(es).
[ View ]
#4 drupal-fix_typo-1853050-4.patch842 bytesJerenus
FAILED: [[SimpleTest]]: [MySQL] Repository checkout: failed to checkout from [git://git.drupal.org/project/drupal.git].
[ View ]
#3 drupal-fixed_typo-1853050-3.patch1.15 KBJerenus
PASSED: [[SimpleTest]]: [MySQL] 39,627 pass(es).
[ View ]
#2 drupal-fixed_typo-1853050.patch1.15 KBJerenus
PASSED: [[SimpleTest]]: [MySQL] 39,638 pass(es).
[ View ]

Comments

Here is my patch.

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

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

Sorry about my mistake, here is the new one.

Component:plugin system» documentation
StatusFileSize
new842 bytes
FAILED: [[SimpleTest]]: [MySQL] Repository checkout: failed to checkout from [git://git.drupal.org/project/drupal.git].
[ View ]

May be you like "default", new one ;)

Status:Needs review» Needs work

The last submitted patch, drupal-fix_typo-1853050-4.patch, failed testing.

Title:There may be a Drupal core typo in the bootstrap.inc fileTypos in docs for drupal_send_headers()
Version:7.x-dev» 8.x-dev

That patch in #4 looks fine... but could we also fix the typos in the next line down? "headers have already *been* sent... the specified header*s*". That will most likely also cause the line to need wrapping to 80-character maximum. Thanks!

Also, we need to fix this in Drupal 8.x first, then backport the fix to 7.x.

StatusFileSize
new1023 bytes
PASSED: [[SimpleTest]]: [MySQL] 48,898 pass(es).
[ View ]

jhodgdon,
Here is my new patch for Drupal 8.x, please check.

Thanks!
Jerenus

Both of the function parameters have optional values. Hence, each of the description lines should start with '(optional)' and include a statement of what the default value is. Each @param directive also could use a variable type hint.

I am not at a development machine right now, but if this is not re-rolled before late tomorrow, I will roll a patch with the above changes.

StatusFileSize
new1.01 KB
PASSED: [[SimpleTest]]: [MySQL] 48,898 pass(es).
[ View ]

jhodgdon,
Sorry for missing your last piece of advice. Here is new one.

Thanks!!
Jerenus

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

Lars Toomre,
Here is additional one for your advice(#8).

Thanks,
Jerenus

Status:Needs review» Needs work

RE #8 - Starting with (optional) - Yes. Including information on the default values - No, unless it is not obvious from the function signature.

So on the patch in #10 - you can take out the "Defaults to FALSE". Also, if you're going to put the type "bool" into one parameter, maybe put the type "array" in for the other one? Thanks!

Status:Needs work» Needs review
StatusFileSize
new1.26 KB
PASSED: [[SimpleTest]]: [MySQL] 48,920 pass(es).
[ View ]

jhodgdon,
Here is the new one, please check.

Thanks!
Jerenus

This is much better, thanks!

But... the parameter names for this function are quite confusing, since they have "default" in the name, which doesn't make any sense. Now that we have clear documentation about what the parameters *do*, it is quite obvious that the names of the parameters are bad. Maybe we should change the names of the parameters to $additional_headers and $only_additional, or something like that? As they are only parameter names and local variables, that is considered "documentation" and can be done in this patch...

I also think that the previous paragraph of documentation is confusing... well I can't figure out what this is trying to say, given the code and the rest of the documentation:
" Default headers are not set if they have been replaced or unset using drupal_add_http_header()."

Any guesses? If you think these additional changes are out of scope for this issue, then we can commit the docs fix (which is good!) and work on those other problems in a different issue. Thoughts?

Title:Typos in docs for drupal_send_headers()Fix docs and param names for drupal_send_headers()
Status:Needs review» Active

OK... I went ahead and committed the patch in #12 to both 8.x and 7.x. Thanks for your work on this patch!

I'd like to leave this issue open though. What I'd like to see happen:
- Make sure the whole documentation for this function makes sense
- Make sure the parameter names make sense

In my opinion, neither of these is true at this time (see comment #13).

jhodgdon,
Sorry about our jet lag. $only_additional is good. If you want to improve this patch, i'd like to do. But, can you change the author name of the patchs to my name? Thank you very much!

Jerenus

What are you talking about on the "author name of the patches"? Both of the commits have your user name in the commit message (which is how we attribute patches in Drupal Core):
http://drupalcode.org/project/drupal.git/commit/11fafe6
http://drupalcode.org/project/drupal.git/commit/2b6e2c1

@jhodgdon

he was saying that: his patch contains author meta data, but after you committed it, the author was changed to you. Please see the commit msg, both the author and the committer was you. You probably didn't simply apply his patch, which was the correct work flow. You might have just edited the file and committed; this way, git doesn't credit the people.

For more info: http://www.mcdruid.co.uk/content/git-commit-author-give-credit-where-cre...

@skyredwang: In the Drupal Core project, we do not use author attribution for git patches, as a general rule.

To quote @webchick, replying to another user by email a few days ago:

I believe [this user] might be referring to giving credit via the --author flag, e.g.

git commit --author=[username]@[uid].no-reply.drupal.org

This makes people show up here: http://drupal.org/node/3060/committers as well as in Git log.

To date, however, we do not employ this method of commit credit in Drupal core, however, because in nearly all cases [...], a patch is the cumulative effort of several people: someone to do the initial patch, one or more people to provide feedback on it, maybe more people doing re-rolls and adding tests, etc. Unfortunately, Git's "author" flag only supports single values, not multiple values. :\ The people who are listed on that page are generally people who've done major "forklift" operations into core for major initiatives that were merged in from an external sandbox.

So Jennifer is correct that in Drupal core, we do this via comma-separated names in the commit message itself, and parse these out using various tools, such as stats.marvil07.net/drupal-core/. This is more fair overall to everyone.

jhodgdon,
Thanks for your patience and good attitude. Anyway, i will continue to dedicate my efforts to the community. Any new idea about the #13&#14? Or i just try to patch as you said above? Or you want to keep the present?

Thanks!
Jerenus

The patch in #12 has been committed, and I would like to see a change as noted in #13/14... Do you have a different idea? I'm definitely open to suggestions. It would be good if this function's code actually made sense. :)