Last updated 4 March 2017.

On this page

  1. Issue scope
  2. Considerations for issue scope
  3. Examples of good and bad issue scope
  4. References

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 patch. 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 patch. However, contributors must then understand (and review and improve) multiple different contexts in order to complete the issue. The patch 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 patches. (For example, if a word is misspelled in multiple places, it takes about the same amount of time to review one patch whether it fixes one misspelling or all of them, but it takes much longer overall to create, review, and commit six patches that each correct only one word.) Furthermore, such incomplete patches leave Drupal core in an inconsistent state, which is bad developer experience and sets a bad example for future patches. This results in the same problems being continually reintroduced, ultimately making the patches a wasted effort.

Problems with scoping by file group rather than concept (#)

Very large patches are extremely difficult to create and review. For this reason, it can seem like a good idea to create a series of patches 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 patch 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 patch into more distinct conceptual units.

Release schedule (#)

Some types of changes cannot be included in patch 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 patch release. (Reference: Allowed changes during the Drupal 8 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.

Patch size (#)

Large patches are more difficult to create, update, and review. 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 of code. (Reference: 11 Best Practices for Peer Code Review.) As a rule of thumb, patches of between 10-40K are a good size, but this varies with the type of fix. Large patches with many instances of the same simple change can still be easy to create and review, while even small patches with complex new code or documentation can take longer. Good conceptual scope is more important than the size alone.

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 patch 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 patch 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 patches, 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 patches 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.

Special note about coding standards cleanups (#)

Following DrupalCons Amsterdam and Barcelona, we adopted a new process for coding standards cleanups.

  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 patch 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 patch 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 #2622992: Run a spellchecker against core and fix all the errors in comments. Patches 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 patch 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 patch 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 patches 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 patches 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 patches is very unnecessary overhead, especially if the patches are created by the same authors, and a frustration to committers.

References (#)