Make Block module pass Coder Review

+++ b/core/modules/block/block.api.phpundefined
@@ -163,7 +163,7 @@ function hook_block_configure($delta = '') {
-      '#options' => drupal_map_assoc(array(2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 25, 30)),
+      '#options' => drupal_map_assoc(array_merge(range(2, 20), array(25, 30))),

but worse in performance ?

Since that's just example code, I'd recommend shortening it to whatever's in core elsewhere that it's not complaining about. The new code definitely raises more questions than it answers, which is particularly a problem since it is code intended to explain to new developers how the code works. :)

i have tested your latest patch in #6 it is showing the same error

FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
 335 | ERROR | global variables should start with a single underscore followed
     |       | by the module and another underscore

Core is exempt from that rule, afaik.

Now, i don't get the error on #7
Here's an updated patch with more fixes.

FILE: /Sites/htdocs/d8review/core/modules/block/block.module
--------------------------------------------------------------------------------
FOUND 0 ERROR(S) AND 2 WARNING(S) AFFECTING 2 LINE(S)
--------------------------------------------------------------------------------
 862 | WARNING | Line exceeds 80 characters; contains 81 characters
 863 | WARNING | Line exceeds 80 characters; contains 84 characters
--------------------------------------------------------------------------------

Only these two warnings left.
Fixing these two warnings makes the comments less readable (a lot). What's the best practice here: ignore the warnings if it's more readable, or be strict about the coding rules?

It appears that the patch in #10 does not include any changes to the BlockTest.php file. I wonder why that is.

When this patch is next re-rolled, we need to remember to change class members to use camel case. Hence, members like admin_user need to be changed to adminUser.

Also, we need to check all tests to make sure that there is a blank line after the class declaration and another before the closing brace of the class declaration.

I am not sure if either of these types of issues are being flagged in the Coder Review process.

Finally, when this patch is re-rolled, all type hinting of @param and @return directives should be removed. They instead should be added as a separate patch to #1807674: Add missing type hinting to Block module docblocks.

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

Updated summary.

I have created a patch after passing through coder. Please review.

16: drupal-make_block_module_pass_coder_review-1522384-16.patch queued for re-testing.

Here is a reworking. The following errors still show up but there's some reasons we can't fix them:

