Files: 
CommentFileSizeAuthor
#59 interdiff.txt5.13 KBalansaviolobo
#59 make_file_module_pass-1533236-59.patch136.46 KBalansaviolobo
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,260 pass(es).
[ View ]
#57 interdiff.txt101.16 KBalansaviolobo
#57 make_file_module_pass-1533236-57.patch141.07 KBalansaviolobo
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed.
[ View ]
#56 interdiff.txt822 bytesalansaviolobo
#56 make_file_module_pass-1533236-56.patch84.38 KBalansaviolobo
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,683 pass(es).
[ View ]
#54 interdiff.txt357 bytesalansaviolobo
#54 make_file_module_pass-1533236-54.patch83.68 KBalansaviolobo
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,662 pass(es), 1 fail(s), and 2 exception(s).
[ View ]
#52 interdiff.txt629 bytesalansaviolobo
#52 make_file_module_pass-1533236-52.patch83.44 KBalansaviolobo
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: failed during invocation of run-tests.sh.
[ View ]
#50 interdiff.txt345 bytesalansaviolobo
#50 make_file_module_pass-1533236-50.patch82.87 KBalansaviolobo
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: failed during invocation of run-tests.sh.
[ View ]
#48 make_file_module_pass-1533236-48.patch82.64 KBalansaviolobo
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: failed during invocation of run-tests.sh.
[ View ]
#30 drupal-make_file_module_pass_coder_review-1533236-30.patch115.34 KBJalandhar
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch drupal-make_file_module_pass_coder_review-1533236-30.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#28 drupal-make_file_module_pass_coder_review-1533236-28.patch125.39 KBJalandhar
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch drupal-make_file_module_pass_coder_review-1533236-28.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#15 file-module-pass-coder-review-1533236-15.patch32.7 KBsaki007ster
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch file-module-pass-coder-review-1533236-15.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#12 file-module-pass-coder-review-1533236-12.patch29.38 KBschoobidoo
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch file-module-pass-coder-review-1533236-12.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#11 file-module-pass-coder-review-1533236-11.patch27.57 KBdeepakaryan1988
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch file-module-pass-coder-review-1533236-11.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#8 interdiff.txt4.09 KBpfrenssen
#7 drupal-core-make-file-module-pass-coder-review-1533236-4.patch24.49 KBsandergo90
PASSED: [[SimpleTest]]: [MySQL] 58,624 pass(es).
[ View ]
#4 drupal-core-make-file-module-pass-coder-review-1533236-4.patch25.14 KBsandergo90
PASSED: [[SimpleTest]]: [MySQL] 58,585 pass(es).
[ View ]

Comments

scottalan’s picture

Assigned:Unassigned» scottalan
TravisCarden’s picture

Status:Active» Postponed

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

sphism’s picture

Status:Postponed» Active

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

sandergo90’s picture

StatusFileSize
new25.14 KB
PASSED: [[SimpleTest]]: [MySQL] 58,585 pass(es).
[ View ]

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

sandergo90’s picture

Status:Active» Needs review
pfrenssen’s picture

Status:Needs review» Needs work
+++ 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.

sandergo90’s picture

Status:Needs work» Needs review
StatusFileSize
new24.49 KB
PASSED: [[SimpleTest]]: [MySQL] 58,624 pass(es).
[ View ]

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

pfrenssen’s picture

StatusFileSize
new4.09 KB

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.

pfrenssen’s picture

Status:Needs review» Needs work

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.

deepakaryan1988’s picture

Assigned:scottalan» deepakaryan1988
Issue summary:View changes
deepakaryan1988’s picture

Status:Needs work» Needs review
StatusFileSize
new27.57 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch file-module-pass-coder-review-1533236-11.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

I have created patch for it.

schoobidoo’s picture

StatusFileSize
new29.38 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch file-module-pass-coder-review-1533236-12.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

There were some further fixes needed to get

drush coder core/modules/file/ --no-empty

running without change proposals. I attached a patch that is based on #11 and fixes the remaining coder warnings.

TravisCarden’s picture

Status:Needs review» Needs work
  • 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',
    ));
saki007ster’s picture

saki007ster’s picture

StatusFileSize
new32.7 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch file-module-pass-coder-review-1533236-15.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

re-rolled the patch

saki007ster’s picture

Status:Needs work» Needs review
TravisCarden’s picture

Status:Needs review» Needs work

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, 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!

Jalandhar’s picture

Assigned:saki007ster» Jalandhar

Hi,

I had started working on coder reviews of File module.

Jalandhar’s picture

Status:Needs work» Needs review
StatusFileSize
new123.33 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,269 pass(es), 4 fail(s), and 9 exception(s).
[ View ]

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

