Problem/Motivation

Coding standards changes are allowed in minor releases but not patch releases since they introduce disruptions, and especially disruptive ones are scheduled during the beta and release candidate phases. For people checking against the core standard, backporting new rules can suddenly cause regressions in their compliance. (This came up again in a meeting with Mixologic at DrupalCon Vienna where we were discussing what we needed to do to enable automatic coding standards fixes via DrupalCI; the discussion underscored the need to be careful about not backporting rules in the future, although I forget all the details.) This is also even true of new rules that are technically a part of our standard but that core does not yet comply with.

On the other hand, there's also a risk in not backporting the cleanups, because the branches diverge in noisy ways that break cherry-picks of actual bugfixes that need to be backported.

In the past, changes weren't backported to the previous branch. With all the well-scoped, straightforward progress that's been happening on coding standards since DrupalCon, I think we sort of forgot about this.

Proposed resolution

We haven't had an 8.4.x patch release yet, so we can still avoid accidentally disrupting production stuff.

  • Revert the changes to the ruleset only since 8.4.0 on 8.4.x only.
  • Leave the cleanups themselves in 8.4.x, since a committer already considered them safe enough for backport.
  • Leave 8.5.x as it is.
  • In the future, backport cleanups at committer discretion if they are safe and the disruption of the cleanups themselves is limited, but enable the new rule only in the new minor branch.
  • This mostly won't affect the coding standard compliance of backports, because the checks only run on the minor branch for the commit, not on the cherry-pick, and the 8.4.x ruleset will be pinned on the less restrictive version from 8.4.0.

Remaining tasks

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

xjm created an issue. See original summary.

xjm’s picture

Issue summary: View changes
Status: Active » Needs review
FileSize
5.58 KB

Here's the patch.

xjm’s picture

Also tried to clarify this in the allowed changes policy:
https://www.drupal.org/node/2581501/revisions/view/10398592/10699558

catch’s picture

Status: Needs review » Reviewed & tested by the community

This makes sense, keep the clean-ups in both branches but don't backport the rulesets. And yes I at least have been backporting coding standards changes since it means that any patches against that section of the code base will apply against both branches, and they're so much tidier than they used to be, but I can see why we'd want to avoid backporting the rulesets especially since all 8.4 patches get tested against 8.5.x too.

  • catch committed 4264f4c on 8.4.x
    Issue #2919983 by xjm: Restore the 8.4.0 phpcs.xml.dist on 8.4.x
    
catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.4.x ready for the 8.4.1 release.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.