Make Contact module pass Coder Review

That is correct according to the coding standards see http://drupal.org/coding-standards#naming

Although I don't know if this should actually be changed or not.

Errors in #1 don't show up anymore.
Only these:

FILE: /Sites/htdocs/d8review/core/modules/contact/contact.pages.inc
--------------------------------------------------------------------------------
FOUND 2 ERROR(S) AND 2 WARNING(S) AFFECTING 4 LINE(S)
--------------------------------------------------------------------------------
  84 | ERROR   | Expected "if (...) {\n"; found "if (...){\n"
 191 | WARNING | A comma should follow the last multiline array item. Found: ]
 248 | ERROR   | Expected "if (...) {\n"; found "if (...){\n"
 335 | WARNING | A comma should follow the last multiline array item. Found:
     |         | name
--------------------------------------------------------------------------------

Updated the patch to fix it.

I have checked the complete contact module, and it doesn't output any codesniffer errors.

About #5.1. and the output of the array, i dont really see the issue here, i think it more readable as done in the patch. I can agree with you if the array is done with

$rows[] = [[x, y, z]];

#5.2 - No need for splitting it up in two issues, since we're handling the file already and it's such small a fix.

When the patch for this issue is next re-rolled, please remember to include a blank line before the closing brace in the declaration of the class in the files ContactAuthenticatedUserTest.php and ContactPersonalTest.php.

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

I'll take a look at this and review it today

Just downloaded and applied the patch successfully.

contact.module looks good to me

contact.pages.inc looks good too

CategoryListController.php: should the function declarations within a Class have @param and @return documentation?

MessageFormController.php:

// For the e-mail message, clarify that the sender name is not verified;
// it could potentially clash with a username on this site.

not really sure what the point of the ';' is here, probably makes more sense to say "...sender name is not verified as it could potentially ..."
Other than that it's good

Message.php is good

ContactAuthenticatedUserTest.php: I'm not sure about how this stuff is meant to be written so can't really review it, but I think it's correct... OK, according to this php documentation http://php.net/manual/en/language.oop5.visibility.php if you declare a function within a class without declaring it as public, protected or private, then it IS public, so adding the keyword 'public' is probably best practice - i'd say this is all good.

ContactPersonalTest: This one is interesting.... not sure i can review it, it seems correct to me but can someone else please review

ContactSitewideTest: good

ContactUpgradePathTest: good

MessageEntityTest: good

ContactFieldsTest: good

ContactLinkTest: good

drupal-7.contact.database: good

contact_test_views: why do we need an empty file? The changes are good, but seems redundant.

When I run drupalcs on command line I still get a lot of errors, most of them seem to be due to:

/**
   * {@inheritdoc}
   */

Not starting with a capital letter, and not ending with a full stop (period). Has this been filed as an issue with Coder project?

The only other issues I see are the array declarations in ContactSitewideTest.php

I think we should split out the arrays onto multiple lines, the issues are on lines; 98, 111, 148, 219 and 276 - although I'm happy to leave these for the time being since no one's really sure what they are meant to look like :)

Great work @traviscarden, if you can convince me about those few things I'm not sure about (eg private vs protected) i'll mark it RTBC

This change is incorrect:

+  /**
+   * {@inheritdoc}
+   */
   public static function getInfo() {

On tests, the getInfo() method does *not* get any documentation. period. Nor setUp(), or tearDown(). If Coder is saying they need docs, Coder is wrong.

I also don't think generally that we are using camelCase for member variables in classes, are we? I'd like to know whether (a) that's the standard or (b) it's generally followed in Core before we try to "fix" this all over core.

+++ b/core/modules/contact/lib/Drupal/contact/Tests/ContactPersonalTest.php
@@ -26,22 +26,25 @@ class ContactPersonalTest extends WebTestBase {
    *
    * @var object
    */
-  private $admin_user;
+  protected $adminUser;

RE #18:
1. Yes that other issue would be good to decide on a more consistent standard, but it isn't decided and has been sitting for months. The current standard is at https://drupal.org/node/325974

2. What I was trying to say was that camelCase for class properties is I think one of those "standards" that has been formally adopted but that most of core doesn't comply with (I think). In cases like that we need to consider whether complying with the adopted standard is a better idea than revising the standard. I'd like to see a survey of class properties in Core to see how many are camel and how many use underscores... but my guess is that we're mostly using underscores and it would be better to change the standard to match what we're doing than to try to enforce a standard that no one follows.

Attaching the patch which fixes all coder reviews. Please review it.

I applied the patch and got an issue with the following:
Patch Failed
core/modules/contact/lib/Drupal/contact/Tests/ContactPersonalTest.php:50
core/modules/contact/lib/Drupal/contact/Tests/ContactSitewideTest.php:118
patch does not apply
core/modules/contact/lib/Drupal/contact/Tests/ContactPersonalTest.php
core/modules/contact/lib/Drupal/contact/Tests/ContactSitewideTest.php

I will look into it and repost soon.

I found a couple of things that didn't tie up in the ContactPersonalTest portion of the patch that I found different to the file itself.

1. Line 58 of the 8.x file has four parameters in it were as the path only has three:

-    $this->admin_user = $this->drupalCreateUser(array('administer contact forms', 'administer users', 'administer account settings'));

$this->admin_user = $this->drupalCreateUser(array('administer contact forms', 'administer users', 'administer account settings', 'access site reports'));

2. Line 90 to 102 in the 8.x file is entirely missing from the patch

// Check there was no problems raised during sending.
    $this->drupalLogout();
    $this->drupalLogin($this->admin_user);
    // Verify that the correct watchdog message has been logged.
    $this->drupalGet('/admin/reports/dblog');
    $placeholders = array(
    '@sender_name' => $this->web_user->username,
    '@sender_email' => $this->web_user->getEmail(),
    '@recipient_name' => $this->contact_user->getUsername()
    );
    $this->assertText(String::format('@sender_name (@sender_email) sent @recipient_name an e-mail.',    $placeholders));
    }

I started throwing myself down a rabit hole so I stopped looking because I am not sure whether these are errors in the patch or a result of my naive understaing of patches.

Ok so I slept on it and finished an essay and had an idea, so I will try to create a new patch by removing and redoing the bits that I have problems with.

I have created a new patch for this issue by.

• Removing the parts of the patch that were causing the issue
• Applying the patch
• Redoing the parts of the patch that were causing the issues
• Finally running the coder review tool again to double check

Attempt two to pass the tests.

reroll, and update.

missed that one.

Patch #36, no longer applies. It needs reroll.

reroll + a few more coder fixes.

sorry, bad reroll.

I am reviewing/rerolling this now. Patch in #40 does not apply to HEAD right now.

I've rerolled the patch manually.

It failed with the "Too many connections" errors. I'll retest to see how it goes.

Fixing the test, and a couple more coder issues.

@herom, is there anything else I can do to help on this? i.e. What needs to happen now?

You can do a review by running Coder against the contact module, to see if any errors are remaining. The parent issue explains how to do that:

Clone TravisCarden's fork of Coder module, which has been modified for this initiative (git clone --branch=issue-1518116 git@github.com:TravisCarden/coder.git) and install it according to the project's instructions.
Run Coder Sniffer against the relevant directory or files, e.g., drush dcs includes/.

Just a note that Coder Sniffer might mark some /** @var lines as error, but that is a false-positive.

You can also look at the patch to see if all the changes are correct.

If you didn't find any issues with the above, you can mark the issue as RTBC.

Re-rolled patch.

Only conflict was addition of the $third_party_settings argument to the addContactForm function:

< -  function addContactForm($id, $label, $recipients, $reply, $selected) {
< +  public function addContactForm($id, $label, $recipients, $reply, $selected) {
---
> -  function addContactForm($id, $label, $recipients, $reply, $selected, $third_party_settings = []) {
> +  public function addContactForm($id, $label, $recipients, $reply, $selected, $third_party_settings = []) {

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.