Support from Acquia helps fund testing for Drupal Acquia logo

Comments

TravisCarden’s picture

Status: Active » Needs review
FileSize
7.13 KB
crobinson’s picture

#1: drupal-1530700-1.patch queued for re-testing.

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

The last submitted patch, drupal-1530700-1.patch, failed testing.

crobinson’s picture

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

TravisCarden’s picture

Status: Needs work » Needs review
FileSize
4.01 KB

Thanks, @crobinson. Here's an updated patch (without the @param and @return types, per the new direction of the larger initiative).

crobinson’s picture

Status: Needs review » Reviewed & tested by the community

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

chx’s picture

Status: Reviewed & tested by the community » Needs work

Thanks for working on this.

+  return theme(
+    'table',
+    array(

Is this really necessary? I thought it's return theme('table', array( and then one array argument per line.

jhodgdon’s picture

TravisCarden’s picture

Status: Needs work » Postponed

Postponing pending a standard re: #8 .

Lars Toomre’s picture

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

Lars Toomre’s picture

[#1807068] is open to deal separately with any missing type hinting from @param and/or @return directives.

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

iler’s picture

Assigned: TravisCarden » iler

Assigning this issue to myself.

iler’s picture

Status: Active » Needs review
FileSize
33.77 KB

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

--------------------------------------------------------------------------------
 xx | ERROR | Function comment short description must start with a capital
    |       | letter
 xx | ERROR | Function comment short description must end with a full stop
--------------------------------------------------------------------------------

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.

FILE: /var/www/d8.dev/core/modules/book/book.module
--------------------------------------------------------------------------------
FOUND 0 ERROR(S) AND 1 WARNING(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
 665 | WARNING | Format should be "* Implements hook_foo().", "* Implements
     |         | hook_foo_BAR_ID_bar() for xyz_bar().", or "* Implements
     |         | hook_foo_BAR_ID_bar() for xyz_bar.tpl.php.".
--------------------------------------------------------------------------------

This is caused by the following comment which utilises twig.

/**
 * Implements hook_preprocess_HOOK() for block.html.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.

FILE: .../core/modules/book/lib/Drupal/book/Plugin/Block/BookNavigationBlock.php
--------------------------------------------------------------------------------
FOUND 6 ERROR(S) AFFECTING 4 LINE(S)
--------------------------------------------------------------------------------
  91 | ERROR | No key specified for array entry; first entry specifies key
 112 | ERROR | No key specified for array entry; first entry specifies key
--------------------------------------------------------------------------------

Status: Needs review » Needs work

The last submitted patch, book-coder-fixes-1530700-14.patch, failed testing.

iler’s picture

Here is a new version of this patch where the failure in tests is now fixed.

iler’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, book-coder-fixes-1530700-16.patch, failed testing.

iler’s picture

Status: Needs work » Needs review
FileSize
34.23 KB

Had missed those 2 fails in the last fix. Here is a new one.

iler’s picture

Issue summary: View changes

Updated issue summary.

parthipanramesh’s picture

Issue summary: View changes
Status: Needs review » Needs work

Latest patch doesn't work..

error: patch failed: core/modules/book/book.module:34
error: core/modules/book/book.module: patch does not apply
error: patch failed: core/modules/book/lib/Drupal/book/BookExport.php:35
error: core/modules/book/lib/Drupal/book/BookExport.php: patch does not apply
error: patch failed: core/modules/book/lib/Drupal/book/BookManager.php:60
error: core/modules/book/lib/Drupal/book/BookManager.php: patch does not apply
error: patch failed: core/modules/book/lib/Drupal/book/Form/BookOutlineForm.php:
33
error: core/modules/book/lib/Drupal/book/Form/BookOutlineForm.php: patch does no
t apply
error: patch failed: core/modules/book/lib/Drupal/book/Plugin/Block/BookNavigati
onBlock.php:88
error: core/modules/book/lib/Drupal/book/Plugin/Block/BookNavigationBlock.php: p
atch does not apply
YesCT’s picture

Issue tags: +Needs reroll
YesCT’s picture

Assigned: iler » Unassigned

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

deepakaryan1988’s picture

Assigned: Unassigned » deepakaryan1988
deepakaryan1988’s picture

I have fix all the coder notice which was coming.

deepakaryan1988’s picture

Status: Needs work » Needs review
Anonymous’s picture

Status: Needs review » Reviewed & tested by the community

The patch from comment #24 looks great. It mainly edits comments and adds empty lines and whitespaces.

Tested and ready for commit,

Anonymous’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
11.68 KB

Ooops, I finally ran

drush coder core/modules/book/ --no-empty

and found another two missing whitespaces.
Added them and here ist an updated patch.

TR’s picture

Status: Needs review » Needs work

@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

sidharthap’s picture

Updating issue tag. Will have a look at Drupal Camp Mumbai sprint on this weekend.

TravisCarden’s picture

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

saki007ster’s picture

Assigned: Unassigned » saki007ster
saki007ster’s picture

re-rolled the patch.

saki007ster’s picture

Status: Needs work » Needs review
saki007ster’s picture

Assigned: saki007ster » Unassigned
TR’s picture

Status: Needs review » Needs work

You didn't address the issues I raised in #28.

mfernea’s picture

I'm working on this one.

mfernea’s picture

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

  • /** @var $entity_types \Drupal\Core\Entity\EntityTypeInterface[] */
  • BookManager::menu_build_tree, BookManager::_menu_build_tree, BookManager::_menu_tree_check_access, BookManager::_menu_link_translate, BookManager::menu_tree_data, BookManager::_menu_tree_data
  • * @param \Drupal\menu_link\MenuLinkStorageControllerInterface $menu_link_storage
mfernea’s picture

Status: Needs work » Needs review

The last submitted patch, 37: book-cs-1530700-37.patch, failed testing.

mfernea’s picture

FileSize
46.75 KB

There were some conflicts. Here is the reroll.

TravisCarden’s picture

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, fix Drupal.Commenting.FunctionComment.Missing*, so those changes need to be removed from the patch. Please remember to follow steps 6 and 7 in the meta issue.
  • If you use Sniffer according to the instructions in the meta, you'll find that there are a lot of errors left to fix.
  • Please don't do anything in the patch but fix coding standards issues. I noticed a few really good corrections in there that should be patched in their own issues. If we lump them in here, they'll slow things down and reduce the likelihood of getting anything committed. I've actually found issues aimed at fixing the documentation of a single function a really successful approach. They're easy to review, they don't cause conflicts, and they go really quick.
  • +++ b/core/modules/book/book.module
    @@ -616,7 +616,7 @@ function book_form_node_delete_confirm_alter(&$form, $form_state) {
     /**
    - * Implements hook_preprocess_HOOK() for block templates.
    + * Implements hook_preprocess_HOOK().
      */
    

    I think this should actually be Implements hook_preprocess_HOOK() for block.html.twig. That should hopefully make Sniffer stop complaining.

  • +++ b/core/modules/book/tests/Drupal/book/Tests/Menu/BookLocalTasksTest.php
    @@ -25,6 +28,9 @@ public static function getInfo() {
    +  /**
    +   * Adds the required module directories before setting up the Drupal site.
    +   */
       public function setUp() {
    

    This isn't overriding a parent method, such that it should just get {@inheritdoc} doxygen?

mfernea’s picture

Status: Needs work » Needs review
FileSize
39.12 KB

I'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.

Jalandhar’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll

Patch #42 needs reroll.

xjm’s picture

Version: 8.0.x-dev » 8.1.x-dev
Priority: Normal » Minor
Status: Needs work » Postponed

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.