Make file module pass Coder Review

Postponing till feature freeze. If you want to help in the meantime, please work on the blockers on the meta issue. Thanks!

We have the go ahead with all these issues again, see #1518116: [meta] Make Core pass Coder Review for more details

Attached to the issue you can find the patch for the file.module coding standards.

+++ b/core/modules/file/file.moduleundefined
@@ -121,6 +121,7 @@ function file_load($fid, $reset = FALSE) {
  * Returns the file usage service.
  *
  * @return Drupal\file\FileUsage\FileUsageInterface.
+ *   A file usage service.

Typically, you would use "The file usage service" instead of "A file usage service".

+++ b/core/modules/file/file.moduleundefined
@@ -143,10 +144,10 @@ function file_usage() {
- * @param $replace
+ * @param constant $replace
  *   Replace behavior when the destination file already exists:
  *   - FILE_EXISTS_REPLACE - Replace the existing file. If a managed file with

I'm not sure if @param constant is correct. I grepped core and it is not used anywhere else. These constants map to integers, so you could use int. Try to find an example where this is used in core.

+++ b/core/modules/file/file.moduleundefined
@@ -215,10 +220,10 @@ function file_copy(File $source, $destination = NULL, $replace = FILE_EXISTS_REN
- * @param $replace
+ * @param constant $replace
  *   Replace behavior when the destination file already exists:

@param constant, as above.

+++ b/core/modules/file/file.moduleundefined
@@ -481,13 +490,13 @@ function file_validate_image_resolution(File $file, $maximum_dimensions = 0, $mi
- * @param $replace
+ * @param constant $replace
  *   Replace behavior when the destination file already exists:

@param constant, as above.

+++ b/core/modules/file/file.moduleundefined
@@ -731,20 +754,20 @@ function file_cron() {
- * @param $replace
+ * @param constant $replace
  *   Replace behavior when the destination file already exists:

@param constant, as above.

+++ b/core/modules/file/file.moduleundefined
@@ -1443,10 +1471,10 @@ function file_managed_file_submit($form, &$form_state) {
- * @param $element
+ * @param managed_file $element
  *   The FAPI element whose values are being saved.

'managed_file' is not the correct data type I think. This is an array probably.

+++ b/core/modules/file/file.moduleundefined
@@ -1465,7 +1493,7 @@ function file_managed_file_save_upload($element) {
-  $files_uploaded |= !$element['#multiple'] && !empty($_FILES['files']['name'][$upload_name]);
+  $files_uploaded |=!$element['#multiple'] && !empty($_FILES['files']['name'][$upload_name]);

This seems to introduce a whitespace error. There should be a space following the |= as it was originally. Is this a code sniffer false positive perhaps?

+++ b/core/modules/file/file.moduleundefined
@@ -1475,7 +1503,9 @@ function file_managed_file_save_upload($element) {
     // Value callback expects FIDs to be keys.
     $files = array_filter($files);
-    $fids = array_map(function($file) { return $file->id(); }, $files);
+    $fids = array_map(function($file) {
+      return $file->id();
+    }, $files);
 

Anonymous functions should have a space between 'function' and the opening parenthesis.

+++ b/core/modules/file/file.moduleundefined
@@ -1820,22 +1850,22 @@ function file_icon_map(File $file) {
  *   reference check to the given field.
- * @param $age
+ * @param constant $age
  *   (optional) A constant that specifies which references to count. Use

@param constant, as above.

I've created the patch again with the comments from pfrenssen corrected.

Here's the interdiff between patches #4 and #7. It is very much appreciated if you include these, as it makes reviewing easier. Will review the new patch now.

Thanks for the corrections! I've looked through the patch again and found a few things that I missed last time:

+++ b/core/modules/file/file.moduleundefined
@@ -164,7 +165,11 @@ function file_usage() {
+      watchdog('file', 'File %file (%realpath) could not be copied because the destination %destination is invalid. This is often caused by improper use of file_copy() or a missing stream wrapper.', array(
+        '%file' => $source->getFileUri(),
+        '%realpath' => $realpath,
+        '%destination' => $destination)
+      );

Usually the last line of an array declaration ends in a comma, and the closing parenthesis is on the next line, like this:

+        '%destination' => $destination,
+      ));

+++ b/core/modules/file/file.moduleundefined
@@ -238,7 +243,11 @@ function file_copy(File $source, $destination = NULL, $replace = FILE_EXISTS_REN
-      watchdog('file', 'File %file (%realpath) could not be moved because the destination %destination is invalid. This may be caused by improper use of file_move() or a missing stream wrapper.', array('%file' => $source->getFileUri(), '%realpath' => $realpath, '%destination' => $destination));
+      watchdog('file', 'File %file (%realpath) could not be moved because the destination %destination is invalid. This may be caused by improper use of file_move() or a missing stream wrapper.', array(
+        '%file' => $source->getFileUri(),
+        '%realpath' => $realpath,
+        '%destination' => $destination)
+      );

Same as above.

+++ b/core/modules/file/file.moduleundefined
@@ -652,7 +669,10 @@ function file_file_download($uri, $field_type = 'file') {
-          $grants = array_merge($grants, array($module => module_invoke($module, 'file_download_access', $field, $entity, $file)));
+          $grants = array_merge($grants, array(
+            $module => module_invoke($module, 'file_download_access', $field, $entity, $file),
+            )
+          );

This indentation is weird. I would move the two closing parentheses together like this: ));

+++ b/core/modules/file/file.moduleundefined
@@ -706,7 +726,10 @@ function file_cron() {
-        watchdog('file system', 'Did not delete temporary file "%path" during garbage collection because it is in use by the following modules: %modules.', array('%path' => $file->getFileUri(), '%modules' => implode(', ', array_keys($references))), WATCHDOG_INFO);
+        watchdog('file system', 'Did not delete temporary file "%path" during garbage collection because it is in use by the following modules: %modules.', array(
+          '%path' => $file->getFileUri(),
+          '%modules' => implode(', ', array_keys($references)),
+          ), WATCHDOG_INFO);

Last line is indented two spaces too deep.

I have created patch for it.

• Unfortunately, the patch no longer applies:
  

  Checking patch core/modules/file/file.install...
  error: while searching for:
  function file_update_8000() {
    update_variables_to_config('file.settings', array(
      'file_description_type' => 'description.type',
      'file_description_length'=>'description.length',
      'file_icon_directory'=>'icon.directory',
    ));
  }
  
  
  error: patch failed: core/modules/file/file.install:233
  error: core/modules/file/file.install: patch does not apply

• When rerolling, please remember that, per the initiative instructions, we're using drush drupalcs, not drush coder.
• Also remember that adding @param and @return type hinting has been descoped. To make the patch easier to review and to make it more likely to get committed, please remove those changes. Everyone who wants to contribute here, please be sure you're following the (current) initiative instructions.

• +++ b/core/modules/file/file.module
  @@ -114,6 +123,7 @@ function file_load($fid, $reset = FALSE) {
    * @return \Drupal\file\FileUsage\FileUsageInterface.
  + *   A file entity.
  

  This isn't actually correct. It should be "A file usage backend.".

• Finally, there's some inconsistency in the way the patch handles line breaking arrays as arguments. Here's an example of how core generally does it, from core/tests/Drupal/Tests/Core/Routing/RouteCompilerTest.php:
  

  $route = new Route('/test/{something}/more/{here}', array(
    'here' => 'there',
  ));
  

re-rolled the patch

A few things here:

• Adding doxygen type hinting has been descoped from this initiative and moved to #1800046: [META] Add missing type hinting to core docblocks, fix Drupal.Commenting.FunctionComment.Missing*, so those changes need to be removed from the patch. Please remember to follow steps 6 and 7 in the meta issue.
• Make sure that the patch addresses all issues reported by Sniffer when run as described above.
• Please don't do anything in the patch but fix coding standards issues.
• What's that massive addition at the top of the patch?

Please address these issues so someone can productively perform a closer code review. Thanks!

Hi,

I had started working on coder reviews of File module.

I have created a patch which solves all coder errors for file core module.

Please review it.

Another try at patch

Another try at patch

Removed the errors in comment #23 and updating the patch.

Hi,

Please review the patch attached here which fixes all the coder reviews.

Patch no longer applies, tagging for reroll.

Hi LinL,

I have updated the patch with the required changes, Please review the patch attached.

15: file-module-pass-coder-review-1533236-15.patch queued for re-testing.

11: file-module-pass-coder-review-1533236-11.patch queued for re-testing.

12: file-module-pass-coder-review-1533236-12.patch queued for re-testing.

28: drupal-make_file_module_pass_coder_review-1533236-28.patch queued for re-testing.

30: drupal-make_file_module_pass_coder_review-1533236-30.patch queued for re-testing.

Looks like the patch needs another reroll following the PSR-4 changes.

More info here: https://groups.drupal.org/node/424758

Will try to reroll this one

I'll reroll it.

Looks like simple reroll is not enough, i'll unassign for now and see if i have time later.

ran the latest code against coder module.

The coder module seems to be making a wrong suggestion to replace t() with $t = get_t();

Thanks for all the work here so far. See #1518116-86: [meta] Make Core pass Coder Review. This issue is postponed until the meta issue is either closed or reopened.

Closing in favor of #2571965: [meta] Fix PHP coding standards in core. In this issue the coding standards will be fixed on a sniff-per-sniff basis rather than a module-per-module basis.