Files: 
CommentFileSizeAuthor
#46 interdiff.txt776 bytesTravisCarden
#46 drupal-block-code-review-1522384-46.patch74.75 KBTravisCarden
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch drupal-block-code-review-1522384-46.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Comments

TravisCarden’s picture

Status:Active» Needs review
StatusFileSize
new17.25 KB
PASSED: [[SimpleTest]]: [MySQL] 36,158 pass(es).
[ View ]

Here's a patch.

  1. The most potentially contentious change is probably this in block.api.php:
    -      '#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))),

    It brings the line under 80 characters, and it avoids the unseemly 21 line array declaration, but if people find it confusing, we can discuss alternatives.

  2. After the patch, the Drupal Code Sniffer reports this:
    FILE: /home/quickstart/websites/d8.dev/core/modules/block/block.api.php
    --------------------------------------------------------------------------------
    FOUND 3 ERROR(S) AFFECTING 3 LINE(S)
    --------------------------------------------------------------------------------
    310 | ERROR | There must be no blank line following an inline comment
    332 | ERROR | global variables should start with a single underscore followed
         |       | by the module and another underscore
    335 | ERROR | There must be no blank line following an inline comment
    --------------------------------------------------------------------------------

    Is line 332 a false positive? It seems like we seldom if ever do this across core.

    I don't know if there's a good solution for lines 310 and 335. The comments refer to the whole function, so it doesn't seem right to merge them with another comment (310) or place them right above an individual statement (335). I'd take suggestions.

droplet’s picture

+++ 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 ?

TravisCarden’s picture

StatusFileSize
new2.1 KB
new18.52 KB
PASSED: [[SimpleTest]]: [MySQL] 35,070 pass(es).
[ View ]
  1. Yes, marginally less performant, but it's in an api.php file, so it's never actually executed. I don't think we should be concerned with it.
  2. It's been confirmed that the global variable naming error is a false positive. I've opened an issue to fix the sniffer: #1535558: $language_interface missing from global variables whitelist.
  3. I moved the function-level comments into the docblocks to resolve the other two errors.
webchick’s picture

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. :)

TravisCarden’s picture

StatusFileSize
new18.49 KB
PASSED: [[SimpleTest]]: [MySQL] 35,061 pass(es).
[ View ]
  1. Good point, @webchick. In that case, let's just lop off the two irregular values at the end of the array and use a simple range():
    +++ b/core/modules/block/block.api.php
    @@ -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(range(2, 20)),

    That's nice 'n simple.

TravisCarden’s picture

Issue summary:View changes

Added related issue.

TravisCarden’s picture

StatusFileSize
new19.23 KB
PASSED: [[SimpleTest]]: [MySQL] 35,344 pass(es).
[ View ]

Rerolled for upstream changes and updated summary.

dinathecool’s picture

Status:Needs review» Needs work

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
lotyrin’s picture

Status:Needs work» Needs review

Core is exempt from that rule, afaik.

TravisCarden’s picture

Status:Needs review» Needs work

Yes, core is exempt. This patch nevertheless needs to be rerolled without the @param and @return types per the new directions on the initiative page. I'll get back to it when I can.

roborn’s picture

Status:Needs work» Needs review
StatusFileSize
new20.88 KB
PASSED: [[SimpleTest]]: [MySQL] 40,382 pass(es).
[ View ]

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?

Lars Toomre’s picture

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.

TravisCarden’s picture

Status:Needs review» Postponed

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

sphism’s picture

Status:Postponed» Active

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

sphism’s picture

Issue summary:View changes

Updated summary.

TravisCarden’s picture

Assigned:TravisCarden» Unassigned
Issue summary:View changes
Parent issue:» #1518116: [meta] Make Core pass Coder Review
visabhishek’s picture

Assigned:Unassigned» visabhishek
visabhishek’s picture

Status:Active» Needs review
StatusFileSize
new68.29 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch drupal-make_block_module_pass_coder_review-1522384-16.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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

brantwynn’s picture

Status:Needs review» Needs work

The last submitted patch, 16: drupal-make_block_module_pass_coder_review-1522384-16.patch, failed testing.

brantwynn’s picture

Assigned:visabhishek» brantwynn
brantwynn’s picture

Status:Needs work» Needs review
StatusFileSize
new64.86 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,977 pass(es), 6 fail(s), and 0 exception(s).
[ View ]

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.

brantwynn’s picture

Assigned:brantwynn» Unassigned

Status:Needs review» Needs work

