Files: 
CommentFileSizeAuthor
#28 drupal-make_contextual_module_pass_coder_review-1533208-28.patch7.69 KBJalandhar
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,057 pass(es).
[ View ]
#26 drupal-make_contextual_module_pass_coder_review-1533208-26.patch9.62 KBJalandhar
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,588 pass(es).
[ View ]
#20 sniffer.txt3.96 KBTravisCarden
#19 interdiff.txt792 bytesWim Leers
#19 1533208-contextual-code-review-19.patch2.2 KBWim Leers
PASSED: [[SimpleTest]]: [MySQL] 64,684 pass(es).
[ View ]
#4 interdiff.txt1005 bytescam8001
#4 tests_with_public_setup.txt1.75 KBcam8001
#4 tests_with_no_specified_visibility_setup.txt18.79 KBcam8001
#4 1533208-contextual-code-review-4.patch3.42 KBcam8001
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1533208-contextual-code-review-4_1.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#2 1533208-contextual-code-review.patch3.67 KBcam8001
PASSED: [[SimpleTest]]: [MySQL] 40,741 pass(es).
[ View ]

Comments

Assigned:Unassigned» scottalan

Assigned:scottalan» cam8001
Status:Active» Needs review
StatusFileSize
new3.67 KB
PASSED: [[SimpleTest]]: [MySQL] 40,741 pass(es).
[ View ]

A quick tidy up. A couple of things don't pass drupalcs (I am using HEAD).

contextual.module:
- line 118 has an usually formatted commented, but I think it passes coding style. drupalcs just doesn't quite know how to deal with it.
- line 131 ignored as per the meta issue instructions.

ContextualDynamicContextTest.php
- Undocumented getInfo() and setUp() methods, which is the case in every test as far as I can see.

FILE: /Users/cam8001/Sites/drupal/core/modules/contextual/contextual.module
--------------------------------------------------------------------------------
FOUND 2 ERROR(S) AFFECTING 2 LINE(S)
--------------------------------------------------------------------------------
118 | ERROR | Last parameter comment requires a blank newline after it
131 | ERROR | Data type of return value is missing
--------------------------------------------------------------------------------
FILE: ...contextual/lib/Drupal/contextual/Tests/ContextualDynamicContextTest.php
--------------------------------------------------------------------------------
FOUND 2 ERROR(S) AFFECTING 2 LINE(S)
--------------------------------------------------------------------------------
24 | ERROR | Missing function doc comment
32 | ERROR | Missing function doc comment
--------------------------------------------------------------------------------

Status:Needs review» Needs work

This patch is mercifully short, so it could probably be reviewed and committed... but there are two problems:

a) See #1518116: [meta] Make Core pass Coder Review (#5 in the issue summary). These patches are specifically *not* supposed to be adding things like:

- * @param $element
+ * @param array $element

because in longer patches they take a *very* long time to review. It's not a problem here since it's only affecting two lines and they are clearly correct, but in a longer patch as part of this meta-issue, please do not do this change.

b) There has also been a long discussion on a separate issue about changes like this:

-    $web_user = $this->drupalCreateUser(array('access content', 'access contextual links', 'edit any article content'));
+    $web_user = $this->drupalCreateUser(
+      array(
+        'access content',
+        'access contextual links',
+        'edit any article content',
+      )
+    );

We currently have nothing in the coding standards stating that this type of change is a good idea, and no one can agree on making this a standard, so this type of change should also not be included. If code sniffer flags it as a coding standards bug, then code sniffer is not following the Drupal official coding standards. See issue #1539712: [policy, no patch] Coding standards for breaking function calls, function declarations, and language constructs across lines for the whole, long, still unresolved discussion.

Marking this needs work because of (b), because that change should not be made.

c) I'm also not sure about these changes:

