Review and revise patches

Needs revision -- Last peer review: 27 Oct 2016
Description: 
Learn to review and revise patches submitted to the drupal.org issue queue.
Overview: 

In this lesson you will review and possibly revise a REAL drupal patch currently in the issue queues of drupal.org. You will make notes of your observations during each step of the lesson. You will use these notes to create your Patch Review.

You do NOT need be a drupal expert to review and revise patches. Simply choose a patch that matches your level of ability. Even the smallest, most basic patches need to be reviewed and possibly revised. These include patches that remove extra whitespaces from existing code, patches that correct typos in a function's comment, and patches that make very simple code corrections.

Prerequisites: 
  • a clean checkout of the Drupal code base in which the issue appears (instructions)
  • the ability to apply and test patches (instructions)
  • at least a beginner-level familiarity with PHP and the Drupal API
  • your favorite note-taking application
Steps: 

Quick Outline:

  1. Search for a patch to review
  2. Review the patch's coding-standards
  3. Review the issue
  4. Review the patch
  5. Write your review
  6. Review your review
  7. Post your review

Detailed Steps:

  1. Search the drupal.org issue queues for a recent, beginner-level (priority=minor) .patch file that “needs review” or “needs work”.
    1. Login to drupal.org
    2. Click on one of the search links below
      1. Search All Projects for a recent Minor patch that Needs Review
        https://www.drupal.org/project/issues/search/drupal?project_issue_follow...
      2. Search All Projects for a recent Minor patch that Needs Work
        https://www.drupal.org/project/issues/search/drupal?project_issue_follow...
    3. Scroll the search results table into view.
    4. Locate the “Summary” column of the search results table.
    5. Quickly scan patches.
      1. Click on the first (or next) entry in the “Summary” column to open its issue page.
      2. For the moment, ignore the issue discussion (yes, that's right, ignore it) and simply scroll to the latest .patch file within the issue discussion.
      3. Click on the .patch file to see if it really is within your patch reviewing ability. You are looking for a patch that does something very basic, like simply removing extra whitespaces, fixing a comment typo, or correcting a very simple snippet of code.
      4. If the .patch file seems too advanced for you, simply navigate back to the search results table and repeat the “Quickly scan patches” step, using the next entry in the “Summary” column of the search results table. You may have to repeat the “Quickly scan patches” step quite a few times in order to find a patch that suits you. This is expected: methodically scanning through the issue queues for a patch that matches your skill & interest level is a necessary part of the patch review process, regardless of your level of expertise. Take your time.
    6. Once you find a .patch file that you can review, make a note of its URL.
  2. Review the patch's coding-standards, making notes as you go.
    1. The "General" Pass - Look for any Drupal coding-standards errors in the patch. Don't concern yourself with what the patch is supposed to do just yet, or whether the patch's approach is even a good idea. Merely examine the structure of the patch to verify that it's well-formed according to the coding-standards described at http://drupal.org/coding-standards.
      1. Manually check the patch for errors in:
        • Indenting and Whitespace
        • Operators
        • Casting
        • Control Structures
        • Line length and wrapping
        • Function Calls
        • Function Declarations
        • Class Constructor Calls
        • Arrays
        • Quotes
        • String Concatenations
        • Comments
        • Including Code
        • PHP Code Tags
        • Semicolons
        • Example URLs
        • Naming Conventions
      2. If the patch has coding-standards errors, fix them (if you can) to help the patch proceed. Otherwise, just make notes about any coding standard errors that you find.
    2. The "Functions" Pass - If there are any functions in the patch, read the patch in hunks from the bottom up, examining each function's readability, without relying on the function's comments.
      1. Look for:
        1. Meaningful Function Names - Determine if each function name is meaningful. A function name should immediately give you a clue about what the function does.
        2. Small Size - Determine if each function is too large. Would the function be more understandable if it were refactored as several smaller functions?
        3. Drupal's “t() function” - If the function will display any human-readable text on a web page, is the text first run through Drupal's t() function?
      2. If the patch has any function-readability errors, fix them (if you can) to help the patch proceed. Otherwise, just make notes about any function-readability errors that you find.
    3. The "Comments" Pass - If there are any comments in the patch, examine the comments for readability errors.
      1. Check the comments for:
        1. Grammar
        2. Punctuation
        3. Clarity - Do the comments reflect your understanding of the code and/or maybe clarify parts of the patch you don't quite understand?
      2. If the patch has any comment-readability errors, fix them (if you can) to help the patch proceed. Otherwise, just make notes about any comment-readability errors that you find.
  3. Review the issue that the patch attempts to fix, making notes as you go.
    1. Read the entire Issue Discussion
      1. Is the Discussion itself understandable?
      2. Is it an issue at all?
      3. Is it a duplicate Issue?
      4. Does the issue summary makes sense?
  4. Review the patch, making notes as you go.
    1. Review the patch's coding approach
      1. Determine the following:
        1. Is the patch secure?
        2. Are the theme functions/files in the proper place?
        3. Is it a "stinky?"?
        4. Does it duplicate code?
        5. Is the code efficient?
        6. Are the patch's tests sufficient?
        7. Does the patch implements useful tweaks brought up in the discussion?
      2. If the patch has any of the above coding errors, fix them (if you can) to help the patch proceed. Otherwise, just make notes about any coding errors that you find.
    2. Review the patch's success at correcting the issue.
      1. Replicate the issue
      2. Apply the patch
      3. Determine if the patch corrects the issue.
        1. If it's a large patch, confirm that the patch doesn't break any of Drupal's automated tests
          • Simpletest
          • Patch-induced bugs?
            • Uninstall the patch
            • Confirm no bugs
            • Re-apply the patch
            • Confirm bugs
      4. Make notes about the success of the patch.
  5. Write your review.
    1. Help the patch proceed.
  6. Review your review.
    1. Are you being constructive, collaborative, and respectful?
  7. Post your Review
    1. Change the issue's status, if applicable. See http://drupal.org/node/156119

.patch Search Tips:
Familiarize yourself with drupal.org's Advance Search pages, search options, and sort-table-on-heading features. In particular, Drupal Core has its own Advanced Search Page with additional search options (“Version” and “Component”) that are NOT available from the All Projects Advanced Search Pages. The more you can fine-tune your issue queue searches, the easier it will be to find patches in need of review that are both interesting and within your skill level.

Patch Reviewing and Revising Tools:
Dreditor (a browser plugin for reviewing patches and more)
Coder module (an automated way of checking for code standards compliance)
Testing (Simpletest) module
PARevview (still D7)

References:
How to Work the Issue Queue - http://drupal.org/node/945492
Diaries of a Core Maintainer #5: The 6-pass patch review - http://webchick.net/6-pass-patch-reviews
Drupal Coding Standards - http://drupal.org/coding-standards
Doxygen and comment formatting conventions - http://drupal.org/node/1354
Code Review of Full Project Applications - http://groups.drupal.org/code-review
Increasing efficiency in manual code reviews - http://groups.drupal.org/node/184389
Writing Secure Code - http://drupal.org/writing-secure-code
Programming Best Practices - http://drupal.org/node/287350
Priority Levels of Drupal - http://drupal.org/node/45111
Clean Code: A Handbook of Agile Software Craftmanship, by Robert C. Martin
This Code Stinks! - PDF: http://london2011.drupal.org/sites/default/files/Code%20Smells_0.pdf
HOWTO: Submit tests with your patch - http://groups.drupal.org/node/11020
Unit Testing with Simpletest - http://drupal.org/node/811254

Comments

add1sun's picture

xjm just wrote up a great blog post that covers how she reviews patches http://xjm.drupalgardens.com/blog/core-mentoring-and-xjms-guide-patch-re.... Could be a good resource to fill things out more.

I read many drupal patches and lesson but this described it much easily. You brief it in a better way also.