Advertising sustains the DA. Ads are hidden for members. Join today

On this page

Issue scope guidelines for Drupal core issues

Last updated on
30 January 2025

Short URL for this page: drupal.org/core/scope

Issue scope

Issue scope refers to the set of changes included in an issue.

  • In general, Drupal core issues should include only a single scope, for example fixing a single bug with test coverage, or making one specific type of cleanup.
  • The issue title should summarize the issue's specific scope, and the issue summary should outline it in more detail.
  • Additional issues should be created for each additional scope.
  • Plan issues can be used to connect related issues that each have a specific scope.

Scope creep

Scope creep means adding more and more improvements in a single merge request. In the course of resolving an issue, you will often discover additional problems, or come up with bigger ideas for how to improve Drupal. It can be tempting to include these changes in the same merge request. merge request becomes larger and more difficult to manage, and it's never really clear when the issue is "done". In the long term, scope creep ultimately delays commits of your contributions and increases technical debt.

Incomplete scope

Issues that have too narrow a scope (or incomplete scope) add unneeded overhead for the mechanics of creating, reviewing, and committing merge requests. (For example, if a word is misspelled in multiple places, it takes about the same amount of time to review one merge request whether it fixes one misspelling or all of them, but it takes much longer overall to create, review, and commit six merge requests that each correct only one word.) Furthermore, such incomplete merge requests leave Drupal core in an inconsistent state, which is bad developer experience and sets a bad example for future merge requests. This results in the same problems being continually reintroduced, ultimately making the merge requests a wasted effort.

Problems with scoping by file group rather than concept

Very large merge requests are extremely difficult to create and review. For this reason, it can seem like a good idea to create a series of merge requests like "Fix coding standards errors in module A", "Fix coding standards errors in module B", etc. However, this can turn out to be the worst of both worlds: the merge request mixes multiple different concepts and contexts, while also leaving the codebase in an incomplete state.

Considerations for issue scope

Context, conceptual scope, and reviewability

Consider the knowledge the reviewer needs and the tools the reviewer can use.

  • Does it affect one specific concept, or multiple concepts?
  • Can some of the changes be reviewed with a word diff, but not others?
  • Are some changes straightforward while others will require signoff from a maintainer?
  • Do some changes have consensus while others are blocked on contentious decisions?
  • Can some changes be tested with current automated tools while others cannot?

In all of these cases, consider splitting up the merge request into more distinct conceptual units.

Release schedule

Some types of changes cannot be included in merge request releases. Mixing API additions or disruptive changes in with a bug fix will therefore delay that bug fix, preventing it from from being available in the next merge request release. (Reference: Allowed changes during the Drupal core release cycle.)

Consistency, completeness, and "shippability" of the codebase

Keep core consistent. Small, incremental changes across the whole codebase will be more successful, provide better developer experience, and be less likely to regress than big steps in only some of the codebase.

Merge Request size

A good target size for a merge request is a diff size 10-40K, or about 100 changed lines of code. The ideal merge request size varies with the type of fix.

Good conceptual scope is more important than the size alone. Large size merge requests with many instances of the same simple change can still be easy to create and review, while small merge requests with complex code or documentation can be difficult to create and review.

These limits are based on 11 Best Practices for Peer Code Review. That analysis shows that reviewers are twice as likely to miss problems when reviewing more than about 100 lines of code, and many times more likely to miss problems when reviewing more than 300-400 lines.

Merge history, merge conflicts, and other work

Each change should have a distinct commit in our version control history that is associated with a distinct issue, so it is easy to determine when a change was made and why. Mixing different changes in a single merge request also often causes work to be duplicated or issues to conflict with each other. Check for other issues related to an out-of-scope change. (Don't be caught by the fallacy that if it's on the same line or an adjacent line, you should fix it in the same merge request to avoid conflicts. Remember that a word diff can be used to review changes within lines, and that an unrelated change on the same line or adjacent line will still be missing accurate information of "why" in the commit log. Instead, create a followup issue.)

Large initiatives