The last submitted patch, 20: drupal-block-code-review-1522384-20.patch, failed testing.

brantwynn’s picture

Status:Needs work» Needs review
StatusFileSize
new66.29 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch drupal-block-code-review-1522384-23.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new1.81 KB

Status:Needs review» Needs work

The last submitted patch, 23: drupal-block-code-review-1522384-23.patch, failed testing.

brantwynn’s picture

Status:Needs work» Needs review
StatusFileSize
new65.96 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,005 pass(es).
[ View ]

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

tim.plunkett’s picture

Status:Needs review» Needs work
  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

brantwynn’s picture

Status:Needs work» Needs review
StatusFileSize
new63.72 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,061 pass(es).
[ View ]
new5 KB

Thanks for the review!

tim.plunkett’s picture

+++ 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.

brantwynn’s picture

StatusFileSize
new63.66 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,993 pass(es).
[ View ]
new725 bytes

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

tim.plunkett’s picture

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'],
);

brantwynn’s picture

StatusFileSize
new63.51 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,114 pass(es).
[ View ]
new941 bytes

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

tim.plunkett’s picture

Status:Needs review» Reviewed & tested by the community

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

TravisCarden’s picture

Status:Reviewed & tested by the community» Needs work

Sorry; just one more thing:

+++ b/core/modules/block/lib/Drupal/block/Tests/BlockTest.php
@@ -130,7 +133,7 @@ function testBlock() {
-    $this->assertNoText(t($block['settings[label]']));
+    $this->assertNoText(t('@label', array('@label' => $block['settings[label]'])));

This is an ingenius little trick to satisfy the translation system's interface (and thus Coder Sniffer), but I'm afraid that the practical result may be equally undesirable: "@label" isn't a string that a translator could possibly do anything meaningful with. It may be better to treat this as a false positive and leave it out of the patch.

@tim.plunkett, do you agree?

tim.plunkett’s picture

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

sriharsha.uppuluri’s picture

Assigned:Unassigned» sriharsha.uppuluri

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

sriharsha.uppuluri’s picture

StatusFileSize
new72.44 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch drupal-block-code-review-1522384-36.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new3.4 KB
new13.19 KB

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

sriharsha.uppuluri’s picture

Status:Needs work» Needs review
brantwynn’s picture

brantwynn’s picture

Status:Needs review» Needs work

The last submitted patch, 36: drupal-block-code-review-1522384-36.patch, failed testing.

brantwynn’s picture

Assigned:sriharsha.uppuluri» Unassigned
Status:Needs work» Needs review
StatusFileSize
new72.5 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,469 pass(es).
[ View ]

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

TravisCarden’s picture

Issue tags:+Quick fix
StatusFileSize
new75.5 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,641 pass(es).
[ View ]
new11.33 KB

Upstream changes necessitated a reroll. There were also some issues that the previous patch didn't address and a few patterns it didn't get quite right. (See the interdiff.txt.) It's possible we could find more niggles if we kept looking, but I believe what I've attached satisfies all the strict requirements. I think we should get this committed before we need to reroll again!

TravisCarden’s picture

StatusFileSize
new75.5 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,545 pass(es).
[ View ]
new868 bytes

D'oh! Missed one little thing.

TravisCarden’s picture

Issue summary:View changes
tim.plunkett’s picture

+++ 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.

TravisCarden’s picture

StatusFileSize
new74.75 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch drupal-block-code-review-1522384-46.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new776 bytes

Thanks, @tim.plunkett. I just got the same feedback elsewhere from @webchick. We'll want to fix Coder Sniffer, because it complains about it:

FILE: ...ww/community/d8.dev/core/modules/block/custom_block/custom_block.module
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
83 | ERROR | Inline doc block comments are not allowed; use "// Comment"
    |       | instead
--------------------------------------------------------------------------------

The folks over there like it when we can point to documentation. A quick search didn't turn up any on this topic for me. You don't know of anything do you?

In the meantime, here's a patch with that change removed.

TravisCarden’s picture

Update re Coder Sniffer: issues have been filed to fix false positives, and our Coder fork has been patched accordingly. See #1518116-76: [meta] Make Core pass Coder Review.

dcam’s picture

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

Status:Needs review» Needs work

The last submitted patch, 46: drupal-block-code-review-1522384-46.patch, failed testing.

harijari’s picture

Issue tags:+dcwroc2014
xjm’s picture

Version:8.0.x-dev» 8.1.x-dev
Priority:Normal» Minor
Status:Needs work» Postponed
Issue tags:-Novice

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.