Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 UTC on 18 March 2024, to get $100 off your ticket.
Part of meta-issue #1518116: [meta] Make Core pass Coder Review
- The Drupal 8 branch test code review tab is not available to check at the moment.
- Drupal Code Sniffer reports no errors but the missing directive types, as expected.
- Coder Review finds no problems at the minor (strictest) severity warning level.
Comment | File | Size | Author |
---|---|---|---|
#42 | book-cs-1530700-42.patch | 39.12 KB | mfernea |
#40 | book-cs-1530700-40.patch | 46.75 KB | mfernea |
#37 | book-cs-1530700-37.patch | 46.71 KB | mfernea |
#32 | book-coder-fixes-1530700-32.patch | 11.89 KB | saki007ster |
#27 | book-coder-fixes-1530700-27.patch | 11.68 KB | Anonymous (not verified) |
Comments
Comment #1
TravisCarden CreditAttribution: TravisCarden commentedComment #2
crobinson CreditAttribution: crobinson commented#1: drupal-1530700-1.patch queued for re-testing.
Comment #4
crobinson CreditAttribution: crobinson commentedI attempted to review/test this patch tonight but it failed to apply. It looks like commits have been made to book.module since the patch was created. I re-submitted the test and the failed elements are now showing in the test result. Please address these items and I'll keep an eye on it so hopefully we can get it submitted more quickly.
Comment #5
TravisCarden CreditAttribution: TravisCarden commentedThanks, @crobinson. Here's an updated patch (without the
@param
and@return
types, per the new direction of the larger initiative).Comment #6
crobinson CreditAttribution: crobinson commentedThanks for the update, @TravisCarden. I did a fresh git pull and was able to apply the new patch with no problems. The patch also addresses Coder Review for me. It's pretty obvious that there's some work underway in the Book module, but none of that is related to this ticket, and this patch addresses this ticket's request. I'm therefore marking this RTBC.
Comment #7
chx CreditAttribution: chx commentedThanks for working on this.
Is this really necessary? I thought it's return theme('table', array( and then one array argument per line.
Comment #8
jhodgdonRE #7 - The standard for that is being discussed on
#1539712: [policy, no patch] Coding standards for breaking function calls and language constructs across lines
Comment #9
TravisCarden CreditAttribution: TravisCarden commentedPostponing pending a standard re: #8 .
Comment #10
Lars Toomre CreditAttribution: Lars Toomre commentedWhen a patch for this issue is next re-rolled, a couple of issues need to be addressed in BookTest.php file:
a) Class members like book_author should be in camelCase like bookAuthor. There are three such changes needed in this test file. I wonder if this type of issue is being flagged by the coder review process.
b) There needs to be a blank line added just before the closing brace separating it from the end of the final method class.
Comment #11
Lars Toomre CreditAttribution: Lars Toomre commented[#1807068] is open to deal separately with any missing type hinting from @param and/or @return directives.
Comment #12
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 #13
iler CreditAttribution: iler commentedAssigning this issue to myself.
Comment #14
iler CreditAttribution: iler commentedPlease find a patch fixing major part of the issues found in Book module. I used the Coder module with the patch provided here. However there are still couple of errors reported by the Coder module that should be discussed.
This error is caused by the {@inheritdoc} documentation comment and there are several of these errors in several different files. There is active discussion going or for subject here #1994890: Allow {@inheritdoc} and additional documentation.
This is caused by the following comment which utilises twig.
There is also this issue with no key specified for array entry. I couldn't find a way to fix this so tips for this issue would be appreciated.
Comment #16
iler CreditAttribution: iler commentedHere is a new version of this patch where the failure in tests is now fixed.
Comment #17
iler CreditAttribution: iler commentedComment #19
iler CreditAttribution: iler commentedHad missed those 2 fails in the last fix. Here is a new one.
Comment #19.0
iler CreditAttribution: iler commentedUpdated issue summary.
Comment #20
parthipanramesh CreditAttribution: parthipanramesh commentedLatest patch doesn't work..
Comment #21
YesCT CreditAttribution: YesCT commentedreroll instructions: https://drupal.org/contributor-tasks/reroll
Comment #22
YesCT CreditAttribution: YesCT commented@iler, it has been a few months. unassigning. feel free to post a comment and jump back in if you like. otherwise, this is up for anyone to try.
Comment #23
deepakaryan1988Comment #24
deepakaryan1988I have fix all the coder notice which was coming.
Comment #25
deepakaryan1988Comment #26
Anonymous (not verified) CreditAttribution: Anonymous commentedThe patch from comment #24 looks great. It mainly edits comments and adds empty lines and whitespaces.
Tested and ready for commit,
Comment #27
Anonymous (not verified) CreditAttribution: Anonymous commentedOoops, I finally ran
drush coder core/modules/book/ --no-empty
and found another two missing whitespaces.
Added them and here ist an updated patch.
Comment #28
TR CreditAttribution: TR commented@return should be followed by a data type, not a variable name. So declarations in the patch like "@return array_book" and "@return menu_link" are wrong.
This is explained at https://drupal.org/coding-standards/docs#return
And why are you changing the @see to just plain "see"?
Use of @see is explained at https://drupal.org/coding-standards/docs#see
Comment #29
sidharthapUpdating issue tag. Will have a look at Drupal Camp Mumbai sprint on this weekend.
Comment #30
TravisCarden CreditAttribution: TravisCarden commentedThe instructions for this meta at #1518116: [meta] Make Core pass Coder Review stipulate running Coder Sniffer, not Coder. Is it clear enough how to actually perform this work, or do the instructions need more clarification for everyone?
Comment #31
saki007sterComment #32
saki007sterre-rolled the patch.
Comment #33
saki007sterComment #34
saki007sterComment #35
TR CreditAttribution: TR commentedYou didn't address the issues I raised in #28.
Comment #36
mfernea CreditAttribution: mfernea commentedI'm working on this one.
Comment #37
mfernea CreditAttribution: mfernea commentedUsing the instructions in #1518116: [meta] Make Core pass Coder Review I ran Code Sniffer against the book module and solved most of the issues.
The only problems remaining are:
Comment #38
mfernea CreditAttribution: mfernea commentedComment #40
mfernea CreditAttribution: mfernea commentedThere were some conflicts. Here is the reroll.
Comment #41
TravisCarden CreditAttribution: TravisCarden commentedA few things here:
I think this should actually be
Implements hook_preprocess_HOOK() for block.html.twig.
That should hopefully make Sniffer stop complaining.This isn't overriding a parent method, such that it should just get
{@inheritdoc}
doxygen?Comment #42
mfernea CreditAttribution: mfernea commentedI've rerolled the patch.
1. I've removed modifications related to @param and @return in doc blocks.
2. I used the sniffer standard in coder_sniffer/Drupal from issue-1518116 branch in coder project. The remaining issues are:
2.1 method names like 'menu_build_tree' in lib/Drupal/book/BookManager.php.
2.2 Missing function doc comment in lib/Drupal/book/BookManagerInterface.php
2.3 Four "Line exceeds 80 characters;" errors in tests/Drupal/book/Tests/BookManagerTest.php
2.4 Four "Inline doc block comments are not allowed; use "// Comment" instead" in book.module.
Is it correct? Am I missing something?
Should we try to solve those issues? Can you please advise on how to do this?
3. Is there anything else we should remove from the patch?
4. Ok, done.
5. Ok, done.
Comment #43
Jalandhar CreditAttribution: Jalandhar commentedPatch #42 needs reroll.
Comment #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.