Further clean up of API docs for File module

Thanks! Most of the updates here are good. A few things to fix:

a) This change is not necessary (or even a good idea):

+++ b/core/modules/file/file.field.inc
@@ -301,7 +301,7 @@ function file_field_is_empty($item, $field) {
  *   A field array.
  *
  * @return
- *   Boolean TRUE if the file will be displayed, FALSE if the file is hidden.
+ *   A Boolean TRUE if the file will be displayed, FALSE if the file is hidden.

b)

+++ b/core/modules/file/file.module
@@ -107,8 +107,9 @@ function file_entity_info() {
 /**
  * Loads file entities from the database.
  *
- * @param array $fids
+ * @param array|null $fids
  *   (optional) An array of entity IDs. If omitted, all entities are loaded.
+ *   Default to NULL.

Default => Defaults

c)

class CopyTest extends FileManagedTestBase {
+
   public static function getInfo() {

Please take all out-of-scope pieces out of this patch. This is a documentation clean-up issue, and I am not even sure if we have a coding standard for this blank line you've added, in any case.

d) CopyTest.php:

/**
-   * Test renaming when copying over a file that already exists.
+   * Tests the renaming when copying over a file that already exists.
    */
   function testExistingRename() {

Take out "the", for consistency with the other tests in this file.

e) Same file:

/**
-   * Test that copying over an existing file fails when FILE_EXISTS_ERROR is
+   * Tests that copying over an existing file fails when FILE_EXISTS_ERROR is
    * specified.
    */
   function testExistingError() {

Needs to be shortened to one line.

f)

/**
- * Tests for download/file transfer functions.
+ * Tests for download/file transfer functions in File module.
  */
 class DownloadTest extends FileManagedTestBase {

Take out "for" to turn "Tests" into a verb.

g) There are still several ??? in this file. I stopped reviewing when I got to the first one.

What are you asking in #5? There are ??? in the patch, so it is not done, and I marked it "needs work" after reviewing quite a bit of it. Is that a problem? It is not the job of the reviewer to write the patch, sorry.

Re-roll with fixes suggested in #4.

It took me insane amount of time to look through all the changes! :)
Everything seems looking good, no grammatical mistakes found, variables in comments are correct, comment standards are used.
Marking as RTBC. Probably someone more competent in File module can take a more close look on it.
Thanks!

This patch unfortunately touches some of the same files as an "avoid commit conflicts" issue, so I'm going to wait on committing it until
#1888424: Make Drupal's URL generation logic available to HttpKernel, and minimize code repetition/divergence
is taken care of. At which point it might need a reroll.

Definitely needs a reroll

curl https://drupal.org/files/1811674-7-file-docs.patch | git apply --check
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 48135  100 48135    0     0  22561      0  0:00:02  0:00:02 --:--:-- 30935
error: core/modules/file/file.admin.css: No such file or directory
error: patch failed: core/modules/file/file.api.php:51
error: core/modules/file/file.api.php: patch does not apply
error: patch failed: core/modules/file/file.field.inc:352
error: core/modules/file/file.field.inc: patch does not apply
error: patch failed: core/modules/file/file.module:505
error: core/modules/file/file.module: patch does not apply
error: patch failed: core/modules/file/lib/Drupal/file/Tests/DownloadTest.php:8
error: core/modules/file/lib/Drupal/file/Tests/DownloadTest.php: patch does not apply
error: patch failed: core/modules/file/lib/Drupal/file/Tests/FileFieldDisplayTest.php:8
error: core/modules/file/lib/Drupal/file/Tests/FileFieldDisplayTest.php: patch does not apply
error: patch failed: core/modules/file/lib/Drupal/file/Tests/FileFieldTestBase.php:124
error: core/modules/file/lib/Drupal/file/Tests/FileFieldTestBase.php: patch does not apply
error: patch failed: core/modules/file/lib/Drupal/file/Tests/FileManagedTestBase.php:88
error: core/modules/file/lib/Drupal/file/Tests/FileManagedTestBase.php: patch does not apply
error: patch failed: core/modules/file/lib/Drupal/file/Tests/SaveTest.php:8
error: core/modules/file/lib/Drupal/file/Tests/SaveTest.php: patch does not apply
error: patch failed: core/modules/file/lib/Drupal/file/Tests/ValidatorTest.php:45
error: core/modules/file/lib/Drupal/file/Tests/ValidatorTest.php: patch does not apply
error: patch failed: core/modules/file/tests/file_test/file_test.module:52
error: core/modules/file/tests/file_test/file_test.module: patch does not apply

