Make dblog module pass Coder Review

I started working on this sine the patch in #1326600 was committed.

Patch attached.

Coder reports no problems on minor.

Code Sniffer reports 3 errors:

FILE: /var/www/dev/drupal8.local/public_html/core/modules/dblog/dblog.module
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
 172 | ERROR | If the line declaring an array spans longer than 80 characters,
     |       | each element should be broken into its own line
--------------------------------------------------------------------------------

I don't think changing this would improve readability - leave it as it is?

FILE: ...cal/public_html/core/modules/dblog/lib/Drupal/dblog/Tests/DBLogTest.php
--------------------------------------------------------------------------------
FOUND 2 ERROR(S) AFFECTING 2 LINE(S)
--------------------------------------------------------------------------------
 180 | ERROR | Concatenating translatable strings is not allowed, use
     |       | placeholders instead and only one string literal
 190 | ERROR | Concatenating translatable strings is not allowed, use
     |       | placeholders instead and only one string literal
--------------------------------------------------------------------------------

I am not really sure what to do about those two - writing "'" directly in the string would make it hard to read.
Adding quote as an argument would not really make sense either, since it is not dynamic.
I would just leave it the way it is.
Suggestions?

#2: drupal-dblog-coding-standards-1533228-2.patch queued for re-testing.

When this gets re-rolled, we need to add a blank line after the last method and before the closing brace of the declaration of DbLogTest.php.

#2: drupal-dblog-coding-standards-1533228-2.patch queued for re-testing.

New patch included.

The only two remaining errors are:

FILE: ...v/drupal8.local/core/modules/dblog/lib/Drupal/dblog/Tests/DBLogTest.php
--------------------------------------------------------------------------------
FOUND 2 ERROR(S) AFFECTING 2 LINE(S)
--------------------------------------------------------------------------------
178 | ERROR | Concatenating translatable strings is not allowed, use
| | placeholders instead and only one string literal
188 | ERROR | Concatenating translatable strings is not allowed, use
| | placeholders instead and only one string literal
--------------------------------------------------------------------------------

As stated above, I would just leave those be.

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

Hi,
Please find the updated patch and review.

Thanks, @visabhishek! A few comments:

1. +++ b/core/modules/dblog/dblog.admin.inc
   @@ -20,7 +20,7 @@ function dblog_filters() {
   -    $types[$type] = t($type);
   +    $types[$type] = $type;
   

   We'd better leave this alone. I assume Coder Sniffer complained about it--and rightly so, because it's not a proper use of the translation system--but removing it will have functional changes.

2. +++ b/core/modules/dblog/lib/Drupal/dblog/Tests/DbLogTest.php
   @@ -28,15 +28,18 @@ class DbLogTest extends WebTestBase {
   +  /**
   +   * A user without any permissions.
   +   */
   

   Per Drupal coding standards, "In a class, if you are overriding or implementing a method from a base class or interface, and the documentation should be exactly the same as the base class/interface method, use the following docblock syntax:"

     /**
      * {@inheritdoc}
      */

3. +++ b/core/modules/dblog/lib/Drupal/dblog/Tests/DbLogTest.php
   @@ -45,12 +48,22 @@ public static function getInfo() {
   +   */
   

   This one, too, of course:

     /**
      * {@inheritdoc}
      */

4. +++ b/core/modules/dblog/lib/Drupal/dblog/Tests/DbLogTest.php
   @@ -45,12 +48,22 @@ public static function getInfo() {
   +          'access administration pages',
   ...
   +        );
   

   The correct way to break and indent this would actually be like so:

       $this->bigUser = $this->drupalCreateUser(array(
         'administer site configuration',
         'access administration pages',
         'access site reports',
         'administer users',
       ));

5. +++ b/core/modules/dblog/lib/Drupal/dblog/Tests/Views/ViewsIntegrationTest.php
   @@ -32,11 +32,14 @@ class ViewsIntegrationTest extends ViewUnitTestBase {
   +  /**
   +   * Modules to enable.
   +   */
   

   inheritdoc again.

There's also one valid error left after the above patch. (There are two warnings that should be ignored.) Here's another patch to bring it home.

Thanks TravisCarden, i will update my other patches as per your comment #13.

Re-tagging for sprint.

We are working on it @dankh, @leonevers.

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.