This is part of meta-issue #1518116: [meta] Make Core pass Coder Review focused on the new D8 Ban module.
- Whether this directory either passes or fails on the Drupal 8 branch test code review tab. If it fails, also note whether the failures are itemized on that page.
- Whether the directory has been tested with Drupal Code Sniffer (patched per #36) with the patch applied, whether it passes or fails, and what settings were used.
- Whether the directory has been tested with the D8 sandbox of Coder with the patch applied, whether it passes or fails, and what settings were used.
>> There are 0 minor / 0 critical / 0 normal issues.
>> It has been tested using 7.x-2.x branch of Coder module as Drupal Code Sniffer module is part of Coder module now. I tried applying this patch #36 on Code Sniffer module but nothing happens. No errors and no changes. Even if I make those changes manually, it doesn't ignore excluded sniffs.
>> #1536122: Drupal 8 port no longer works with 7.x-2.x branch of Coder module. So I have tested it using #1816302: Code Review 7.x-1.2 to 8.x-1.x Patch patch. This patch also takes care of {system} table issue.
Comment | File | Size | Author |
---|---|---|---|
#26 | 181660-interdiff-23-20.txt | 714 bytes | jaxxed |
#23 | make_ban_module_pass_coder_review-1816690-23.patch | 4.47 KB | tatarbj |
#21 | coder_issues_ban_2015_09-07..txt | 3.91 KB | jaxxed |
#20 | make_ban_module_pass_coder_review-1816690-1.patch | 4.47 KB | tatarbj |
#9 | coder_issues-ban-1816690-d8-2012_10_23.patch | 1.21 KB | amitgoyal |
Comments
Comment #1
Anonymous (not verified) CreditAttribution: Anonymous commentedComment #2
Lars Toomre CreditAttribution: Lars Toomre commented#1816732: Add missing type hinting to Ban module docblocks is open to deal separately with any missing type hinting from @param and/or @return directives.
Comment #3
amitgoyal CreditAttribution: amitgoyal commentedBased on the learnings from #1533244: Make help module pass Coder Review issue, I have made fixes, related to coding standards, in Ban module. As per learning there, I have ignored following issues,
- Standard simpletest functions getInfo(), setUp(), and tearDown() don't get doxygen comments
- No scope modifier specified for function getInfo() in class files
Attaching a Patch here which will fix the remaining issues. I am also attaching Coder output coder_issues_ban_2012_10_22.txt so that we can see the list of all issues.
Comment #4
Kars-T CreditAttribution: Kars-T commentedTwo comments are now below 80 chars and the array has an "," at its end. Seems okay to me.
Comment #5
TravisCarden CreditAttribution: TravisCarden commentedA couple of tiny things, actually:
It's not proper to use an ampersand in place of the word "and" in full English prose. Let's just use the original comment as-is and wrap it.
As long as we're here, why don't we capitalize this sentence? "on" -> "On"
Thanks!
Comment #6
amitgoyal CreditAttribution: amitgoyal commented@TravisCarden - I have changed "on" -> "On" and attached the updated patch.
Regarding first point "It's not proper to use an ampersand in place of the word "and" in full English prose. Let's just use the original comment as-is and wrap it.", if we wrap it to next line then there would be new issue,
Function comment short description must be on a single line
So I haven't changed it. Please let me know if you have some other solution.
Comment #7
Lars Toomre CreditAttribution: Lars Toomre commented@amitgoyal Thanks for the patch in #7. I hope to get to reviewing this later today (once it goes green).
Perhaps you could do all of us a favor and post some simple instructions about how you generated the txt file attached in #3? To my knowledge, I am unable to do so.
The Ban module is a fairly small module that was spun out from an include file in the system module. Action is a similarly new D8 module that lived in the System module in D7.
Not that I expect anyone to post a patch for those modules, but for my own edification, I would love to get a sense of what lays ahead of us for some of the older and bigger modules. Is it possible to generate a report similar to what is in #3 for some of the modules with lots of legacy code? Such modules include Comments, Node, Poll, System, Taxonomy, and/or User.
If you can easily do so, could you post any such reports in the appropriate sub-issue(s) of #1518116: [meta] Make Core pass Coder Review? Thanks in advance for your help @amitgoyal. I am so glad to see us inching forward on this initiative and congrats for getting the patch for the Helper module committed last week!!
Comment #8
TravisCarden CreditAttribution: TravisCarden commentedHow about "
Tests various user input to confirm correct validation and saving of data.
"? That preserves the English and satisfies the sniff.Comment #9
amitgoyal CreditAttribution: amitgoyal commentedSure @TravisCarden. Here is the updated patch.
Comment #10
TravisCarden CreditAttribution: TravisCarden commentedThanks, @amitgoyal. That looks better! If you can update the issue summary per the meta issue's Proposed Resolution #7 we can RTBC this.
Comment #11
amitgoyal CreditAttribution: amitgoyal commented@Lars - Here are the steps to check the status of drupal 8 modules using coder or generate the txt file attached in #3,
1) Clone (git clone --recursive --branch 8.x http://git.drupal.org/project/drupal.git) the latest D8 branch on your system or update your existing D8 branch (git pull).
2) Clone (git clone --recursive --branch 7.x-2.x http://git.drupal.org/project/coder.git) the 7.x-2.x branch of Coder module.
3) Download and apply this patch #1816302: Code Review 7.x-1.2 to 8.x-1.x Patch to 7.x-2.x branch of Coder module.
4) Enable the Code Review module and review any core / contributed module from Configuration->Coder (admin/config/development/coder). Make sure to check "Drupal CodeSniffer" and "minor (most)" in the Reviews section. The output by Code Review module can be copied into text file.
Hope this will help you and others if they want to review modules for d8.
@TravisCarden - I am not very sure what do you want me to do. As per my understanding and points mentioned in #7, I have added instructions here to code review any d8 module.
Comment #12
TravisCarden CreditAttribution: TravisCarden commented@amitgoyal: Sorry, I had a bad hyperlink in my comment. Here's proposed resolution #7 from the meta issue:
That's what I'm asking for. :) Does that make sense?
Comment #13
amitgoyal CreditAttribution: amitgoyal commented@TravisCarden - I have updated the issue summary. Please let me know if this is what you are asking for. If not then please help me in doing so.
Comment #14
TravisCarden CreditAttribution: TravisCarden commentedThat looks great, @amitgoyal. Thanks!
Comment #15
webchickPassing the buck. :)
Comment #16
jhodgdonI will get this committed soon -- but there's a big Views sprint going on and commits are delaying their patch reviews, so postponing a few days. Sorry!
Comment #17
catch#9: coder_issues-ban-1816690-d8-2012_10_23.patch queued for re-testing.
Comment #18
jhodgdonThanks! This one is committed to 8.x.
Comment #19.0
(not verified) CreditAttribution: commentedIssue summary changed as per http://drupal.org/node/1816690#comment-6638678
Comment #20
tatarbjI'm sorry but need to reopen this ticket because ban module has unsolved coding standards errors. I'm attaching patch which solve those.
The discussion about it is followable here: https://www.drupal.org/node/1518116
Comment #21
jaxxed CreditAttribution: jaxxed at Wunder commentedCoder/phpcs now reports no issues after applying #20 patch (see attached issues list coder_issues_ban_2015_09-07.txt for issues without patch)
Comment #22
pfrenssenI reviewed the patch and it is looking great! I found a spelling mistake, but this is not 100% in scope of this issue, so leaving status to RTBC.
I would take the opportunity to fix this spellign error if we are touching this line.
The codesniffer doesn't like this?
Comment #23
tatarbjHi @pfrenssen,
the first spelling mistake is fixed in my new patch.
The second one what you mention, if it's not fixed the codesniffer throws this error:
So in this case it thinks it's not correct because there are two capital letters which brakes this rule, i think we should accept it. Btw it's the same with this change:
where the change is not just the visibility but the IP goes to Ip as well.
Comment #24
tatarbjComment #25
tatarbjBecause the test was ok, i'm taking the issue back to rtbc based on others' check.
Comment #26
jaxxed CreditAttribution: jaxxed at Wunder commented23-20 interdiff added
Comment #27
alexpott@tatarbj opening a closed issue after 3 years messes up issue statistics and how we assign credit for issues - can you please move your patch to a new issue.
Comment #28
tatarbjHi people, here is the new issue, please follow this one: #2566313: Make Ban module pass Coder Review