@jhodgdon, Should this be marked 'postponed' as you mentioned in #10 or should I reroll to get a working copy.

The reroll monkey

@pwieck that issue appears to have been completed/committed, so you can move forward with a re-roll.

Working on reroll

Reroll complete.

Reroll passed and ready for review.

Forgot interdiff.txt

This patch is mostly pretty good, but I have several concerns... and I wonder whether it is even a good idea to try to get such large patches reviewed/committed, sigh. Maybe we should just abandon this type of effort?

a) [file.api.php] Why is changing "doesn't" to "does not" even being done? That is not cleanup. That is someone's personal preference who apparently doesn't like contractions. Contractions are not forbidden in any style guide that I'm aware of.

b) In several places in file.module:

- *   Replace behavior when the destination file already exists:
+ *   (optional) Replace behavior when the destination file already exists:
  *   - FILE_EXISTS_REPLACE - Replace the existing file. If a managed file with
  *       the destination name exists then its database entry will be updated. If
  *       no database entry is found then a new one will be created.
  *   - FILE_EXISTS_RENAME - Append _{incrementing number} until the filename is
  *       unique.
  *   - FILE_EXISTS_ERROR - Do nothing and return FALSE.
+ *   Defaults to FILE_EXISTS_RENAME.

- The list syntax should be improved (should have : instead of - after the items).
- We have a standard way to indicate the default item in the list:

Indicate optional list items with (optional) and default list items with (default). With keys, these words are placed just after the colon.

c) file.module:

- * @return \Drupal\file\FileInterface
+ * @return Drupal\file\File|false

This change is incorrect. First of all, our policy is to use interfaces not classes for data types wherever possible. Second, if you use a namespace, it must start with a \. The |false part is fine.

d)

+++ b/core/modules/file/lib/Drupal/file/Tests/FileFieldTestBase.php
@@ -23,6 +23,11 @@
   */
   public static $modules = array('file', 'file_module_test', 'field_ui');
 
+  /**
+  * A user with administrative permissions.
+  *
+  * @var object
+  */
   protected $admin_user;

In D7, we shouldn't use @var object for a user object. It should be referencing a specific User interface class.

e) FileFieldTestBase.php

+   * @param int $size
+   *   (optional) The desired size of the file, in bytes. Defaults to NULL.
+   *
+   * @return
+   *   A file object.
    */
   function getTestFile($type_name, $size = NULL) {

Saying "Defaults to NULL" here is kind of useless without telling me what that means as far as the size of the returned file. I would either leave that out or figure out what it means.

f) FileFieldTestBase.php

   /**
    * Updates an existing file field with new settings.
+   *
+   * @param $name
+   *   The name of the new field (all lowercase), exclude the "field_" prefix.
+   * @param $type_name
+   *   The node type that this field will be added to.
+   * @param $instance_settings
+   *   (optional) A list of instance settings that will be added to the instance
+   *   defaults. Defaults to an empty array.
+   * @param $widget_settings
+   *   (optional) A list of widget settings that will be added to the widget
+   *   defaults. Defaults to an empty array.
    */
   function updateFileField($name, $type_name, $instance_settings = array(), $widget_settings = array()) {

It looks like this doc block was copied from an insert/create function. It isn't quite right for the update function. Also the third param description doesn't end in ".".

g) Same file

+   * @return int
+   *   The node ID
    */
   function uploadNodeFile($file, $field_name, $nid_or_type, $new_revision = TRUE, $extras = array()) {

Doesn't end in "."

h) Same file

+   * @param string $message
+   *   (optional) The assertion message to display. Defaults to NULL.
    */
   function assertFileExists($file, $message = NULL) {
     $message = isset($message) ? $message : t('File %file exists on the disk.', array('%file' => $file->getFileUri()));

This would be a lot more useful if it said something like "The default message is ..." rather than "Defaults to NULL", which tells me really nothing. Either that or just leave out the "Defaults to NULL". Same applies to several other methods here and in other classes.

i)

