Clean-up file module test members - ensure property definition and use of camelCase naming convention

Here's an initial attempt. I also changed array(..) to [..] in various places. I know it is out of scope but it just looks neater.

I know it is out of scope but it just looks neater.

That makes it a pain to review the patch. There are about 30 tabs open in my browser right now for the parent meta.

For instance:

+++ b/core/modules/file/src/Tests/FileFieldTestBase.php
@@ -97,36 +102,36 @@ function createFileField($name, $entity_type, $bundle, $storage_settings = array
-  function attachFileField($name, $entity_type, $bundle, $field_settings = array(), $widget_settings = array()) {
-    $field = array(
+  function attachFileField($name, $entity_type, $bundle, $field_settings = [], $widget_settings = []) {
+    $field = [

As a reviewer, I have to figure out if you've renamed a variable in here to be camel case. Then I have to figure out whether to say, "Hey, can we not do the array thing?" Then you have to figure out whether you're annoyed with me. :-)

If it were a line with an in-scope change in it, then yay. Otherwise it's just kind of a pain.

Otherwise... Missed one.

class FileFieldValidateTest extends FileFieldTestBase {
  protected $field;
  protected $node_type;

$node_type needs the camelCase love, but it also needs some re-evaluation because I don't think it ever gets used.

I have removed the properties mentioned in #4.

@Mile23: I appreciate your reviews and I know it gets harder to review the patches with these changes. Thank you for taking the effort. I just thought it would be good to change them now while I am already touching the files as it is not likely they would get changed otherwise (as a part of it's own issue, for instance).

The reason I am doing this is it makes it a bit more readable. I know it is not an earth shattering improvement but every bit helps, IMHO. Arrays in function calls, and nested arrays become much more readable because of a different bracket style.

Lastly, this is just whimsical but I just found that if we replace every array definition, we reduce the source code size by about 350 KB. I know the benefits (1% size decrease) are not that much compared to the effort (~58K usages) and I am not going to even suggest that we do this. :) But I think we can do this with every change that we make.

Personally I like this kind of discussions. You can refer this stachoverflow answer.

While reviewing the patch in #5 I noticed this change:

+++ b/core/modules/file/src/Tests/FileFieldTestBase.php
@@ -22,15 +22,20 @@
-  public static $modules = array('node', 'file', 'file_module_test', 'field_ui');
+  public static $modules = ['node', 'file', 'file_module_test', 'field_ui'];

I am not accustomed to this kind of array declaration. Frankly speaking I did not even knew we can initialize array in PHP using [].

My point is coding style may vary among developers, but we should make the coding standard consistent in a project. I agree with @hussainweb's comment #5, but it simply break the coding consistency.

I am switching this to "Needs work", so that we can make the coding style consistent across Drupal.

@hussainweb you can always open an issue for such changes, and I am sure experienced community members will give their opinion :)

@subhojit777: I think achieving coding consistency in this aspect is very difficult, to put it mildly. There are lots of places in core that use the new array syntax and it seems to me that we almost always use this syntax for new blocks of code (few of them change existing lines).

I didn't create an issue because that might turn into a debate on which is better. Creating a separate issue for this would make it out to be a policy change, which is not worth it, I guess. But of course, I am open to it.

There is no difference between them in performance (unless you are counting saving five bytes as a performance gain). I think that in terms of readability, the new syntax is much better, especially when using arrays in function calls but of course, everyone has different preferences. Seeing that most new patches go in with this syntax, I am changing the lines in the patch which make sense (either in reducing the line length or if it is used as a argument to a function call).

If the only reason to set it to needs work was code consistency, I think we should switch back to needs review because like I said, the code is not at all consistent right now and this issue isn't going to solve it.

@hussainweb Just thought you should see this. But may be it is a testbot problem, otherwise it would also have failed here.

@subhojit777: It was a testbot problem. Couple of my patches failed with the same error and then passed after some time.

The array style ought not be changed in this issue. It is beyond the scope and will make it harder to be committed in this development phase.

If we allowed this, are we going to return to every issue in the parent meta, reopen them and change the array style? No.

Reverting all new array syntax.

Thanks @subhojit777 for pointing out this issue. I forgot where we discussed the new array syntax. I am adding the policy issue as reference here.

The patch in #11 applies cleanly, phpcs tells me that there are no camel case/underscore errors in the test classes, and all the out of scope array declaration changes have been reverted.

Thanks, @hussainweb!

....again. :-)

STILL removes all camel case errors. :-)

+++ b/core/modules/file/src/Tests/FileListingTest.php
@@ -23,11 +23,18 @@ class FileListingTest extends FileFieldTestBase {
     parent::setUp();
...
+    $this->adminUser = $this->drupalCreateUser(array('access files overview', 'bypass node access'));

@@ -58,12 +65,12 @@ protected function sumUsages($usage) {
+    $this->drupalLogin($this->adminUser);

This is super weird since it clashes with the already logged in user in FileFieldTestBase. I think we should have a new property for this user.

assigning & working on a fix as per @alexpott suggestion

@gobinathm Any help needed here..?

@chintan4u: It's been six days and no comments. I think you should assign it to yourself and work on it.

Nice job, @sachinwable!

It looks RTBC for me.

Thanks @sachinwable. Just a note for future patches. Always put an interdiff (unless you are just rerolling and not actually changing anything).

+++ b/core/modules/file/src/Tests/FileListingTest.php
@@ -23,11 +23,18 @@ class FileListingTest extends FileFieldTestBase {
+    $this->administratorUser = $this->drupalCreateUser(array('access files overview', 'bypass node access'));

This property does not exist - sure be $this->adminUser to be the same as the existing test.

Renaming it to adminUser.

Good!

Still applies, and I can use this one-liner to verify it fxes the lowerCamel problem: https://gist.github.com/paul-m/bdb4c44ecd1de8f934cd

Committed e53131d and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation to the issue summary.