Large initiatives that will make changes across core should be scoped in iterative steps. For example, an initiative might first add self-contained dependencies, then a new API, then a BC layer for the API, then deprecations and conversions to the BC layer, and finally full conversions. This will be less risky and more successful than trying to change everything in a single patch.

Blockers versus followups

Distinguish steps that must be done before your change can be made from changes that can be done afterward. Blocker issues should be done first, then your intended change, and finally the followups.

Ease of contribution

Sometimes, using a smaller scope than normal for issues can be beneficial during a sprint or for novice contributors making their first patch. However, if one contributor is making many tiny, similar merge requests, that benefit is lost and it is no longer fair or a good use of resources.

Get the big picture

Get a sense of the overall picture before you create a big patch. Search the codebase to get a sense of the fixes needed for your planned change, and document this information in the issue summary.

  • Are there 10 lines that need to be fixed? 50? 500?
  • Are all the lines that need fixing similar, or are there different patterns that emerge?
  • Can some of the changes be automated safely, or does each line require a decision from the developer?

Use this information to decide whether to make the change in a single patch, and to create a plan for splitting the change up if its scope is too big for a single patch.

In general, if a change requires exactly the same fix in multiple places, do it in a single patch. If the change requires multiple different patterns of fixes, create a Plan issue with child issues for each pattern.

How do I fix the scope of a patch?

Sometimes, you will get feedback from the maintainer of an issue that the scope needs to be changed. This is okay! Just follow the maintainer's suggestion to organize the issues more effectively.

  • If the scope of your issues is too narrow, expand the scope of one issue to include the recommended scope, and mark other issues as a duplicate of that one. Then, apply the merge requests together locally at the same time and create a new, combined patch. (If others were involved on specific issues, be sure they comment on the new main issue to get credit!)
  • If the scope of your issue is too broad or unspecific, create a followup issue (for specific out-of-scope changes) or a Plan issue (for conceptually related changes that do not fit in a single scope). Then, split up the existing patch. You can use git add -p or git merge -i to select only some changes for a new patch.
  • If the scope of your issues is problematic because it conflicts with other work, read the other issues carefully, and incorporate your work on those issues.

How to scope specific types of issues

Unused local variables

Each unused variable must be researched to find out why it is not used. For example, the variable may be unused due to to broken code or incomplete testing. This is best done in a single issue.

Git is a useful tool for this work, particularly git log -S "SOME TEXT" and git blame. Both of these will help locate the commit when changes involving the variable happened.

Coding standards

  1. If you are proposing a new coding standard, follow the policy on the Coding Standards project.
  2. If you have found a bug where core is not compliant with an existing coding standard, first make sure the Coder project detects the bug. Otherwise, if it is possible to add a Coder rule, create an issue in the Coder queue to add it. (For JS and CSS coding standards, file issues against the existing core .eslintrc and .csslintrc rule sets.)
  3. Once an issue can be detected by Coder, if core fails the rule, file a merge request to add that rule to the exclusion list in the core phpcs.xml.dist file.
  4. Next, check for whether there is overlap with an existing issue in the Core coding standards meta issue, or add a new child to that issue. Use Coder to identify all the failures of that standard in core, and summarize these in your issue's summary.
  5. If there are too many failures (or too many different patterns) to fix in a single patch, use the guidelines in this document to break your issue into multiple steps. You might want to get maintainer feedback about your planned scope before proceeding. The examples below might also help.
  6. Once core is compliant with the rule, file a final merge request to remove it from the core exclusion list.

Do not mix fixes to documentation coding standards with adding missing documentation or improving the existing documentation. Coding standards fixes alone can often be checked with automated tools, whereas (re)writing documentation requires review for technical accuracy as well as skill reading and writing in English.

Spelling errors

For spelling errors, see #3122088: [Meta] Remove spelling errors from dictionary.txt and fix them. merge requests fixing typos in a single file will not generally be accepted.

Examples of good and bad issue scope