+++ b/core/modules/file/lib/Drupal/file/Tests/FileFieldValidateTest.php
@@ -13,7 +13,19 @@
  * Tests various validations.
  */
 class FileFieldValidateTest extends FileFieldTestBase {
+
+  /**
+   * An unused member of this class.
+   *
+   * @var
+   */
   protected $field;
+
+  /**
+   * An unused member of this class.
+   *
+   * @var
+   */
   protected $node_type;
 

@var is missing data types

j) FileManagedTestBase

  /**
-   * Assert that all of the specified hook_file_* hooks were called once, other
+   * Asserts that all of the specified hook_file_* hooks were called once, other
    * values result in failure.

needs to be one line.

k)

+++ b/core/modules/file/lib/Drupal/file/Tests/MoveTest.php
@@ -8,7 +8,7 @@
 namespace Drupal\file\Tests;
 
 /**
- * Move related tests
+ * Move related tests for the File module.
  */
 class MoveTest extends FileManagedTestBase {

Should start with a verb ending in s and "move-related" should be hyphenated.

l) same file

  /**
-   * Test that moving onto an existing file fails when FILE_EXISTS_ERROR is
+   * Tests that moving onto an existing file fails when FILE_EXISTS_ERROR is
    * specified.
    */
   function testExistingError() {

Needs to be one line.

m) SaveUploadTest

   /**
    * The largest file id when the test starts.
+   *
+   * @var string
    */
   protected $maxFidBefore;

This is probably an integer not a string? Also you could capitalize ID in the previous line.

n)

+++ b/core/modules/file/lib/Drupal/file/Tests/ValidatorTest.php
@@ -8,7 +8,7 @@
 namespace Drupal\file\Tests;
 
 /**
- *  This will run tests against the file validation functions (file_validate_*).
+ *  Tests for the File validation functions (file_validate_*).
  */

Why was "File" capitalized?

o)

+++ b/core/modules/file/tests/file_module_test.module
@@ -27,6 +27,15 @@ function file_module_test_menu() {
 /**
  * Form constructor for testing a 'managed_file' element.
  *
+ * @param bool $tree
+ *   (optional) A Boolean indicating the desired state of $form['#tree']
+ *   element. Defaults to TRUE.
+ * @param bool $extended
+ *   (optional) A boolean indicating the desired state of the '#extended' form
+ *   element.  Defaults to FALSE.

2nd parameter - boolean vs. Boolean

p)

+++ b/core/modules/file/tests/file_test/file_test.module
@@ -38,6 +38,99 @@ function file_test_stream_wrappers() {
 
 /**
  * Reset/initialize the history of calls to the file_* hooks.
+ * Form to test file uploads.
+ *
+ * @see _file_test_form_submit()
+ */
+function _file_test_form($form, &$form_state) {
+  $form['file_test_upload'] = array(
+
...

Um.... Why is all this code being added to the module here???

q) same file

/**
- * Get the arguments passed to invocation of a given hook since
+ * Gets the arguments passed to invocation of a given hook since
  * file_test_reset() was last called.
  

Needs to be one line.

r) same file

 * @param $filepath
- *   File path
+ *   (optional) A file path.  Defaults to NULL.

Two spaces after . should be 1.

s)

+++ b/core/modules/file/tests/file_test/lib/Drupal/file_test/DummyStreamWrapper.php
@@ -20,20 +20,23 @@ function getDirectoryPath() {
   }
 
   /**
-   * Override getInternalUri().
+   * Overrides getInternalUri().
    *
-   * Return a dummy path for testing.
+   * @return string
+   *   A string with a dummy path for testing.
    */
   function getInternalUri() {
     return '/dummy/example.txt';
   }
 
   /**
-   * Override getExternalUrl().
+   * Overrides getExternalUrl().
    *
-   * Return the HTML URI of a public file.
+   * @return string
+   *   A string with the HTML URI of a public file.
    */
   function getExternalUrl() {
     return '/dummy/example.txt';
   }

For extra credit, saying "Overrides [same method name that is being documented]" is not really useful or correct. It should say something like "Overrides ClassName::methodName()". or "Overrides \Fully\Namespaced\ClassName::methodName()".

Here's a pass to clean up the outstanding issues.

s) I don't believe these methods are overriding anything.

Thanks! Looking better. There are still a few problems:

a) file.field.inc -- file_field_widget_uri()

- *   An array of token objects to pass to
- *   \Drupal\Core\Utility\Token::replace().
+ *   (optional) An array of token objects to pass to token_replace(). Defaults
+ *   to an empty array.

This change is incorrect. The code is actually calling ]Drupal::token()->replace(). I don't think token_replace() still exists in D8.

b) file.module file_icon_url()

  * @param $icon_directory
  *   (optional) A path to a directory of icons to be used for files. Defaults to
- *   the value of the "icon.directory" variable.
+ *   NULL, which results in the value of the "file_icon_directory" variable.

This is incorrect. This parameter is passed to file_icon_path(), which does config('file.settings')->get('icon.directory')
so I don't see how that is the value of the "file_icon_directory" variable?

c)

+++ b/core/modules/file/lib/Drupal/file/Tests/FileFieldTestBase.php
@@ -23,6 +23,11 @@
   */
   public static $modules = array('file', 'file_module_test', 'field_ui');
 
