Further cleanup for documentation in core/includes files starting with A-G

#1: Easy_Lists_Fixes_A_G-1333534-1.patch queued for re-testing.

Re-roll.

I gave this a quick look and found some issues:

In ajax.inc:

+ *   (optional) An CSS selector which must be inside $selector. If specified, an

--> A CSS selector

In bootstrap.inc:

*   from $title using a function like check_plain() or filter_xss(). With this
- *   flag the string will be passed through unchanged.
+ *   flag set so, the string will be passed through unchanged.

"flag set so"... that is not good English. How about "With this flag setting, the ..."

Sorry, I don't have time to review the rest of the patch right now. It's a big one.

Yeah, this re-roll was definitely a pain. Here's those two items fixed.

Thanks! I'll try to get the rest of it reviewed soon too.

Also, sorry, but I just committed a patch that will require a reroll (removal of a bit of the bootstrap.inc patch) on
#1853050: Fix docs and param names for drupal_send_headers()

Re-rolled. Much easier this time :)

#10: easy_list_fixes_a_g-1333534-10.patch queued for re-testing.

I gave a lot of this this patch a thorough review, and most of it is ... well...

It just seems to be a huge waste of time to create/review/commit this patch. If a function signature has a default value for a parameter, it is really not necessary for the @param to tell us the parameter is (optional), and it's also a waste of time for the docs to tell us what the default value is. So ... I'm not going to review the rest of the patch that I didn't get to right now -- I can find better ways to spend my time...

I did find these errors in the parts I did review:

a) common.inc -- l() function docs:

- *   - 'html' (default FALSE): Whether $text is HTML or just plain-text. For
+ *   - html: (optional) Whether $text is HTML or just plain-text. For
...
- *     safe.
+ *     safe. Default to FALSE.

Default to FALSE -> Defaults to FALSE. Also it would be nice if this information about the default value could be moved back up to the beginning of this section. Maybe the first sentence could say "Whether $text is HTML (TRUE) or plain text (FALSE, the default)."?

b) common.inc

* @see drupal_get_css()
+ * @see drupal_pre_render_conditional_comments()
  */
 function drupal_add_css($data = NULL, $options = NULL) {

We don't need this. The function is already referenced in the relevant section of the documentation.

c) Somewhere in common.inc

- *   Set to TRUE to indicate that the attachments are added to every page on the
- *   site. Only attachments with the every_page flag set to TRUE can participate
+ *   (optional) Set to TRUE to indicate that the attachments are added to every
+ *   page on the site. Only attachments with the every_page flag set to TRUE can participate
*   in JavaScript/CSS aggregation.
<code>
Needs to be wrapped to 80 characters.

d) common.inc
* @param $force
- *   (Optional) If TRUE, the value is forced into the structure even if it
- *   requires the deletion of an already existing non-array parent value. If
- *   FALSE, PHP throws an error if trying to add into a value that is not an
- *   array. Defaults to FALSE.
+ *   (optional) A boolean with default FALSE value. If TRUE, the value is forced
+ *   into the structure even if it requires the deletion of an already existing
+ *   non-array parent value. If FALSE, PHP throws an error if trying to add into
+ *   a value that is not an array.

Boolean should be capitalized. And can we possibly start the @param docs with an explanation of what the parameter *does*, not the overly wordy sentence that just tells us the default value? What was wrong with the previous docs?

e) common.inc function drupal_parse_dependency()

* @return
  *   An associative array with three keys:
- *   - 'name' includes the name of the thing to depend on (e.g. 'foo').
- *   - 'original_version' contains the original version string (which can be
- *     used in the UI for reporting incompatibilities).
- *   - 'versions' is a list of associative arrays, each containing the keys
- *     'op' and 'version'. 'op' can be one of: '=', '==', '!=', '<>', '<',
- *     '<=', '>', or '>='. 'version' is one piece like '4.5-beta3'.
+ *   - name: The name of the thing to depend on (e.g. 'foo').
+ *   - original_version: The original version string (which can be used in the
+ *     UI for reporting incompatibilities).
+ *   - versions: A list of associative arrays, each containing the keys 'op' and
+ *     'version'. 'op' can be one of: '=', '==', '!=', '<>', '<', '<=', '>', or
+ *     '>='. 'version' is one piece like '4.5-beta3'.