FILE: ...stom_block/lib/Drupal/custom_block/Controller/CustomBlockController.php
--------------------------------------------------------------------------------
FOUND 0 ERROR(S) AND 1 WARNING(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
 49 | WARNING | Line exceeds 80 characters; contains 81 characters
--------------------------------------------------------------------------------

This is a case where we declare the PSR-0 namespace of a class.

FILE: ...upal/custom_block/Plugin/Menu/LocalAction/CustomBlockAddLocalAction.php
--------------------------------------------------------------------------------
FOUND 0 ERROR(S) AND 1 WARNING(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
 5 | WARNING | Line exceeds 80 characters; contains 83 characters
--------------------------------------------------------------------------------

This is a case where we declare the PSR-0 namespace of a class.

FILE: ...om_block/lib/Drupal/custom_block/Tests/CustomBlockTranslationUITest.php
--------------------------------------------------------------------------------
FOUND 0 ERROR(S) AND 2 WARNING(S) AFFECTING 2 LINE(S)
--------------------------------------------------------------------------------
 59 | WARNING | Line exceeds 80 characters; contains 101 characters
 96 | WARNING | Line exceeds 80 characters; contains 96 characters
--------------------------------------------------------------------------------

This is a case where we declare the PSR-0 namespace of a class.

FILE: .../Sites/drupal8/core/modules/block/lib/Drupal/block/Annotation/Block.php
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
 33 | ERROR | Class property $admin_label should use lowerCamel naming without
    |       | underscores
--------------------------------------------------------------------------------

This is essentially an API change if I understand correctly so might be worth bringing up in its own issue.

FILE: .../Sites/drupal8/core/modules/block/lib/Drupal/block/BlockListBuilder.php
--------------------------------------------------------------------------------
FOUND 2 ERROR(S) AFFECTING 2 LINE(S)
--------------------------------------------------------------------------------
 339 | ERROR | Line indented incorrectly; expected 23 spaces, found 6
 340 | ERROR | Line indented incorrectly; expected 25 spaces, found 8
--------------------------------------------------------------------------------

Seems coder doesn't know what to do with a usort function? This looks like proper indentation to me.

FILE: ...rupal8/core/modules/block/lib/Drupal/block/Plugin/Type/BlockManager.php
--------------------------------------------------------------------------------
FOUND 0 ERROR(S) AND 1 WARNING(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
 51 | WARNING | Line exceeds 80 characters; contains 84 characters
--------------------------------------------------------------------------------

This is a case where we declare the PSR-0 namespace of a class.

core/modules/block/tests/Drupal/block/Tests/BlockBaseTest.php was updated. Reroll and fixed error from #23.

1. +++ b/core/modules/block/lib/Drupal/block/BlockFormController.php
   @@ -290,9 +290,7 @@ public function validate(array $form, array &$form_state) {
   -    $settings = array(
   -      'values' => &$form_state['values']['settings']
   -    );
   +    $settings = array('values' => &$form_state['values']['settings']);
   

   This is a tricky bit of code, and its much clearer when split onto multiple lines like this

2. +++ b/core/modules/block/lib/Drupal/block/BlockFormController.php
   @@ -327,9 +325,7 @@ public function submit(array $form, array &$form_state) {
   -      'options' => array(
   -        'query' => array('block-placement' => drupal_html_class($this->entity->id()))
   -      ),
   +      'options' => array('query' => array('block-placement' => drupal_html_class($this->entity->id()))),
   

   Similarly, I don't find this more readable.

3. +++ b/core/modules/block/lib/Drupal/block/BlockPluginBag.php
   @@ -44,8 +44,6 @@ public function __construct(PluginManagerInterface $manager, $instance_id, array
      /**
       * {@inheritdoc}
   -   *
   -   * @return \Drupal\block\BlockPluginInterface
   

   This is done on purpose for typehinting. Please revert this change.

4. +++ b/core/modules/block/lib/Drupal/block/BlockPluginInterface.php
   @@ -59,7 +59,8 @@ public function build();
   +   * @todo This does not set a value in \Drupal::config(), so the name is
   +   * confusing.
   

   If a @todo is multiline, the other lines must be indented 2 extra spaces

5. +++ b/core/modules/block/tests/Drupal/block/Tests/BlockConfigEntityUnitTest.php
   @@ -22,14 +22,16 @@ class BlockConfigEntityUnitTest extends UnitTestCase {
   -   * @var \Drupal\Core\Entity\EntityTypeInterface|\PHPUnit_Framework_MockObject_MockObject
   +   * @var \Drupal\Core\Entity\EntityTypeInterface
   +   *   \PHPUnit_Framework_MockObject_MockObject
   ...
   -   * @var \Drupal\Core\Entity\EntityManagerInterface|\PHPUnit_Framework_MockObject_MockObject
   +   * @var \Drupal\Core\Entity\EntityManagerInterface
   +   *   \PHPUnit_Framework_MockObject_MockObject
   
   @@ -43,7 +45,8 @@ class BlockConfigEntityUnitTest extends UnitTestCase {
   -   * @var \Drupal\Component\Uuid\UuidInterface|\PHPUnit_Framework_MockObject_MockObject
   +   * @var \Drupal\Component\Uuid\UuidInterface
   +   *   \PHPUnit_Framework_MockObject_MockObject
   
   +++ b/core/modules/block/tests/Drupal/block/Tests/Plugin/views/display/BlockTest.php
   @@ -19,24 +19,30 @@ class BlockTest extends UnitTestCase {
   -   * @var \Drupal\views\ViewExecutable|\PHPUnit_Framework_MockObject_MockObject
   +   * @var \Drupal\views\ViewExecutable
   +   *   \PHPUnit_Framework_MockObject_MockObject
   ...
   -   * @var \Drupal\views\Plugin\Block\ViewsBlock|\PHPUnit_Framework_MockObject_MockObject
   +   * @var \Drupal\views\Plugin\Block\ViewsBlock
   +   *   \PHPUnit_Framework_MockObject_MockObject
   ...
   -   * @var \Drupal\block\Plugin\views\display\Block|\PHPUnit_Framework_MockObject_MockObject
   +   * @var \Drupal\block\Plugin\views\display\Block
   +   *   \PHPUnit_Framework_MockObject_MockObject
   

   This is no longer valid. Put it back to one line and keep the | please

Thanks for the review!

+++ b/core/modules/block/lib/Drupal/block/BlockFormController.php
@@ -290,8 +290,8 @@ public function validate(array $form, array &$form_state) {
-    $settings = array(
-      'values' => &$form_state['values']['settings']
+    $settings = array('values' =>
+      &$form_state['values']['settings'],
     );

@@ -327,8 +327,8 @@ public function submit(array $form, array &$form_state) {
-      'options' => array(
-        'query' => array('block-placement' => drupal_html_class($this->entity->id()))
+      'options' => array('query' =>
+        array('block-placement' => drupal_html_class($this->entity->id())),

This still defeats the purpose of multiline arrays, since it will look *really* weird if we ever need to add a new line to this. Thanks for making the other changes, but I still think this should just be left alone.

Okay. We should still add the comma though, no?

Sure adding the comma is fine, but this is still wrong
- $settings = array(
- 'values' => &$form_state['values']['settings']
+ $settings = array('values' =>
+ &$form_state['values']['settings'],
);

Ah yeah, of course. Thanks for being so patient with me here.

Thank you, I figured it was okay to nitpick a coding standards fixing issue :)
Looks good to me!

Honestly, we can just drop the t() altogether.

Needs some rework on this patch. I'm working on this.

I have ignored few errors referring to this comment https://drupal.org/comment/8644877#comment-8644877 point 3.

36: drupal-block-code-review-1522384-36.patch queued for re-testing.

This is basically a reroll as block.js has moved to js/block.js

+++ b/core/modules/block/custom_block/custom_block.module
@@ -80,7 +80,7 @@ function custom_block_load($id) {
-  /** @var $entity_types \Drupal\Core\Entity\EntityTypeInterface[] */
+  // @var $entity_types \Drupal\Core\Entity\EntityTypeInterface[]

This is not okay, what's in HEAD is a standard pattern.

Closed the related issue as a duplicate. There is a 7.x patch in it.

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.