+  /**
+  * A user with administrative permissions.
+  *
+  * @var Drupal\user\User
+  */
   protected $admin_user;

- When using namespaces in documentation, you must start them with a \
- When specifying @var/@param/@return types, we want to see interfaces rather than classes. In fact, there's another issue that is fixing up uses of the User class in favor of I think it is UserInterface or AccountInterface or something.

d) Same file, method updateFileField

   /**
    * Updates an existing file field with new settings.
+   *
+   * @param string $name
+   *   The name of the new field (all lowercase), exclude the "field_" prefix.

This isn't a new field. It's an existing field. Also, I don't think the field_ prefix is really excluded. The function calls field_info_instance() passing in $name exactly as it is passed in, and that function doesn't add "field_" to field names as far as I can tell?

e) Same file, method uploadNodeFile

+   * @param object $file
+   *   A file object.

Don't use object here. Either just leave it out or figure out what class (or preferably interface) it actually needs to be. It's not a generic object.
==> Applies to a lot of other methods in this file

f) same method

+   * @param bool $new_revision
+   *   (optional) Determines if file is a new revision. Defaults to TRUE.

Not really accurate. You set it to TRUE if you want the node to get a new revision when this new file is uploaded, and FALSE if you don't want a new revision to be created.
==> This also applies to the removeNodeFile and replaceNodeFile methods below in the patch.

g) same method

+   * @param array $extras
+   *   (optional) An associative array of settings to change from the defaults,
+   *   keys are node properties. Defaults to an empty array.

Not really accurate. This is actually a terrible name for the parameter. Can we change it to $node_properties (and yes, changing a parameter name is considered documentation so it's OK to do) and document it as the properties to use to create the node if $nid_or_type is a node type? We should also probably mention in $nid_or_type that if it's a node type string, a new node is created inside this function.

h) removeNodeFile method (and also applies to other methods in this file)

+   * @param int $nid
+   *   A node ID.

Can this please say "The node ID of the node to remove the file from" or at least "The node ID of the node" rather than a non-specific "A node ID"?

i) Can we add to removeNodeFile and replaceNodeFile a note that it assumes that there is exactly one file attached to the node, because it works by submitting with the "Remove" button on the edit screen?

j) same file

+   * @param string $message
+   *   The assertion message to display.
    */
   function assertFileEntryExists($file, $message = NULL) {

There are a bunch of these methods with optional $message parameters. On some, (optional) is included, and not on others. Let's put it on all of them. It would also be nice to describe the default message or at least say something like "if omitted a default message is provided"?
==> This also applies to other files in this patch.

k) FileFieldValidateTest

+
+  /**
+   * An unused member of this class.
+   *
+   * @var \Drupal\field\Plugin\Core\Entity\Field
+   */
   protected $field;
+
+  /**
+   * An unused member of this class.
+   *
+   * @var string
+   */
   protected $node_type;

If these properties are unused (and I think that is correct), let's just remove them rather than documenting what they are and what type they are? Or at least let's leave out the @var since if they are unused they certainly don't have a type. I realize all member properties are supposed to have @var but if they are unused they don't have a type.

I also checked and there is nothing extending this class anywhere... Actually let's leave them undocumented and file a separate issue to remove them?

l) FileManagedTestBase

-   * @return \Drupal\file\FileInterface
-   *   File entity.
+   * @return
+   *   File object.
    */
   function createFile($filepath = NULL, $contents = NULL, $scheme = NULL) {

Why lose the @return type and documentation here? Was it incorrect?

m) SaveUploadTest

+   * The largest file ID when the test starts.
+   *
+   * @var int string
    */
   protected $maxFidBefore;

This should either be int|string, int, or string, but not "int string". Probably it's an int?

n) file_module_test.module file_module_test_form()
The $default_fids parameter is documented here as $default_fid instead.

o) Comment (s) from my previous review still applies to the DummyStreamWrapper methods. If those methods are not overriding anything, then don't document them as overriding something. If they are overriding something (probably from an interface somewhere?) then put that interface name into the method name.

Cleaned up some more. In reference to the overridden methods, it turns out that getExternalUrl() was an override, but getInternalUri() doesn't override anything, nor does it appear to be used. Perhaps we could remove that in a follow-up.

These issues are a lot of work with very little tangible payoff, so I'm closing the rest of them as "won't fix". Your efforts on working on this issue were appreciated... it was just my fault for starting a task that was very difficult to get right.

Let's instead put our effort into fixing and reviewing documentation that is really unclear and/or wrong, and I hope that the people who worked on these issues are not afraid to jump into a more reasonable issue!