Make syslog module pass Coder Review

Completed.

And again #1533202-4: Make config module pass Coder Review. :) Thanks for taking on so many patches, @FluxSauce!

#2: syslog_standardized-1533392.patch queued for re-testing.

The patch passed upon re-test. Updated summary; thanks for the feedback @TravisCarden!

1. +++ b/core/modules/syslog/syslog.module
   +++ b/core/modules/syslog/syslog.module
   @@ -3,18 +3,15 @@
   
   @@ -3,18 +3,15 @@
    /**
     * @file
     * Redirects logging messages to syslog.
   + *
   + * Note that LOG_LOCAL0 through LOG_LOCAL7 are not available on Windows, so we
   + * check for availability. If LOG_LOCAL0 is defined by the PHP environment, we
   + * set that as the default; if not, we use LOG_USER.
   + *
   + * @see http://php.net/manual/function.syslog.php
     */
    
    if (defined('LOG_LOCAL0')) {
   -  /**
   -   * Sets the proper logging facility.
   -   *
   -   * Note that LOG_LOCAL0 through LOG_LOCAL7 are not available on Windows, so we
   -   * check for availability. If LOG_LOCAL0 is defined by the PHP environment, we
   -   * set that as the default; if not, we use LOG_USER.
   -   *
   -   * @see http://php.net/manual/function.syslog.php
   -   */
      define('DEFAULT_SYSLOG_FACILITY', LOG_LOCAL0);
    }
    else {
   

   I don't think this is the right change. This dockblock is documenting the DEFAULT_SYSLOG_FACILITY constant, not the file. I don't see that any of the review tools complained about where it was to begin with. What made you change it?

2. +++ b/core/modules/syslog/syslog.module
   @@ -70,10 +71,11 @@ function syslog_form_system_logging_settings_alter(&$form, &$form_state) {
   + *   Array of facilities, keyed by LOG_LOCAL*
   

   This line needs a period at the end of it.

3. +++ b/core/modules/syslog/syslog.test
   @@ -9,14 +9,23 @@
   +  /**
   +   * Describe the test case to Drupal.
   +   * @return array
   +   *   Standardized test case description.
   +   */
   

   This should just be

     /**
      * Implement getInfo().
      */
   

4. +++ b/core/modules/syslog/syslog.test
   @@ -9,14 +9,23 @@
   -      'name' => 'Syslog functionality',
   -      'description' => 'Test syslog settings.',
   -      'group' => 'Syslog'
   +      'name' => t('Syslog functionality'),
   +      'description' => t('Test syslog settings.'),
   +      'group' => t('Syslog'),
   

   I don't think these need to be translated. What made you change this?

5. +++ b/core/modules/syslog/syslog.test
   @@ -9,14 +9,23 @@
   +  /**
   +   * Common set up for all tests within the case.
   +   */
   

   This should just be

     /**
      * Implement setUp().
      */
   

I don't think this is the right change... I don't see that any of the review tools complained about where it was to begin with.

It was reported with CodeSniffer.

drupalcs core/modules/syslog

FILE: /media/sf_sandbox/core/modules/syslog/syslog.module
--------------------------------------------------------------------------------
FOUND 8 ERROR(S) AFFECTING 7 LINE(S)
--------------------------------------------------------------------------------
  9 | ERROR | Inline doc block comments are not allowed; use "// Comment"
    |       | instead

What made you change it?

The content of the comment, specifically referring to LOG_LOCAL* availability and providing context for syslog, apply to the entire module and not just that if/else statement at the top. I felt that it was more appropriate to provide that context at a high-level rather than sticking in an if statement. I can switch it back, but I don't feel that it's right where it is right now.

I don't think these need to be translated. What made you change this?

I had assumed that strings needed to be translatable; as it's not public facing, I guess there's no justification and I've reverted.

I've removed the doc block from getInfo() and setUp() based on Drupal SimpleTest coding standards, which I wasn't aware of previously.

Thanks for the review, I've attached an amended version.

Okay, thanks for the explanation.

1. +++ b/core/modules/syslog/syslog.module
   @@ -3,18 +3,15 @@
    if (defined('LOG_LOCAL0')) {
   -  /**
   -   * Sets the proper logging facility.
   -   *
   -   * Note that LOG_LOCAL0 through LOG_LOCAL7 are not available on Windows, so we
   -   * check for availability. If LOG_LOCAL0 is defined by the PHP environment, we
   -   * set that as the default; if not, we use LOG_USER.
   -   *
   -   * @see http://php.net/manual/function.syslog.php
   -   */
      define('DEFAULT_SYSLOG_FACILITY', LOG_LOCAL0);
    }
   

   This comment is specifically explaining what the "if" statement is doing. If anything, I think it should be moved above the "if" statement, but I don't think we want to move it away from the code it's actually documenting. According to the coding standards, it is proper to document constants with docblocks. I take CodeSniffer is complaining because it's inside a control structure ("inline"). Let's get someone else's feedback on this.

2. Thank you for confirming the SimpleTest coding standards. If drupalcs complains about the lack of PHPDocs, please file an issue to fix it and report back here.

Please update the issue summary as appropriate. Thanks!

Summarizing work completed

Oops.

Summary updated, submitted #1545476: false positive "Missing function doc comment" error on Simple Test inherited methods.

at first it is shown me the following errors

FILE: /home/dina/www/drupal--8-sandbox/core/modules/syslog/syslog.module
--------------------------------------------------------------------------------
FOUND 8 ERROR(S) AFFECTING 7 LINE(S)
--------------------------------------------------------------------------------
  9 | ERROR | Inline doc block comments are not allowed; use "// Comment"
    |       | instead
 32 | ERROR | If the line declaring an array spans longer than 80 characters,
    |       | each element should be broken into its own line
 62 | ERROR | Array closing indentation error, expected 4 spaces but found 5
 74 | ERROR | Expected 2 space(s) before asterisk; 1 found
 75 | ERROR | Expected 2 space(s) before asterisk; 1 found
 76 | ERROR | Expected 2 space(s) before asterisk; 1 found
 76 | ERROR | Missing comment for @return statement
 77 | ERROR | Expected 2 space(s) before asterisk; 1 found
--------------------------------------------------------------------------------

i applied the patch in #8 it doesn't show any errors

so i assumed that there is no error

correct me if i was wrong

Ummm... In the patch, what is this supposed to mean?

  * @return array
+ *   Array of facilities, keyed by LOG_LOCAL*.
  */
 function syslog_facility_list() {

I don't understand from this what the keys are.

Also, regarding comment #9 above, I agree with Travis. That doc block is documenting the define, and if Coder/Sniffer/whatever is complaining, the complaint is wrong, not the code/comment.

And regarding the review in #12, ... do we have review instructions for these patches? In any case, coder/sniffer/whatever needs to be verified, after the patch has been applied (I don't care about before), to *all* the PHP files in the modules/syslog directory (and subdirectories).

I've attached a new version of the patch.

I disagree with the reasoning regarding the commenting at the beginning of syslog.module, but I'll leave it for consensus sake.

This is how it's currently written:

if (defined('LOG_LOCAL0')) {
  /**
   * Sets the proper logging facility.
   *
   * Note that LOG_LOCAL0 through LOG_LOCAL7 are not available on Windows, so we
   * check for availability. If LOG_LOCAL0 is defined by the PHP environment, we
   * set that as the default; if not, we use LOG_USER.
   *
   * @see http://php.net/manual/function.syslog.php
   */
  define('DEFAULT_SYSLOG_FACILITY', LOG_LOCAL0);
}
else {
  define('DEFAULT_SYSLOG_FACILITY', LOG_USER);
}

This is how I feel it should be written; more contextually accurate and doesn't trigger PHP_CodeSniffer.

// Sets the proper logging facility.
// @see http://php.net/manual/function.syslog.php 
if (defined('LOG_LOCAL0')) {
  // LOG_LOCAL0 through LOG_LOCAL7 are not available on Windows;
  // if LOG_LOCAL0 is defined by the PHP environment, use as the default.
  define('DEFAULT_SYSLOG_FACILITY', LOG_LOCAL0);
}
else {
  // Windows compatible logging facility.
  define('DEFAULT_SYSLOG_FACILITY', LOG_USER);
}

Updating with false positives.

The problem with the idea in #14 is that it doesn't put a /** documentation block before the define(), meaning that there is no official documentation for that define, which we would like to have. If code sniffer is complaining, that that is a bug in code sniffer, IMO.

@jhodgdon I see what you're saying; Documenting constants and variables doesn't cover this exactly; defines should be documented with the /** docblock, but it doesn't (at least that I can see) mention explicitly how to indent in this situation. I don't want to point fingers at code sniffer until I can cite exactly what's wrong :-)

This is such a rare case (once in all of core, I believe) that I don't think we want to document it on node/1354. But we in general make docblocks match the indentation of the code they are documenting, and definitely all in-code comments in general get indented to the same level as the code they are commenting, so I think this way of doing it makes sense, even if Code Sniffer doesn't.

Got it, I'll consider it an edge case. If I run into it again, I'll report it to drupalcs.

My last patch didn't change that section, so it's still good.

Updating with progress and consensus.

The latest dev version of drupalcs doesn't complain at all anymore. I've updated the summary.

Some of the code referred to here - the DEFAULT_SYSLOG_FACILITY definition - isn't actually used.

// Sets the proper logging facility.
// @see http://php.net/manual/function.syslog.php
if (defined('LOG_LOCAL0')) {
  // LOG_LOCAL0 through LOG_LOCAL7 are not available on Windows;
  // if LOG_LOCAL0 is defined by the PHP environment, use as the default.
  define('DEFAULT_SYSLOG_FACILITY', LOG_LOCAL0);
}
else {
  // Windows compatible logging facility.
  define('DEFAULT_SYSLOG_FACILITY', LOG_USER);
}

There's an issue to have that code removed: #1618178: Remove redundant DEFAULT_SYSLOG_FACILITY definition from Syslog

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

Updated issue summary.

I have created a patch. Please review.

I have updated this patch as per https://drupal.org/comment/8601579#comment-8601579.

Patch #27 needs reroll.

Reroll of #27.

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.