-  function setUp() {
+  public function setUp() {
...
-  function testNodeLinks() {
+  protected function testNodeLinks() {

On what basis was the public/protected chosen for these two functions, and do we have a coding standard saying that every class method needs to be designated as one or the other?

Status:Needs work» Needs review
StatusFileSize
new3.42 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1533208-contextual-code-review-4_1.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new18.79 KB
new1.75 KB
new1005 bytes

Thanks for the feedback, I appreciate the detailed review!

a) Noted. I plan to go through some more bugs in the meta issue, and won't make those changes.

b) I have removed this change (see attached).

I use drupalcs to review the style, and it always flags up this issue, and I have changed a lot of code on client sites to this standard. I have commented to that effect on an issue in the drupalcs queue.

#1500226: Don't flag arrays declared in function calls as breaking the 80 character limit

c) I was also not sure about these, either. This is what drupalcs has to say:

FILE: ...contextual/lib/Drupal/contextual/Tests/ContextualDynamicContextTest.php
--------------------------------------------------------------------------------
FOUND 5 ERROR(S) AFFECTING 4 LINE(S)
--------------------------------------------------------------------------------
24 | ERROR | Missing function doc comment <-- this is getInfo()
32 | ERROR | Missing function doc comment
32 | ERROR | No scope modifier specified for function "setUp"
36 | ERROR | If the line declaring an array spans longer than 80 characters,
    |       | each element should be broken into its own line
43 | ERROR | No scope modifier specified for function "testNodeLinks"
--------------------------------------------------------------------------------

So, for the 'public setUp' change, I searched through the existing setUp() methods in core tests. A truncated sample:

$ egrep -rn "function setUp" *
user/lib/Drupal/user/Tests/UserRoleAdminTest.php:25:  function setUp() {
user/lib/Drupal/user/Tests/UserRolesAssignmentTest.php:26:  function setUp() {
user/lib/Drupal/user/Tests/UserSignatureTest.php:32:  function setUp() {
user/lib/Drupal/user/Tests/UserTokenReplaceTest.php:33:  public function setUp() {
user/lib/Drupal/user/Tests/UserValidateCurrentPassCustomFormTest.php:42:  function setUp() {

There are approximately 19 tests with a modifier on setUp(), and approximately 240 without (see attached files for a list). Where there is a modifier, it is 'public'.

For the 'protected' change, if I change the modifier to private (my first instinct), drupalcs says:

43 | WARNING | The use of private methods or properties is strongly discouraged, use "protected" instead

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!

Status:Postponed» Active

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

Status:Active» Needs review
Issue tags:-Novice, -coding standards, -coder-fixes-2012

Status:Needs review» Needs work
Issue tags:+Novice, +coding standards, +coder-fixes-2012

The last submitted patch, 1533208-contextual-code-review-4.patch, failed testing.

Assigned:cam8001» Unassigned
Priority:Normal» Minor
Issue summary:View changes
Issue tags:-coder-fixes-2012

Any takers?

StatusFileSize
new10.94 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid patch format in 1533208-contextual-code-review-11.patch.
[ View ]

Here's my take on the issue.

Still getting couple of errors:

FILE: /core/modules/contextual/contextual.module
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
239 | ERROR | Last parameter comment requires a blank newline after it
--------------------------------------------------------------------------------

I made sure there's newline before and after that parament. The whole docblock just looks okay.

FILE: ...contextual/lib/Drupal/contextual/Plugin/views/field/ContextualLinks.php
--------------------------------------------------------------------------------
FOUND 2 ERROR(S) AFFECTING 2 LINE(S)
--------------------------------------------------------------------------------
  68 | ERROR | You must use "/**" style comments for a function comment
140 | ERROR | You must use "/**" style comments for a function comment
--------------------------------------------------------------------------------

There's a comment starting // at the beginning of the function, looks like the Code Review thinks it's a function comment?

Thank you! :)

I have quite a bit of feedback for you:

  1. +++ b/core/modules/contextual/contextual.api.php
    @@ -18,12 +18,13 @@
    - *   \Drupal\Core\Menu\ContextualLinkManagerInterface::getContextualLinksArrayByGroup(),
    ...
    + *   ::getContextualLinksArrayByGroup(),
    ...
    + *   \Drupal\Core\Menu\ContextualLinkManagerInterface

    We don't do this. What cannot be split, cannot be split.

  2. +++ b/core/modules/contextual/contextual.module
    @@ -264,7 +264,7 @@ function contextual_pre_render_links($element) {
    +  // @var $contextual_links_manager \Drupal\Core\Menu\ContextualLinkManager
    ...
    -  /** @var $contextual_links_manager \Drupal\Core\Menu\ContextualLinkManager */

    AFAIK that was explicitly done that way to allow IDEs to pick this up. You'll want to check that.

  3. +++ b/core/modules/contextual/contextual.module
    @@ -317,7 +317,8 @@ function contextual_contextual_links_view_alter(&$element, $items) {
    + *  - views_ui_edit:view=frontpage:location=page&view_name=frontpage
    + *    &view_display_id=page_1
    ...
    - *  - views_ui_edit:view=frontpage:location=page&view_name=frontpage&view_display_id=page_1

    This also cannot be split.

  4. +++ b/core/modules/contextual/lib/Drupal/contextual/Tests/ContextualDynamicContextTest.php
    @@ -22,6 +22,9 @@ class ContextualDynamicContextTest extends WebTestBase {
    +  /**
    +   * Gets info about the test.
    +   */
    @@ -30,6 +33,9 @@ public static function getInfo() {
    +   */
    ...
    +   * Sets up the test.
    ...
    +  /**
    +++ b/core/modules/contextual/lib/Drupal/contextual/Tests/ContextualUnitTest.php
    @@ -13,6 +13,9 @@
    +  /**
    +   * Gets info about the test.
    +   */

    We *never* comment these.

  5. +++ b/core/modules/contextual/lib/Drupal/contextual/Tests/ContextualDynamicContextTest.php
    @@ -119,18 +125,21 @@ function testDifferentPermissions() {
    +   *   Boolean from the assertion.
    ...
    +   *   Boolean from the assertion.

    We never do this either.

  6. +++ b/core/modules/contextual/lib/Drupal/contextual/Tests/ContextualUnitTest.php
    @@ -49,9 +50,9 @@ function _contextual_links_id_testcases() {
    -            'bar',
    +            0 => 'bar',
    ...
    -            'qux',
    +            1 => 'qux',
    @@ -84,16 +83,14 @@ function _contextual_links_id_testcases() {
    -            'bar',
    +            0 =>'bar',
    ...
    +            1 => 'qux',
    ...
    -            'qux',

    Why would you do this?

  7. +++ b/core/modules/contextual/lib/Drupal/contextual/Tests/ContextualUnitTest.php
    @@ -112,7 +109,7 @@ function _contextual_links_id_testcases() {
    +    $tests = $this->testCases();
    ...
    -    $tests = $this->_contextual_links_id_testcases();
    @@ -24,7 +27,7 @@ public static function getInfo() {
    -  function _contextual_links_id_testcases() {
    +  function testCases() {
    @@ -122,7 +119,7 @@ function testContextualLinksToId() {
    -    $tests = $this->_contextual_links_id_testcases();
    +    $tests = $this->testCases();

    This makes a helper method into a test method. This will cause tests to fail.

    This method should be made protected, and could be renamed, but it should NOT be renamed to begin with "test".

Assigned:Unassigned» deepakaryan1988
StatusFileSize
new2.92 KB
PASSED: [[SimpleTest]]: [MySQL] 63,147 pass(es).
[ View ]

I have created patch for it.

Status:Needs work» Needs review

The last submitted patch, 11: 1533208-contextual-code-review-11.patch, failed testing.

Status:Needs review» Needs work

Better … almost :)

  1. +++ b/core/modules/contextual/contextual.module
    @@ -216,10 +216,10 @@ function contextual_preprocess(&$variables, $hook, $info) {
    + * @return html

    s/html/string/

  2. +++ b/core/modules/contextual/contextual.module
    @@ -241,18 +241,19 @@ function contextual_pre_render_placeholder($element) {
    + *   ¶

    Trailing whitespace.

  3. +++ b/core/modules/contextual/contextual.module
    @@ -261,7 +262,7 @@ function contextual_pre_render_links($element) {
    -  /** @var $contextual_links_manager \Drupal\Core\Menu\ContextualLinkManager */
    +  // @var $contextual_links_manager \Drupal\Core\Menu\ContextualLinkManager.

    Again:

    AFAIK that was explicitly done that way to allow IDEs to pick this up. You'll want to check that.

  4. +++ b/core/modules/contextual/contextual.module
    @@ -311,7 +312,8 @@ function contextual_contextual_links_view_alter(&$element, $items) {
    - *  - views_ui_edit:view=frontpage:location=page&view_name=frontpage&view_display_id=page_1
    + *  - views_ui_edit:
    + *  - view=frontpage:location=page&view_name=frontpage&view_display_id=page_1

    This is wrong. Please restore the original.

Status:Needs work» Needs review
StatusFileSize
new2.52 KB
PASSED: [[SimpleTest]]: [MySQL] 64,171 pass(es).
[ View ]

Re-rolling from #13

Status:Needs review» Needs work

A few things here:

  • Adding doxygen type hinting has been descoped from this initiative and moved to #1800046: [META] Add missing type hinting to core docblocks, so those changes need to be removed from the patch. Please remember to follow steps 6 and 7 in the meta issue.
  • Make sure that the patch addresses all issues reported by Sniffer when run as described above.
  • Please don't do anything in the patch but fix coding standards issues.

Please address these issues so someone can productively perform a closer code review. Thanks!

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new2.2 KB
PASSED: [[SimpleTest]]: [MySQL] 64,684 pass(es).
[ View ]
new792 bytes

All looking good, thanks! Just one stray trailing period; fixed that in this reroll.

#18: this patch is good to go, I don't see the value in removing most of this patch if it's good to go. Let's just get it committed instead of getting caught up in too much process :)

Status:Reviewed & tested by the community» Needs work
StatusFileSize
new3.96 KB

Thanks, @Wim Leers. I appreciate the reminder not to get caught up in unnecessary process. My experience in this initiative, however, has been that core committers themselves will push back if the things deliniated in #18 aren't observed--especially if patches contain doxygen type hinting. Additionally, some of the type hinting in this patch isn't actually valid, e.g.:

+++ b/core/modules/contextual/contextual.module
@@ -154,10 +154,10 @@ function contextual_preprocess(&$variables, $hook, $info) {
+ * @return html

But most importantly, Coder Sniffer, run according to the instructions in the meta (esp. #6) still reports a bunch of issues. (See attachment.)

Hi,

I have started working on it.

Status:Needs work» Needs review
StatusFileSize
new10.09 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,556 pass(es), 4 fail(s), and 0 exception(s).
[ View ]

I have created a patch which solves all coder errors for Contextual links core module.

Please review it.

Status:Needs review» Needs work

Status:Needs work» Needs review
StatusFileSize
new13.28 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,517 pass(es), 4 fail(s), and 0 exception(s).
[ View ]

Another try at patch.

Status:Needs review» Needs work

StatusFileSize
new9.62 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,588 pass(es).
[ View ]

Hi,

As per the comment https://drupal.org/comment/6180146#comment-6180146, I have ignored the two errors related to coder errors of contextual module(listed below) because of which I had failure in the previous submitted patches.

FILE: ...e/modules/contextual/lib/Drupal/contextual/Tests/ContextualUnitTest.php
--------------------------------------------------------------------------------
FOUND 2 ERROR(S) AFFECTING 2 LINE(S)
--------------------------------------------------------------------------------
64 | ERROR | Key specified for array entry; first entry has no key
106 | ERROR | Key specified for array entry; first entry has no key
--------------------------------------------------------------------------------

Attaching the patch. Please review it.

Status:Needs work» Needs review

StatusFileSize
new7.69 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,057 pass(es).
[ View ]

Updating the patch with reroll and fixed all the coder warnings. Please review it

Status:Needs review» Needs work

Please see comment #18 and try again. Thanks.

Assigned:Jalandhar» Unassigned