Good scope Bad scope Why
Foo widget returns 500 error when submitted with bar configuration (includes bug fix and test coverage) Foo widget returns 500 error when submitted with bar configuration (potstpones test coverage to a followup) Test coverage is part of the scope of a bug fix, and the code base is in an incomplete state without the test coverage.
Miscellaneous bug fixes for foo widget Each issue should fix only one problem.
"Rewrite foo widget using new best practices (fixes 500 error with bar configuration)"

Rearchitecting functionality requires more effort and expertise than fixing a bug, and probably cannot be backported to the current production release.

Instead, focus on fixing the bug in the simplest way possible, and create a followup for the rearchitecture. (In some cases, refactoring code might actually be the fastest and cleanest way to fix a bug. Use your best judgment or ask a maintainer for feedback if you're not sure.)
Replace deprecated entity_load('file', ...) with File::load() (directly replaces function calls only) Replace deprecated entity_load('file', ...) with File::load() (updates each call to use dependency injection)

1:1 replacements do not require any special expertise, can often be done with automation, and can be quickly reviewed with a word diff (like git diff --color-words.

In contrast, a conversion to dependency injection would require contributors to review the code itself and determine the best practices for injecting the dependency. What was a simple novice issue that could be reviewed in 10 minutes or less would turn into a much more advanced merge request that required architectural review.

Create a followup issue for the dependency injection instead.
Replace deprecated entity_load() for all entity types While this would seem to leave core in the most consistent state, it's not a 1:1 replacement (there are many different "before" and "after" states, each of which is its own context) and the size of the change will also be too large to review in one sitting. Instead, use a plan issue with one issue for each entity type.
Replace all deprecated functions in the File module This is the worst of both worlds. It cannot be reviewed easily by scanning a word diff, it requires understanding and agreeing on the correct replacement for many different deprecated functions, and it leaves core in an inconsistent state with most usages of all the deprecated functions left behind.
Add the Layout Plugin API to core Replace the core block system with new Layout API and UIs Major initiatives should be done in scoped steps, not all in one patch.
Replace else if with elseif according to coding standards Clean up coding standards in the Migrate module

Fixing a single coding standard can be automated, reviewed easily, and tested automatically, ensuring it does not regress in the future.

On the other hand, fixing "all" coding standards is difficult to complete and still leaves core in a state where there are other bugs with each standard (which in turn means automated testing still cannot be enabled).
Add missing @param documentation to FooBar namespace (as part of a plan issue to add missing @param documentation) Add all missing @param documentation in core Unlike the previous example, writing new documentation requires understanding the code, technical writing skills, and possibly maintainer input. It is simply not feasible to add all the missing parameter documentation to core in a single patch, and furthermore it could require the conceptual expertise of every component maintainer for components missing this documentation.
Add missing documentation to Foo module While this focuses on a specific conceptual area, it will span multiple different kinds of coder failures, and might result in a merge request that is too large to review properly. If there are just a few instances of missing documentation, this scope could work well, but be careful not to make merge requests that are too extensive or undercut the per-rule work being done in #2571965: [meta] Fix coding standards in core.
Add missing @param documentation typehints for typehinted non-scalar parameters Add missing @param and @return documentation typehints to core

A check for missing documentation typehints is easily automated, and it is easy to check the documentation against the function signature when the signature also has a typehint.

On the other hand, scalars, mixed data types, or other non-typed parameters can require more careful study of the code to determine what values are allowed, and return data types require understanding all code paths.

Instead, create a plan with several children, addressing the easy/obvious cases first, then the more complex cases, and the @return docs entirely separately once the @param docs are known to be correct (since the inputs will affect the outputs).
20 different merge requests each adding missing data type documentation for one method Once a reviewer is in the context of checking parameter data types, checking multiple different parameters is fairly straightforward. Splitting such improvements into many individual merge requests is very unnecessary overhead, especially if the merge requests are created by the same authors, and a frustration to committers.

References

Related Content

Allowed changes during Drupal core release cycles

This policy explains what changes can be made to different core versions during the Drupal core release cycle.

Tags

issues

Help improve this page

Page status: No known problems

You can: