Reviewing and committing RTBC issues

General advice

You don't have to know everything! :)

If you have any question or hesitation about any aspect of an issue, chat with another committer about it. Subsystem and topic maintainers are also a great resource. If you're not as familiar with a particular system, or you don't understand that one weird hunk in the change set, don't hesitate to reach out to another maintainer for help or a second perspective.

If the issue itself does not provide you with enough information, mark it Needs work and tag it for "Needs issue summary update".

Mistakes (and reverts) are okay

Don't worry too much about making mistakes when reviewing or committing issues, and don't be concerned if some of your commits are reverted.

When you're first starting out as a provisional committer, you should generally commit only to the development branch, and get signoff from a full committer (especially a release manager) before backporting a change to the production branch. This means that there will be several months to make sure a change is correct without any disruptions to production sites.

It's also recommended to start by commenting about RTBC issues that you review in committer Slack, indicating when you think they are ready for commit, and waiting a day or so. This gives other committers a chance to give a second opinion or highlight points you might not be aware of. (Full committers often use this process as well for certain issues.)

Subscribe to the core test results (bookmark me!) for at least the "issue testing default" environment for each actively supported branch (for example, subscribe to both 9.2.x and 9.3.x while 9.3.x is still under development). This way you will get an email if HEAD is broken. (You'll need to update your subscriptions each time a new minor release is branched.)

If HEAD is broken, or if new concerns are raised about an issue you committed, you should "revert first and ask questions later" rather than trying to fix the issue with followup commit. Post a comment on the issue indicating why you reverted it, and mark it back to Needs work.

While reverts might seem discouraging, they actually help by making issues actionable. Reverts also reduce pressure on core committers to get everything right: while dozens of contributors might help with an issue, hundreds of thousands of sites are impacted by any change, so it's best to have full confidence in the changes we make. Reverts allow us to safely address concerns and fix mistakes.

Questions to ask yourself before you commit

There are a number of questions to ask yourself before committing an issue. (You might want to bookmark this section and go through the questions for each issue.)

1. Are there reasons not to make this change?
2. Can it be done in contrib?
3. Is the scope correct?
4. Is it an allowed change?
5. Is it a significant change?
6. Is it experimental?
7. Does it pass the core gates?
8. Has all feedback from reviewers been addressed?
9. Does the issue fit in the "big picture" for current work on core?

Are there reasons not to make this change?

For every issue, ask yourself if there are any reasons not to make the change. Even if an issue in itself looks correct, has support from contributors, and complies with all core policies, that does not mean it is an appropriate change for Drupal core. Anyone can propose a change, but as a committer, it is your responsibility to accept or refuse the change.

For example, there are frequent core feature requests to expose new configuration options in the UI. At first glance, such changes might seem small, useful, and harmless, so why not make them? However:

• Having too many options in the UI can reduce usability.
• Adding new functionality adds a maintenance burden to support it for the remainder of the current major release.
• The configuration might not make sense across the board, or include assumptions about how a site works, or conflict with contributed or custom code with different assumptions.

So even though the change may not seem like a significant change in the core governance, there are reasons not to make it, and those reasons should be explored.

If you can think of potential reasons, set the issue Needs review to discuss them, or consult with another committer.

Can it be done in contrib?

Most feature requests can be solved by contributed modules. To be included in core, a feature should generally be something that most Drupal sites would use. (Also see Are there reasons not to make this change? above.)

Related signoffs:

• It's ultimately up to the product managers to decide whether new features belong in core.
• The framework and release managers provide feedback on how the feature might impact core's maintainability.
• Subsystem maintainers for the subsystems should be given opportunity to sign off on new proposed features in their subsystem.
• Finally, applying the core gates will often require a topic maintainer (especially for usability and accessibility) to give feedback on the issue.

As you can see, many aspects of core can be affected by a simple feature addition. If you have doubts about whether a feature should be added, post specific questions on the issue, add the relevant "Needs maintainer review" tags, and also ping a product manager directly so that they're aware of the issue.

New core features, especially significant ones, can be first proposed as Drupal core ideas.

Similarly, public API additions that are not part of specific fixes should also have similar scrutiny. We want to provide good DX to contrib developers, but part of accomplishing that is limiting our API surface to things that benefit enough users to justify the maintenance costs. A really good DX change will remove more lines than it adds. Tag the issue "Needs subsystem maintainer review" and get feedback from a framework manager if you are unsure (or if a subsystem maintainer is not available or does not respond within two weeks).

Is the scope correct?

One of the most common mistakes contributors make in issues is incorrect scoping. Whenever you look at an issue, take a moment to consider whether it has the correct scope. The core issue scope guidelines have detailed explanations for how to scope issues correctly, including numerous examples.

It might not seem like a big deal if an issue makes a change that's incomplete (but still an improvement) or cleans up something small outside of what's strictly necessary. However, such changes often make more work for other contributors (including you) and can cause merge conflicts. Plus, helping contributors learn best practices will make their future contributions easier, so that other committers' feedback makes sense in terms of the best practice (rather than seeming arbitrary).

Specific examples that often crop up:

• "Do something in filename.php", "Various fixes for X", etc. as issue titles are usually red flags indicating the issue is not well scoped.
• Coding standards cleanups should fix a single PHPCS rule across all of core and enable the rule in phpcs.xml.dist.
• If a bug is discovered that blocks the current issue, a separate blocking issue should be created for that bug.
• While it's tempting to make things "consistent", there should not be changes made to surrounding lines of the change set even if you are asked to change something that's wrong both in lines added by the fix and in lines surrounding it.

Retitle issues to clearly reflect the actual scope, even if only for the commit message and future reference.

Is it an allowed change?

Before committing a patch or merge request, always make sure it is an allowed change for the branch and release phase. It's recommended to initially review the Drupal core allowed changes policy every time you commit a patch to verify that the issue doesn't include any disallowed changes.

Also pay close attention to whether the issue has any API additions or backwards compatibility breaks. The Drupal core backwards compatibility policy has detailed information on BC for many different APIs. Any issue should usually deprecate old code and behaviors rather than breaking BC, even for internal APIs, unless there are significant maintenance or other costs to doing so.

If a fix for a bug must introduce a BC break and the positive impact outweighs the disruption, or if the issue introduces something that site owners need to know before they upgrade their sites, get signoff from a release manager for the break. After you commit it, tag the issue with "9.3.0 release notes" (replace the version with the applicable patch or minor release). Even for bugfixes, disruptive changes and additions should only be committed to the minor (development) branch.

Is it a significant change?

Read over the official decision-making process in the Drupal core governance. (When you first start committing, you should read this every time you review an issue.) The governance defines the concept of a significant change, and it is your responsibility as a committer to ensure the appropriate stakeholders have already signed off for any significant change in your area. (This means someone with that role should have commented on the issue either removing the relevant "Needs maintainer review" tag themselves, or clearly stating that they are signing off on the change.)

Adding a new module or theme is almost always a significant change that should receive signoff from the product, framework, and release management teams.

If any signoff is missing, add the appropriate "Needs maintainer review" tag. If the issue is RTBC and it is only missing product, framework, or release manager signoff, it can be left at that status, since the RTBC queue is a list of issues for committers to review. Otherwise, it should be set back to Needs review.

Don't hesitate to reach out to other maintainers for feedback. It's almost always better to ask for (timeboxed) feedback if you are not fully confident about the issue or if you suspect another maintainer might have concerns. (If you get feedback via a channel other than the issue comments, be sure to document it on the issue.)

Is it experimental?

Experimental modules have less restrictive requirements than stable subsystems. They do not need full architectural review and do not need to pass the core gates. (These tasks can be deferred until the module is ready to be marked stable.)

Does it pass the core gates?

All changes to non-experimental code must pass the Drupal core gates: user-facing changes must pass the usability and accessibility gates, issues that might impact performance in the critical path must pass the performance gate, and all issues must pass the documentation and testing gates. Familiarize yourself with the requirements in the core gates, since they are the most common reason to mark an RTBC issue back to Needs work.

Always take note of issues that may need change records as these are often missed by both issue contributors and committers. Be sure to review the change record itself.

Similarly, issues that make a disruptive change, add a dependency, change enabled coding standards rules, or change site-owner-managed files like settings.php and .httaccess should have a draft release note. See the issue summary documentation for guidelines on writing a good release note.

Almost all issues, whether bugfixes or additions, should add test coverage. (Generally only strings, CSS, and presentational markup don't get test coverage, so these things should always be tested manually, with screenshots.)

Has all feedback from reviewers been addressed?

Scan the comments for unaddressed feedback. Pay special attention to any feedback from other committers as well as topic maintainers or maintainers for the subsystem. You can see who has commented on the issue at a glance by looking at the "Credits & Committing" section.

Does the issue fit in the "big picture" for current work on core?

Take a moment to think about how the issue might impact other ongoing work on core.

• Will it conflict with other important issues?
• Are there other recent commits that might change how it should be addressed?
• Does it fit with the strategic product initiatives?
• Are there special considerations based on the release timeline?
• Does it regress recently adopted best practices?

Use your best effort to be aware of other decisions in the project.

Reviewing and committing

Explain and provide references

Whether you commit the issue, set it back, or close it, describe how you reviewed the issue and your thought process. Try to always provide references for standards and policies, for anything from a documentation coding standard to a core gate to a BC break. This makes changes transparent and also helps other contributors become better reviewers, so that you have less work to do the next time.

If you have hesitations or doubts about an issue, make them explicit in your issue comment. This gives contributors an opportunity to provide more information and helps you both understand the issue.

Marking issues Needs review or Needs work

Follow the guidelines for providing a constructive review: Start by thanking the contributor for something specific. In a separate paragraph, describe what needs to be fixed (with references to relevant documentation!). Close with actionable next steps.

Generally, it's better to mark an issue Needs review or Needs work than to leave it at RTBC, even for small questions, because this makes the issue actionable and improves the signal-to-noise ratio in the RTBC queue.

Commit process

1. Finalize the issue metadata

   Review and correct all issue metadata especially the title but also the branch, priority, tags, and issue type.
   An accurate title is helpful when researching a topic with git.

2. Assign issue credits

   Read the comments and adjust issue credits as needed using the information in How is credit granted for Drupal core issues.

3. Draft your issue comment

   Start writing your comment, remembering to be helpful as explained in Explain and provide references.

4. Delete the vendor directory in your maintainer repo

5. Check out the latest (highest open) development branch

6. Pull the latest changes (with rebase)

7. Apply the patch or MR "plain diff"

8. Run composer install

9. Review what will be committed

   Check that the changes to be committed are what you intended to commit. Use git diff –shortstat to confirm the number of changes agrees with the statistics from the patch or MR.

   The change statistics for an MR is found on the 'Changes' tab, in the far right hand side.

10. Commit the issue using the default commit message in the "Credit & committing" widget

    

11. Review what you committed

12. Push the commit to Drupal.org

13. If appropriate, backport the issue

14. Mark the issue Fixed with your comment

15. Tag the issue for the release notes or release highlights if appropriate

16. Publish the change record