Image credit: Aaron Deutsch

In the last few months, I've been doing a lot of Drupal core contributions and, even though I've done plenty of contributions over the years, I've recently learned some new things when reviewing issues. I've made this cheat sheet for anyone who wants to help review Drupal issues and wants to make sure everything has been thoroughly checked before marking "RTBC" (Reviewed & tested by the community).

There is plenty of great documentation on the development contribution process that explains some of these items further which I've also linked to below. Documentation is also being improved right now so I will update this post as new information is available. For now, hopefully this post can serve as a handy reference.

Special thanks to the Bug Smash Initiative team for recent discussions on this topic!

IMPORTANT: You do NOT need to do all of these steps to move an issue forward. For example, you could do the manual testing and nothing else. Or, make sure the issue summary is in good shape and nothing else.

But, if you are going to mark an issue RTBC, please double check that everything has been covered. This is immensely helpful to make things more efficient for not only the maintainers but for all contributors. Thanks!

Issue RTBC checks

Here's a bulleted version of the most common things to check before marking an issue RTBC:

1. Title is clear and accurate
2. Issue summary
   1. Clear and accurate
   2. Uses template
   3. Has steps to reproduce if applicable
   4. Has embedded "before" and "after" screenshots if relevant
3. Metadata is correct
   1. Project is correct
   2. Category is correct
   3. Priority is correct
   4. Status is changed to RTBC (if all good!)
   5. Version is current development branch, e.g. 9.1.x-dev
   6. Component is correct
   7. Assigned is set to "Unassigned"
   8. Issue tags are relevant and not missing tags
4. Other issues
   1. Parent issue set if applicable
   2. Related issues set if applicable such as duplicate issues
5. Files
   1. Only upload screenshots if relevant
   2. Uncheck "Display" for screenshots
6. Code review
   1. Patch applies cleanly
   2. No coding standard issues in test bot
   3. Documentation is clear
   4. Code addresses issue
   5. Code does not change unrelated things
   6. Previous comment feedback addressed
7. Tests
   1. New test code added if applicable
   2. New test code tests the specific issue
   3. Tests pass
   4. Test-only patch fails
8. Manual testing
   1. Issue was reproducible
   2. Patched code fixes issue

A note on "core gates"

There are a number of Drupal "core gates" that aren't obvious to most reviewers for example accessibility or performance related items, so those are best checked by a maintainer or a contributor with those specialized skills. Some items are more easily reviewed by general contributors such as whether or not there should be a change record or if there are missing tests.

But, no one knows everything. If you marked something RTBC and tried to make sure everything was ready, don't feel bad if something is bounced back to "Needs work". Even with checklists, it's easy to miss something. The goal is to try to do as thorough a job as you can to minimize too much back-and-forth on the issue.

Example comment

I've been trying to be very explicit in my comments before marking an issue RTBC. Here's some SAMPLE text. If you use this as a guide, please remove anything that is not applicable to the issue, add anything that is missing, and reword as appropriate. And, don't forget to think as this checklist is not a substitute for stepping back and seeing the whole picture. :)

Thank you for your patch. I am marking RTBC since:

1. Patch applies cleanly.
2. Tests pass.
3. Test-only patch fails as expected.
4. No coding standard problems were found by the test bot.
5. Code is clear and addresses the issue from the issue summary.
6. New test code tests the specific issue from the issue summary.
7. Feedback from previous comments has been addressed.
8. Issue title is clear and relevant.
9. Updated issue summary to use the issue template and clarify issue text.
10. Issue metadata looks okay.
11. Manually tested and patch works as expected. See screenshots.
12. Change record is clear.

Miscellaneous observations

After reviewing a lot of issues the past few months, I've noticed a few things to keep an eye out for:

• "It works for me": Some contributors will mark issues RTBC with a simple "It works for me" type comment or sometimes even no comment. It's very important to be explicit on what you reviewed so that it makes things easier for the maintainers to review your work.
• Interdiffs: For new contributors, they often leave out an interdiff when updating a patch. If the patch is very small, I don't normally comment on the interdiff being missing but, for larger patches, I make a comment that it's missing and link to the interdiff documentation. Having an interdiff makes the review process faster.
• Empty comments: Some contributors will add a new patch but not include a comment. If I see this, I often comment that it is helpful to have a comment even if they think it should be obvious what they did. Empty comments slow down the process because people need to figure out what happened. It's better to err on the side of being "pedantic" and over-commenting than not providing enough detail. That said, if comments are long, they can be hard to follow, so best to use ordered lists for long comments, so that people can refer to the number in the comment.
• Does it need tests: As a rule of thumb, if the issue is a bug fix then it will need a test added. Sometimes this is a judgement call though. If you don't think it needs a test and everything else looks okay, mark it RTBC and make a note that you don't think it needs a test. Just be prepared that it might get bumped back to add a test.
• RTBCing own patch: Occasionally, someone tries to RTBC a patch they created. This doesn't happen that much in core, but is more common in contrib. If the project is their project, then that's fine, but sometimes it's not. You can move an issue back to "Needs review" if you see it was marked RTBC by the contributor who created the patch.
• Old patches: There are lots of issues so many go quiet. If you come across an issue that has been stagnant and has an old patch, if it's old enough, it is good to double check the issue is still reproducible as maybe it was already fixed. If it was fixed, try to find the duplicate issue and close out the old issue and link to the duplicate. If it's not fixed and is reproducible, you'll need to see if the patch still applies cleanly to the current development branch. If not, move it back to "Needs work" and add the "Needs reroll" tag.
• Followup issues: It's really easy to forget the create followup issues if the issue gets marked fixed or closed. Try to create followup issues as soon as they are identified if possible and add it as a related issue (in both directions).

Additional resources

• New issue: Add "Contributor guide" links on issues somewhere
• Old but helpful XJM review checklist
• Super old but helpful post from webchick: Diaries of a Core Maintainer #5: The 6-pass patch review
  
• Drupal docs: Issue etiquette
• Drupal docs: Reviewing Patches
• Drupal docs: Contributor task: Review a Drupal core patch
• Drupal docs: Drupal core gates
• Drupal 9 & 10 core bugs marked RTBC

Errors and improvements

If you notice errors in this post or want to suggest improvements, contact me on drupal.org.

Image credit: Aaron Deutsch

Apologies, my old website is still on Drupal 6 and looks particularly bad on mobile. I've only started posting here again after many years and I've been very busy reviewing Drupal 9 patches and surviving a pandemic. :) Please ignore the cobbler's old shoes for now, thanks!