Please review it.

Status:Needs review» Needs work

The last submitted patch, 19: drupal-make_file_module_pass_coder_review-1533236-19.patch, failed testing.

Jalandhar’s picture

Status:Needs work» Needs review
StatusFileSize
new123.84 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,331 pass(es), 3 fail(s), and 8 exception(s).
[ View ]

Another try at patch

Status:Needs review» Needs work

The last submitted patch, 21: drupal-make_file_module_pass_coder_review-1533236-21.patch, failed testing.

Jalandhar’s picture

StatusFileSize
new124.52 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,151 pass(es), 3 fail(s), and 6 exception(s).
[ View ]

Another try at patch

Jalandhar’s picture

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, 23: drupal-make_file_module_pass_coder_review-1533236-23.patch, failed testing.

Jalandhar’s picture

Status:Needs work» Needs review
StatusFileSize
new125.36 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch drupal-make_file_module_pass_coder_review-1533236-26.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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

Status:Needs review» Needs work

The last submitted patch, 26: drupal-make_file_module_pass_coder_review-1533236-26.patch, failed testing.

Jalandhar’s picture

Status:Needs work» Needs review
StatusFileSize
new125.39 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch drupal-make_file_module_pass_coder_review-1533236-28.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Hi,

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

LinL’s picture

Status:Needs review» Needs work
Issue tags:+Needs reroll

Patch no longer applies, tagging for reroll.

Jalandhar’s picture

StatusFileSize
new115.34 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch drupal-make_file_module_pass_coder_review-1533236-30.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Hi LinL,

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

Jalandhar’s picture

Status:Needs work» Needs review
Jalandhar’s picture

Assigned:Jalandhar» Unassigned
rymo’s picture

The last submitted patch, 15: file-module-pass-coder-review-1533236-15.patch, failed testing.

rymo’s picture

rymo’s picture

rymo’s picture

rymo’s picture

The last submitted patch, 11: file-module-pass-coder-review-1533236-11.patch, failed testing.

The last submitted patch, 12: file-module-pass-coder-review-1533236-12.patch, failed testing.

The last submitted patch, 28: drupal-make_file_module_pass_coder_review-1533236-28.patch, failed testing.

Status:Needs review» Needs work

The last submitted patch, 30: drupal-make_file_module_pass_coder_review-1533236-30.patch, failed testing.

LinL’s picture

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

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

g3r4’s picture

Assigned:Unassigned» g3r4

Will try to reroll this one

bfr’s picture

Assigned:g3r4» bfr

I'll reroll it.

bfr’s picture

Assigned:bfr» Unassigned

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

harijari’s picture

Assigned:Unassigned» harijari
alansaviolobo’s picture

Status:Needs work» Needs review
StatusFileSize
new82.64 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: failed during invocation of run-tests.sh.
[ View ]

Status:Needs review» Needs work

The last submitted patch, 48: make_file_module_pass-1533236-48.patch, failed testing.

alansaviolobo’s picture

Status:Needs work» Needs review
StatusFileSize
new82.87 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: failed during invocation of run-tests.sh.
[ View ]
new345 bytes

Status:Needs review» Needs work

The last submitted patch, 50: make_file_module_pass-1533236-50.patch, failed testing.

alansaviolobo’s picture

Status:Needs work» Needs review
StatusFileSize
new83.44 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: failed during invocation of run-tests.sh.
[ View ]
new629 bytes

Status:Needs review» Needs work

The last submitted patch, 52: make_file_module_pass-1533236-52.patch, failed testing.

alansaviolobo’s picture

Status:Needs work» Needs review
StatusFileSize
new83.68 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,662 pass(es), 1 fail(s), and 2 exception(s).
[ View ]
new357 bytes

Status:Needs review» Needs work

The last submitted patch, 54: make_file_module_pass-1533236-54.patch, failed testing.

alansaviolobo’s picture

Status:Needs work» Needs review
StatusFileSize
new84.38 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,683 pass(es).
[ View ]
new822 bytes
alansaviolobo’s picture

Assigned:harijari» alansaviolobo
StatusFileSize
new141.07 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed.
[ View ]
new101.16 KB

ran the latest code against coder module.

Status:Needs review» Needs work

The last submitted patch, 57: make_file_module_pass-1533236-57.patch, failed testing.

alansaviolobo’s picture

Status:Needs work» Needs review
StatusFileSize
new136.46 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,260 pass(es).
[ View ]
new5.13 KB

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

xjm’s picture

Version:8.0.x-dev» 8.1.x-dev
Assigned:alansaviolobo» Unassigned
Priority:Normal» Minor
Status:Needs review» Postponed
Issue tags:-Novice, -Needs reroll

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.