Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Part of meta-issue #1518116: [meta] Make Core pass Coder Review
Comment | File | Size | Author |
---|---|---|---|
#42 | 1533208-42.patch | 8.31 KB | rpayanm |
#42 | 1533208-interdiff.txt | 2.59 KB | rpayanm |
Comments
Comment #1
scottalan CreditAttribution: scottalan commentedComment #2
Cameron Tod CreditAttribution: Cameron Tod commentedA 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.
Comment #3
jhodgdonThis 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:
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:
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:
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?
Comment #4
Cameron Tod CreditAttribution: Cameron Tod commentedThanks 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:
So, for the 'public setUp' change, I searched through the existing setUp() methods in core tests. A truncated sample:
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:
Comment #5
TravisCarden CreditAttribution: TravisCarden commentedPostponing till feature freeze. If you want to help in the meantime, please work on the blockers on the meta issue. Thanks!
Comment #6
sphism CreditAttribution: sphism commentedWe have the go ahead with all these issues again, see #1518116: [meta] Make Core pass Coder Review for more details
Comment #7
Wim Leers#4: 1533208-contextual-code-review-4.patch queued for re-testing.
Comment #9
Wim LeersMarked #1816764: Add missing type hinting to Contextual module docblocks as a duplicate of this issue.
Comment #10
Wim LeersAny takers?
Comment #11
Risse CreditAttribution: Risse commentedHere's my take on the issue.
Still getting couple of errors:
I made sure there's newline before and after that parament. The whole docblock just looks okay.
There's a comment starting // at the beginning of the function, looks like the Code Review thinks it's a function comment?
Comment #12
Wim LeersThank you! :)
I have quite a bit of feedback for you:
We don't do this. What cannot be split, cannot be split.
AFAIK that was explicitly done that way to allow IDEs to pick this up. You'll want to check that.
This also cannot be split.
We *never* comment these.
We never do this either.
Why would you do this?
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".
Comment #13
deepakaryan1988I have created patch for it.
Comment #14
deepakaryan1988Comment #16
Wim LeersBetter … almost :)
s/html/string/
Trailing whitespace.
Again:
This is wrong. Please restore the original.
Comment #17
AjitSRe-rolling from #13
Comment #18
TravisCarden CreditAttribution: TravisCarden commentedA few things here:
Please address these issues so someone can productively perform a closer code review. Thanks!
Comment #19
Wim LeersAll 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 :)
Comment #20
TravisCarden CreditAttribution: TravisCarden commentedThanks, @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.:
But most importantly, Coder Sniffer, run according to the instructions in the meta (esp. #6) still reports a bunch of issues. (See attachment.)
Comment #21
Jalandhar CreditAttribution: Jalandhar commentedHi,
I have started working on it.
Comment #22
Jalandhar CreditAttribution: Jalandhar commentedI have created a patch which solves all coder errors for Contextual links core module.
Please review it.
Comment #24
Jalandhar CreditAttribution: Jalandhar commentedAnother try at patch.
Comment #26
Jalandhar CreditAttribution: Jalandhar commentedHi,
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.
Comment #27
Jalandhar CreditAttribution: Jalandhar commentedComment #28
Jalandhar CreditAttribution: Jalandhar commentedUpdating the patch with reroll and fixed all the coder warnings. Please review it
Comment #29
TravisCarden CreditAttribution: TravisCarden commentedPlease see comment #18 and try again. Thanks.
Comment #30
Jalandhar CreditAttribution: Jalandhar commentedComment #31
sundersingh CreditAttribution: sundersingh commentedComment #32
sundersingh CreditAttribution: sundersingh commentedI 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:
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.
Comment #35
wiktorb CreditAttribution: wiktorb commentedRe-rolled against the latest dev.
Comment #36
Jalandhar CreditAttribution: Jalandhar commentedPatch #35, no longer applies.
Here is the updated patch which resolves all the coder warnings.
Comment #37
jsobiecki CreditAttribution: jsobiecki commentedComment #40
rpayanmIn core/modules/contextual/src/Plugin/views/field/ContextualLinks.php still missing @return tag in function PHPDoc comment:
Comment #41
Wim LeersThanks, looking good, almost there :)
We don't split this up.
Why remove these?
s/run/provider/, to be consistent with PHPUnit data providers.
And this should be protected, not public.
Comment #42
rpayanmI made a reroll, however here is the new patch :)
Comment #43
rpayanmComment #44
xjmThanks 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.
Comment #45
pfrenssenClosing 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.