Support from Acquia helps fund testing for Drupal Acquia logo

Comments

scottalan’s picture

Assigned: Unassigned » scottalan
Cameron Tod’s picture

Assigned: scottalan » Cameron Tod
Status: Active » Needs review
FileSize
3.67 KB

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

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

Cameron Tod’s picture

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

Wim Leers’s picture

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.

Wim Leers’s picture

Wim Leers’s picture

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

Any takers?

Risse’s picture

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?

Wim Leers’s picture

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

deepakaryan1988’s picture

Assigned: Unassigned » deepakaryan1988
FileSize
2.92 KB

I have created patch for it.

deepakaryan1988’s picture

Status: Needs work » Needs review

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

Wim Leers’s picture

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.

AjitS’s picture

Status: Needs work » Needs review
FileSize
2.52 KB

Re-rolling from #13

TravisCarden’s picture

Status: Needs review » Needs work

A few things here:

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

Wim Leers’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
2.2 KB
792 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 :)

TravisCarden’s picture

Status: Reviewed & tested by the community » Needs work
FileSize
3.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.)

Jalandhar’s picture

Assigned: deepakaryan1988 » Jalandhar

Hi,

I have started working on it.

Jalandhar’s picture

Status: Needs work » Needs review
FileSize
10.09 KB

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

Please review it.

Status: Needs review » Needs work
Jalandhar’s picture

Status: Needs work » Needs review
FileSize
13.28 KB

Another try at patch.

Status: Needs review » Needs work
Jalandhar’s picture

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.

Jalandhar’s picture

Status: Needs work » Needs review
Jalandhar’s picture

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

TravisCarden’s picture

Status: Needs review » Needs work

Please see comment #18 and try again. Thanks.

Jalandhar’s picture

Assigned: Jalandhar » Unassigned
sundersingh’s picture

Assigned: Unassigned » sundersingh
sundersingh’s picture

Assigned: sundersingh » Unassigned
Status: Needs work » Needs review
FileSize
9.84 KB

I started with the patch in #28, which did not apply cleanly.
I updated the files reported with errors by Coder, and now the only complaint left is:

FILE: ...drupal-8.x/core/modules/contextual/contextual.api.php
--------------------------------------------------------------------------------
FOUND 0 ERROR(S) AND 1 WARNING(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
26 | WARNING | Line exceeds 80 characters; contains 88 characters
--------------------------------------------------------------------------------

This particular line is a long class reference, which I chose to ignore per the second bullet on: https://drupal.org/coding-standards#linelength

Patch is attached.

Status: Needs review » Needs work
wiktorb’s picture

Status: Needs work » Needs review
FileSize
8.6 KB

Re-rolled against the latest dev.

Jalandhar’s picture

Patch #35, no longer applies.

Here is the updated patch which resolves all the coder warnings.

jsobiecki’s picture

Issue tags: +dcwroc2014

Status: Needs review » Needs work
rpayanm’s picture

Status: Needs work » Needs review
FileSize
8.48 KB

In core/modules/contextual/src/Plugin/views/field/ContextualLinks.php still missing @return tag in function PHPDoc comment:

/**
   * Overrides \Drupal\views\Plugin\views\field\FieldPluginBase::render().
   *
   * Renders the contextual fields.
   *
   * @param \Drupal\views\ResultRow $values
   *   The values retrieved from a single row of a view's query result.
   *
   * @see contextual_preprocess()
   * @see contextual_contextual_links_view_alter()
   */
  public function render(ResultRow $values) {
Wim Leers’s picture

Status: Needs review » Needs work

Thanks, looking good, almost there :)

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

    We don't split this up.

  2. +++ b/core/modules/contextual/src/Tests/ContextualDynamicContextTest.php
    @@ -113,20 +120,16 @@ function testDifferentPermissions() {
    -   * @return bool
    ...
    -   * @return bool
    

    Why remove these?

  3. +++ b/core/modules/contextual/src/Tests/ContextualUnitTest.php
    @@ -25,9 +25,9 @@ class ContextualUnitTest extends KernelTestBase {
    +  public function runContextualLinksIdTestcases() {
    

    s/run/provider/, to be consistent with PHPUnit data providers.

    And this should be protected, not public.

rpayanm’s picture

2.

+++ b/core/modules/contextual/src/Tests/ContextualDynamicContextTest.php
@@ -113,20 +120,16 @@ function testDifferentPermissions() {
-   * @return bool
...
-   * @return bool

Why remove these?

I made a reroll, however here is the new patch :)

rpayanm’s picture

Status: Needs work » Needs review
xjm’s picture

Version: 8.0.x-dev » 8.1.x-dev
Status: Needs review » 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.

pfrenssen’s picture

Status: Postponed » Closed (duplicate)

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.