In the 'name' component, can we say "('foo' in this example)" at the end instead of using "e.g."? It's referring to the example in the parameter. And I'm not sure what the 'version' part of 'versions' really means? Maybe that can be reworded?

f) file.inc

- *   An associative array of additional options, with the following elements:
- *   - 'nomask': The preg_match() regular expression of the files to ignore.
- *     Defaults to '/(\.\.?|CVS)$/'.
- *   - 'callback': The callback function to call for each match. There is no
- *     default callback.
- *   - 'recurse': When TRUE, the directory scan will recurse the entire tree
- *     starting at the provided directory. Defaults to TRUE.
- *   - 'key': The key to be used for the returned associative array of files.
- *     Possible values are 'uri', for the file's URI; 'filename', for the
+ *   (optional) An associative array of additional options, with the following
+ *   elements:
+ *   - nomask: (optional) The preg_match() regular expression of the files to
+ *     ignore. Defaults to '/(\.\.?|CVS)$/'.
...

All of the elements in $options are optional. A more concise way to state this would be to put in the first line "An associative array of additional options, which can contain any of the following elements:" and then we wouldn't need to clutter the list by putting (optional) on every single element.

g) file.inc

* @param $depth
- *   Current depth of recursion. This parameter is only used internally and
- *   should not be passed in.
+ *   (optional) Current depth of recursion. This parameter is only used
+ *   internally and should not be passed in.

Take this out. I wouldn't say "optional" here since the docs say **not** to use it.

h) file.inc

+ *    - mimetypes: A list of mimetypes, keyed by an identifier,
+ *    - extensions: The mapping itself, an associative array in which the key is

Should be . at end of first line not ,

apparently I forgot to change the status earlier.

I'm working on these issue.

Added patch.

Updated following files by adding *(optional)* for @param description wherever function params has default value assigned.

core/includes/bootstrap.inc
core/includes/common.inc
core/includes/database.inc
core/includes/entity.inc
core/includes/errors.inc
core/includes/file.inc

Needs review.

So in general, this patch is kind of OK, but when you put (optional) on a parameter, it is usually helpful to state what the default behavior or value would be. Not all of these parameter docs have that information. So please go through and check for that.

Also, what happened to the previous patch, which had some additional fixes in it? It seems like you just started over... which maybe was appropriate since it had been 3 years, but it would have been nice to say something about it since this was an existing issue.

A few other concerns:

1. +++ b/core/includes/common.inc
   @@ -242,7 +242,7 @@ function check_url($uri) {
   + *   (optional) The Language code to translate to a language other than what is used
   

   This makes the line go over 80 characters. Needs rewrapping.

2. +++ b/core/includes/database.inc
   @@ -109,10 +109,10 @@ function db_query_range($query, $from, $count, array $args = array(), array $opt
     * @param array $options
     *   An array of options to control how the query operates.
   

   It looks like you put (optional) on the other $options parameters, but this one was missed.

3. +++ b/core/includes/database.inc
   @@ -321,7 +321,7 @@ function db_transaction($name = NULL, array $options = array()) {
   - *   The key in the $databases array to set as the default database.
   + *   (optional) The key in the $databases array to set as the default database.
   

   Really, this is optional? What is the default value? The function docs say "Sets a new active database" so if you don't set a key... what would this do?

4. +++ b/core/includes/database.inc
   @@ -458,7 +458,7 @@ function db_close(array $options = array()) {
   + *   (optional) After a database import, it might be that the sequences table is behind, so
   

   Line goes over 80 chars, rewrap

5. +++ b/core/includes/entity.inc
   @@ -54,7 +54,7 @@ function entity_get_bundles($entity_type = NULL) {
   + *   (optional) Whether to reset the internal cache for the requested entity type.
   

   over 80

6. +++ b/core/includes/file.inc
   @@ -291,7 +291,7 @@ function file_url_transform_relative($file_url) {
   + *   (optional) A bitmask to indicate if the directory should be created if it does
   

   over 80

• There are few functions that has default value set (not NULL or array() ), for those i added default value description. Pls suggest that do we required to add default value description for params which has default NULL or empty array(). I mean like following function.

  
  function MyFun($a = NULL, $b = array()){
  
  }
  
  

• The previous patch is too old. Yes, I started over.

1. Done wrapping
2. Added
3. Yes these is optional & default value is 'default'. And It sets the database of 'default' key in $databases array. ('default' key has another array which holds db connections details i believe)
4. Done wrapping
5. Done wrapping
6. Done wrapping

Restarted test & it passing.

Moving to old state as it moved by BOT

Looking better! So I think there are a few things that still could be improved, in an issue titled "cleanup":

1. +++ b/core/includes/bootstrap.inc
   @@ -353,16 +353,16 @@ function drupal_validate_utf8($text) {
   + *   (optional) The message to store in the log. If empty, a text that contains
   + *   all useful information about the passed-in exception is used.
   

   Thanks for providing a default value explanation here!

   So, in English, "text" is not a countable noun. That means you cannot say "one text", "two texts", or "a text".

   I think I would change this to just say

   If empty, a message is generated containing exception information.

   Because it's also not really (probably?) correct that "all useful information" about the exception is included... so let's just leave it at that. :)

2. +++ b/core/includes/bootstrap.inc
   @@ -901,9 +902,9 @@ function &drupal_static($name, $default_value = NULL, $reset = FALSE) {
   + *   (optional) The Name of the static variable to reset. Omit to reset all
   

   Name should be lower-case here.

3. +++ b/core/includes/bootstrap.inc
   @@ -931,7 +932,7 @@ function drupal_placeholder($text) {
   - *   The shutdown function to register.
   + *   (optional) The shutdown function to register.
   

   Since this is optional, I think it should say "A" instead of "The"?

   Also since this function is called
   drupal_register_shutdown_function()
   it is a bit odd that the shutdown function is optional!

   So this might require a bit more explanation.

   Maybe it would be better to say:

   (optional) An additional shutdown function to register().

   The next param ... is also optional.

   And then in the @return maybe change this to say

   @return array
   The list of all registered shutdown functions.

   That might make it clearer that if you call with no parameters, you're basically just asking for the list to be returned.

   Does that make sense?

4. +++ b/core/includes/common.inc
   @@ -242,8 +242,8 @@ function check_url($uri) {
   + *   (optional) The Language code to translate to a language other than
   + *   what is used to display the page.
   

   Language should be lower-case.

   Also we have changed the wording on this in t() and other functions I thought... it should say:

   (optional) A language code, to translate to a language other than what is used to display the page.

5. +++ b/core/includes/database.inc
   @@ -321,7 +321,8 @@ function db_transaction($name = NULL, array $options = array()) {
   + *   (optional) The key in the $databases array to set as the default database.
   + *   By default key is set to 'default'.
   

   That second line would be slightly clearer, I think, if it said:

   Defaults to 'default'.

6. +++ b/core/includes/database.inc
   @@ -458,9 +459,9 @@ function db_close(array $options = array()) {
   + *   (optional) After a database import, it might be that the sequences
   + *   table is behind, so by passing in a minimum ID, it can be assured that
   + *   we never issue the same ID.
   

   I know we're just putting (optional) here, but the grammar/wording here could be cleaned up... "assured" is wrong... how about:

   After a database import, it could be that the sequences table is behind. Passing in a minimum ID ensures that we never issue the same ID.

7. +++ b/core/includes/entity.inc
   @@ -54,7 +54,8 @@ function entity_get_bundles($entity_type = NULL) {
   + *   (optional) Whether to reset the internal cache for the requested
   + *   entity type.
   

   What is the default value here?

8. +++ b/core/includes/entity.inc
   @@ -159,7 +160,8 @@ function entity_revision_delete($entity_type, $revision_id) {
   + *   (optional) Whether to reset the internal cache for the requested
   + *   entity type.
   

   default value?

9. +++ b/core/includes/errors.inc
   @@ -92,7 +92,7 @@ function _drupal_error_handler_real($error_level, $message, $filename, $line, $c
   + *   (optional) An error to examine for ERROR_REPORTING_DISPLAY_SOME.
   

   default value?

10. +++ b/core/includes/errors.inc
    @@ -120,7 +120,7 @@ function error_displayable($error = NULL) {
    + *   (optional) TRUE if the error is fatal.
    

    default value?

11. +++ b/core/includes/file.inc
    @@ -291,8 +291,8 @@ function file_url_transform_relative($file_url) {
    + *   (optional) A bitmask to indicate if the directory should be created if it
    + *   does not exist (FILE_CREATE_DIRECTORY) or made writable if it is read-only
      *   (FILE_MODIFY_PERMISSIONS).
    

    default value?

12. +++ b/core/includes/file.inc
    @@ -433,14 +433,15 @@ function file_valid_uri($uri) {
    + *   (optional) Replace behavior when the destination file already exists:
      *   - FILE_EXISTS_REPLACE - Replace the existing file.
    - *   - FILE_EXISTS_RENAME - Append _{incrementing number} until the filename is
    - *       unique.
    + *   - FILE_EXISTS_RENAME (default) - Append _{incrementing number} until
    + *     the filename is unique.
      *   - FILE_EXISTS_ERROR - Do nothing and return FALSE.
    

    Let's fix the list formatting.

    Instead of using - it should have :

    So for example:

    - FILE_EXISTS_REPLACE: Replace the...
    ...
    - FILE_EXISTS_RENAME: (default) Append ...

13. +++ b/core/includes/file.inc
    @@ -572,13 +573,13 @@ function file_destination($destination, $replace) {
      *   - FILE_EXISTS_REPLACE - Replace the existing file.
    - *   - FILE_EXISTS_RENAME - Append _{incrementing number} until the filename is
    + *   - FILE_EXISTS_RENAME (default) - Append _{incrementing number} until the filename is
      *       unique.
      *   - FILE_EXISTS_ERROR - Do nothing and return FALSE.
    

    Fix list formatting as in previous comment please.

14. +++ b/core/includes/file.inc
    @@ -620,8 +621,8 @@ function file_unmanaged_move($source, $destination = NULL, $replace = FILE_EXIST
    + *   (optional) If TRUE, drupal_set_message() will be called to display
    + *   a message if the file name was changed.
    

    default value?

15. +++ b/core/includes/file.inc
    @@ -867,15 +868,15 @@ function drupal_move_uploaded_file($filename, $uri) {
      *   - FILE_EXISTS_REPLACE - Replace the existing file.
    - *   - FILE_EXISTS_RENAME - Append _{incrementing number} until the filename is
    - *                          unique.
    + *   - FILE_EXISTS_RENAME (default) - Append _{incrementing number} until the
    + *     filename is unique.
      *   - FILE_EXISTS_ERROR - Do nothing and return FALSE.
    

    list formatting

16. +++ b/core/includes/file.inc
    @@ -925,7 +927,7 @@ function file_unmanaged_save_data($data, $destination = NULL, $replace = FILE_EX
    + *   (optional) The current depth of recursion. This parameter is only used internally and
    

    This line goes over 80 characters.

@jhodgdon : Improved all points #22. Pls let me know something missing.

Nice! This is getting very close. There are only a few small things to fix up:

1. +++ b/core/includes/bootstrap.inc
   @@ -853,10 +853,11 @@ function drupal_classloader_register($name, $path) {
   + *   (optional) TRUE to reset one or all variables(s). This parameter is only
   + *   used internally and should not be passed in; use drupal_static_reset()
   + *   instead.
     *   (This function's return value should not be used when TRUE is passed in.)
   

   The pre-existing last line needs to be wrapped with the rest of the newly wrapped paragraph.

2. +++ b/core/includes/bootstrap.inc
   @@ -931,12 +932,12 @@ function drupal_placeholder($text) {
   + *   (optional) An additional shutdown function to register().
   

   I don't think register() is a function name, so it shouldn't have () on it?

3. +++ b/core/includes/file.inc
   @@ -433,17 +433,18 @@ function file_valid_uri($uri) {
   + * @return URI|FALSE
   

   This needs to be types. So probably URI is really "string" type? And instead of "FALSE" we would put "false" for @param/@return types. So I think it should say:

   @return string|false

   See https://www.drupal.org/node/1354#types
   for more information.

4. +++ b/core/includes/file.inc
   @@ -536,12 +537,12 @@ function file_build_uri($path) {
     *   Replace behavior when the destination file already exists.
   - *   - FILE_EXISTS_REPLACE - Replace the existing file.
   - *   - FILE_EXISTS_RENAME - Append _{incrementing number} until the filename is
   - *       unique.
   - *   - FILE_EXISTS_ERROR - Do nothing and return FALSE.
   + *   - FILE_EXISTS_REPLACE: Replace the existing file.
   + *   - FILE_EXISTS_RENAME: Append _{incrementing number} until the filename is
   + *     unique.
   + *   - FILE_EXISTS_ERROR: Do nothing and return FALSE.
   

   We need to know what the default is here? The rest of these similar lists have (default) in there, but this one was missed.

5. +++ b/core/includes/file.inc
   @@ -536,12 +537,12 @@ function file_build_uri($path) {
   - * @return
   + * @return  URI|FALSE
   

   See note above about return types.

6. +++ b/core/includes/file.inc
   @@ -572,17 +573,17 @@ function file_destination($destination, $replace) {
   + * @return URI|FALSE
   

   See above note about return types.

7. +++ b/core/includes/file.inc
   @@ -613,15 +614,17 @@ function file_unmanaged_move($source, $destination = NULL, $replace = FILE_EXIST
   + * 'system.file:allow_insecure_uploads'. If it evaluates to TRUE, no
   + * alterations will be made, if it evaluates to FALSE, the filename is
   + * 'munged'.
   

   This is a comma splice. Before the "if", the comma should be a ; instead.

8. +++ b/core/includes/file.inc
   @@ -867,18 +870,18 @@ function drupal_move_uploaded_file($filename, $uri) {
   + * @return URI|FALSE
   

   See above note about return types.

Patch attached, please review.

@r_sharma08 : You wasted my 1 hour. Please assign your self before you want to work on any issue.

Thanks for patch. Please add interdiff file too.

@jhodgdon :

1. Updated wrapping.
2. Removed - ()
3. Updated to @return string|false.
4. No default value for file_destination().
5. Updated to @return string|false.
6. Updated to @return string|false.
7. Updated to ;
8. Updated to @return string|false.

+++ b/core/includes/file.inc
@@ -535,12 +536,12 @@ function file_build_uri($path) {
+ * @return  string|false

nitpick: 2 spaces here instead of 1

I edited the patch directly and re-uploaded to fix this one problem. Everything else looks fine. Thanks!

Looks like this needs a reroll.

That patch doesn't apply either.

Re-rolled. Uploading a new patch.

Oops! Looks like an incorrect patch was uploaded. Uploading another one here.

Can you kindly make a patch with more context lines in it? This one is difficult to review because I would need to open up all of the files affected, for instance to check whether parameters that this patch marks as (optional) really are optional in the function signature. Something like 6 or 8 context lines would be helpful. Thanks!

Rerolled the most recently uploaded patch on Drupal 8.0.1 stable release .Leaving one conflict, it was basically an auto-merge . Tested the patch- worked absolutely fine.

Apparently the latest patch doesn't apply.

Any update on this where are we ?

@jhodgdon can i start from #30 if i am right here.

@snehi: yes that seems like a good idea.

This issue is badly scoped according to our current Issue Scope guidelines. As it is a "cleanup misc. standards stuff" issue I am just going to close it as Won't Fix. Please do not make a patch. Thanks!

See https://www.drupal.org/core/scope for more on this...

@jhodgdon Thanks for letting me know about this.