Problem/Motivation
The Drupal core backport policy is confusing to contributors.
Reference: https://www.drupal.org/core/backport-policy
- The current state of an issue that applies to multiple versions is often unclear, and especially confusing and frustrating to contributors who are not already invested in core development.
- Maintaining one issue for multiple versions of Drupal makes it difficult to track which parts of the discussion are relevant to which version of Drupal.
- The codebase (and therefore solutions and patches) can differ drastically between major versions.
- Since patches are only tested against the current version, it takes noise and overhead to maintain patches for multiple versions.
- Issues that have long since been fixed against one version may linger in confusing states.
- The "needs backport to DN" issue tags are not discoverable, unreliable, and different from any other "needs" tags.
- Few people understand what "Patch (to be ported)" means anyway.
- The backport process breaks issue metrics and issue credits, and causes confusion with multiple commits on a single issue.
- Developing patches in parallel may help reduce derailment and noise due to differences between branches or differing contributor priorities.
Proposed resolution
- Issues may be backported from the 8.x development branch directly to 7.x, skipping the current stable branch due to stricter commit policy for patch releases.
- While we will default to working in 8.x first and backporting, the actual 7.x backport should happen in a separate issue so that fixed 8.x issues stay in 8.x issue searches, issue credits are maintained etc.
- Separate 7.x issues may be opened either when 8.x patch is committed, or before then if appropriate. For example in around 50% of backport cases, the 'problem/motivation' is the same but the 'proposed solution' or actual code changes may be very different. Also given there are still more production sites running 7.x, there is currently broader scope for manual testing of patches on production. Issue relationships should always be used so that people are aware of both issues. 7.x issues should be marked 'postponed' on the 8.x issue unless there's a good reason not to wait on the 8.x issue to resolve.
- Since both 8.x and 7.x are in production, we won't automatically require that all issues are fixed in 8.x before they're committed to 7.x, this will be down to committer discretion. i.e. we will explicitly allow fixes to be worked on and committed in parallel when it makes sense.
Maybe:
- (maybe) Provide a special issue reference field for "This issue in other versions of Drupal".
- (maybe) Allow contributors to initially auto-follow the issue against other versions of Drupal unless they unfollow it. (Similar to how "assigned" works?)
Cons of this approach
- Creating a second issue adds overhead. (Mitigation: issue clone feature that Dreditor provides, possibly a d.o feature specifically for filing issues against multiple versions in the future.)
- Discussion relevant to both versions may become fragmented. (Mitigation: "This issue in other Drupal versions" linked prominently, explicitly postpone earlier version's issues where appropriate.)
- Contributors following one issue might miss updates to the other. (Mitigation: Add a feature to auto-folllow the issue in other versions, but allow contributors to unfollow one but not others as they prefer. This is also a feature for people who are only interested in working on the issue against one version and not the other).
Remaining tasks
Discuss.
Original report from @cmonnow
I'm not sure where this should go so please advise of the best place to vent.
Before I forget or move on, I felt I should point out a common frustration in the current method of assigning version-specific Drupal core issues.
Here's the usual case for most bugs from around 2011 onwards. A helpful Drupal 7 user points out a bug they experienced in their stable Drupal release, say it was "7.8" for nostalgia's sake, and after much deliberation a subsequent helpful user identifies the naughty code and suggests changes to the core. Since it's often considered too late to make pronounced changes to 7.x-dev without risking backwards interoperability, the first step is to investigate whether the issue affects or will potentially affect 8.x.
After a few years of chatter the original issue would be updated to version 8.x with a few half-hearted calls for backporting to 7.x. In 2015 someone spots an issue and after some Googling discovers the sam issue from a thread started in 2009. By now the issue is marked as Closed (fixed) for Drupal 8 and a series of patches for Drupal 7 had their spotlight for a few weeks in 2012 and inevitably, were eventually ignored.
What I don't understand is why we don't maintain distinct issues for Drupal 7 and 8? Isn't that what the related issues feature can be used for? Wouldn't it make more sense to have a separate issue fixed for Drupal 8 and Needs Work or Closed (Won't Fix) for Drupal 7?
Only when an issue is finally "solved" in Drupal 8 might we see Patch (to be ported) and 7.x listed as the version once again (e.g. https://www.drupal.org/node/998898) - but isn't it unnecessary to pollute an issue's patch and discussion history with a version that may need entirely different strategies?
Comments
Comment #1
xjmThanks @cmonnow for posting this. This is something that has been on my mind for a long time.
The issue workflow for core patches that apply to multiple versions is indeed confusing to contributors. It also has a negative impact on our data model, patch and testing workflow, metrics, etc., because we always have to make multiple commits on one issue, which in itself is confusing. Issues that were long since fixed in Drupal 8 linger in confusing states. And finally the fact that a need for backport is buried in a difficult-to-discover issue tag is also not good contributor experience.
The two reasons that we don't maintain separate issues for different branches of Drupal are:
That said, I've felt for awhile that the backport workflow is broken, and it would be better to either:
The disadvantage of 1 would be that it still doesn't eliminate the problem where there are still patches, commits, and discussion that apply to only one issue, unless we radically changed other parts of the issue queue as well.
The disadvantage of 2 would be the overhead of creating another issue and summarizing the previous discussion. Personally I think 2 would have clearer UX and workflow, be easier to implement without immediate changes to d.o (though we still should work on the UX in the long-term), and have a cleaner data model.
A related confusion is that the version field includes both branches and specific alpha, beta, and patch releases, but our policy is to always file issues against the relevant dev version. The specific release is only used in rare cases to document the specific version someone identified a problem in, and we always instruct them to test the latest dev anyway to see if it is resolved.
To be clear, I am not suggesting changing the backport policy. I'm suggesting changing our tools and/or processes in a way that makes the backport policy make sense to people.
Comment #2
xjmAlso, I was strongly tempted to be a smartass and put the "needs backport to D7" tag on this. ;)
Comment #3
cilefen commentedWould it make sense to add another type of related field to issues for "This issue in other Drupal versions".
Comment #4
yesct commentedOne of the things that happens when the work stays on the same issue, is that the *people* are already following the issue. (sometimes helpful, sometimes annoying in that people unfollow as it moves to a different version).
*If* we had different issues for different versions... postponing the ones that will get backport seems worth considering as part of the process to help make it clear that it needs to go in another version first.
Comment #5
joelpittet@cilefen was thinking along those lines too. Some kind of joined and separate issues.
The contributors tend to follow and work on issues that directly affect their work, and that incentive usually yields fast turn-arounds on problems or features. If/when it get's bumped to the next version, those contributors who originally contributed have their flow, incentive and momentum broken(additionally so with the vast learning curve between versions).
Any way we can keep the momentum moving on the initial reported version and creating linked issues I believe would be a big benefit to the end goal(committing the fix/feature, reducing burnout). This likely would create a new problem of likely more maintenance but I'm hoping the gains in issue turnaround time would offset that extra work. And maybe a bit better with issue noise if people are trying to push both version on the same issue.
Comment #6
fabianx commentedI agree that we should always split up D7 and D8 issues where both are affected - especially for criticals and majors, where visibility can really muddy the waters.
The backport policy has its merits though and I also vote for it to not change.
Just the question to the D7 issue: When will this be committed, will still be: When issue XYZ is in D8.
Comment #7
pounardI think this policy should be removed, D7 and D8 are not the same product anymore and using the same issue for D7 and D8 will remove the ability to have two teams developing in parallel, D7 maintainers cannot maintain anymore because they have to wait for all D8 contributors to agree on what is in most cases a radically different solution in a radically different codebase for something which only does look like the same at the conceptual level. D7 and D8 have diverged and are not the same software anymore: please get over it. This backport policy is long dead, maybe it did a few years back, but now it does not serve no purpose anymore.
Just linking to this rant https://www.drupal.org/node/2297817#comment-9795213
Comment #8
xjmComment #9
David_Rothstein commentedIt would be great to have a multivalued "Affected versions" field on issues so that it's possible to tag an issue as affecting more than one version of Drupal, in a cleaner way than the "needs backport to D7" tag.
Absent that (or even in addition to that) having two separate issues seems OK, and might help clear up some confusion.
But I think a lot of the problems people see with the backport policy are in theory supposed to be addressed by this part of it (quoting from the "Current process" section of https://www.drupal.org/core/backport-policy):
The problems seem to come when there are issues for which the above is essentially impossible to follow, because they touch some area of core that has undergone overwhelming changes between Drupal 7 and 8 (as @pounard said) and even if there is a "minimal fix" in Drupal 8 it might look very different than the Drupal 7 fix. I do not know what should be done about that.
Comment #10
webchickWhat we risk with two separate issues is:
a) Discussion being "forked" in two places with the same architectural issues that are common between the two being rehashed separately. Worst-case, this rehashing is done in different ways by different people, reaching different conclusions and the two arrived-at solutions are incompatible.
b) Proper credit not carrying over between branches, so people who did 100% of the work in the D8 issue getting 0% of the credit in the D7 issue. (I worry about this a lot with Security issues for example, and try and go out of my way to hunt down the proper people to credit patches with, but not sure this is consistently done.)
c) It's easier to introduce a D7 commit before the D8 commit is ready, or not remember to revert a D7 commit when a D8 commit is reverted, etc.
It's being argued that those downsides are outweighed by the indisputable fact that the current process confuses the crap out of people, which it definitely does.
However, I think it would be interesting for someone to do some analysis to figure out which is the most common scenario: a 7.x patch that's 90% the same as its 8.x version or a 7.x patch that's 10% the same as its 8.x patch. (Don't know how this would work really... maybe a D.o query to find all closed (fixed) issue IDs with the "needs backport" tag and then search both the 8.x and 7.x logs for those issue IDs and compare the diffs, adjusting for known stuff like /core/ and whatnot.)
Because while I think we can all recall several notable examples of the latter, I think the former is actually way more common than you'd probably think. Here's a random example in my queue right now: #2356983: Fix SystemQueue::claimItem() returns items in the wrong order (resolves locale test failures on PostgreSQL) And if "90% the same" does work out to be the most common scenario, we certainly don't want to make that much harder in order to make the 10% case easier. (We could always do a one-off child issue in really bad 10% cases, and in fact have several times in the past.)
If it turns out I'm wrong and the "10% the same" case is the much more common case, then two separate issues mostly makes sense. But to get around a) it'd be great if version-specific sub-issues were only spun off when the "technical direction" was agreed upon (maybe with a tag indicator or something). Because coding standards nitpicks happening in two places isn't a big deal; in a lot of cases the coding standards nitpicks are different between versions anyway. What's not (usually) different between the two versions is the general idea of how to fix the issues, what edge cases to be aware of, etc. It's valuable to have only one place where everyone interested in solving an issue can go to discuss the "how" and the "what" can happen wherever it needs to happen.
So in that respect, we might want to actually make 3 issues, at least on hairy problems: one for the main "discussion" where technical direction is discussed and agreed upon, one for the 8.x code and one for the 7.x code limited to patch review only. But without some extra tooling like David suggests, I can't really imagine this being workable. :\
I also wonder in general how much of the confusion around this could be mitigated with issue queue UI tweaks versus workflow tweaks. For example, in the metadata box, have "Drupal 7 status: / Drupal 8 status" or maybe "quick link" to the latest patch for each version, etc.
Comment #11
xjmSo I spent way too much time investigating what @webchick suggests in #10 by doing some whacky commit log parsing and diffing and php-bash-executing madness. The conclusion? There are over 250 backported issues where the Drupal 8 change was substantively different from the Drupal 7 change (more than 10 differences in the actual code changes, ignoring differences in file paths, function/method/class names, etc.). Back in 2011 and 2012, it was very common for a backport to include no substantive differences between versions, but more recently, it's more than half of backports that include more substantive changes.
Comment #12
cilefen commented@xjm Thanks for doing that analysis. That really helps.
Am I correct to assume we may backport fixes from 8.1.x to 8.0.x? If yes, the patches will likely be as similar as 7-8 in the early days.
Comment #13
webchickThanks a lot for digging into that.
Ok, then it sounds like the situation is that:
- At the beginning of a new major branch, almost 100% of patches are 100% backwards-compatible with the previous major version.
- Over time, as major API changes are made in the new major branch, this percentage drops for any issues that touch those APIs.
- Once the next major branch is in a "mature" / close to release state, the percentage of patches that widely diverge in their solution is around 50%.
So given that, it seems like the best approach is to stick with a single issue in most cases, always against the latest major version. Sub-issues for other major versions should only be done when solutions will diverge significantly (which will be between 0-50% of backportable issues) and where it's necessary to do this, leave said sub-issues in a postponed state until the technical direction is decided in the "parent" issue.
Comment #14
xjmI can see the case for #13 in terms of keeping the issue histories consolidated -- and definitely for 8.1.x vs. 8.0.x -- but I don't think it actually solves the frustrations with (1) the backport tag (2) confusing issue histories (3) metrics/history being totally and utterly broken and wrong for nearly 10% of all commits.
My recommendation would be to always create a separate issue for the previous major version, but possibly to use the same issue for minor versions for the reasons @webchick lists. (Edit: Because we will be opening 9.x precisely with the intention of APIs diverging from 8.x at that point a couple years from now.) Though it still doesn't fix my metrics problem. :P
Comment #15
markhalliwellCan't we just use separate issues for each major and link the two via the parent relationship field, where the backport issue is the child and postponed until the current major issue is fixed? I don't see how or why we have to bombard a single issue now that d.o has these in place (and underutilized as part of the workflow process IMO).
Comment #16
webchick...because you want to bombard a single issue when it comes to discussing the problem space and possible solutions. You definitely do not want to do that 3+ times with different people.
Comment #17
webchickWas talking to some co-workers about this, I remembered that https://events.drupal.org/losangeles2015/sessions/issue-workspaces-bette... is a thing that's hopefully coming later this year, and realized that we might want to hold off any changes to this policy until that's more fleshed out. The short version is issues become their own "repositories" (actually Git namespaces off the core repo) which will allow for multiple branches per-version, etc. That could theoretically solve a lot of the visibility and cross-talk problems between versions.
Comment #18
pounardI don't like the backport policy because it does not ship with an "upport policy" that would allow some D7 trivial patches to get in real quick then once this is done open the discussion about the development release.
All the theoretical discussion is fine here, I understand every point and they make sense, but meanwhile in the real life some Drupal 7 patches are blocked for more than a year. This issue is the mirror of what's happening every time a D7 issue is turned into a "D8 with backport" issue, people are discussing theoretical details while the stable version goes to sleep for an undetermined amount of time.
Comment #19
webchickYeah, people do indeed dislike that aspect, but no one's ever come up with a good answer for how we avoid introducing regressions in the dev release, while also not preventing it from shipping, ever.
Comment #20
catchAt least where the actual proposed fix for the issue is different, I sometimes ask for separate 7.x and 8.x issues, a good example is #2425259: Router rebuild lock_wait() condition can result in rebuild later in the request (race condition) vs. #356399: Optimize the route rebuilding process to rebuild on write. In that one, the root cause is exactly the same, but the way we fixed it was completely different. However all the important discussion in that issue happened before it was split. Having two critical issues means we don't run into the situation where it's fixed in 7.x then forgotten about for 8.x, but that wouldn't help as much for major/normal. Also in that particular case applying the 8.x patch first doesn't help 7.x stability because it's so different.
I'd be completely opposed to reversing the current policy, because we'd end up with many more situations like #2419941: Check every Drupal 7 contrib SA that may affect Drupal 8 core modules. Also that menu race condition/lock stampede technically applies to 6.x too, but I don't think anyone is suggesting starting issues in 6.x if they apply and forward porting through two releases.
I do think it would be useful to have an 'affected branches' field - then we could drop the backport tag which is inconsistently applied anyway. And a 'fixed in branch' field would be handy for when the commit happens. This also applies to contrib just as much I think.
Also the dynamics of this will change over time. Once 8.0.0 opens, neither 8.1.x nor 9.x will be opened immediately, so the 'current stable release' will also be the target for nearly all patches (a situation which has more or less never been the case before). Once 8.1.x opens we'll need to backport relevant patches to 8.0.x, but once 8.1.0 is tagged 8.0.x support is dropped, so we don't have to backport between 8.x.x versions again until 8.2.x opens.
Comment #21
xjmComment #22
xjmComment #23
catchI opened #2650682: [policy, no patch] Update backport policy to reflect 8.x minor branches and 7.x but then realised it's closer to the proposed resolution here than I thought. Closed as duplicate but going to update the title and issue summary here.
Since the allowed changes in patch releases are much stricter for 8.0.x than either 8.1.x or 7.x, we fairly often commit patches to 8.1.x then move them directly to 7.x for backport. This could result in changes getting into a 7.x stable release faster than they get into a stable 8.x release, but generally we don't care - the important thing is they get into the correct 8.x and 7.x branches as quickly as possible. I think this bit is pretty uncontroversial. Sometimes people complain about patches not getting committed to 8.0.x, but they don't complain if they get committed to 7.x.
What we haven't really tackled is the following two issues though:
In the case of critical or major issues where the optimum change requires potentially breaking @internal changes, but it's possible to fix in an ugly but non-breaking way, we sometimes commit to 8.1.x, then move to 8.0.x for the ugly-non-breaking fix. However, 8.0.x's lifespan is only six months, and on the day that 8.1.0 is released, that 8.0.x backport is never going to happen and no-one should care. But the issue might still affect 7.x, which is going to be around for years still. Therefore even if it's possible to fix something in 8.0.x, this shouldn't block fixing it in 7.x.
Then as above, a lot of bugs in 7.x and 8.x have identical steps to reproduce, but the proposed solution is too different to really be called a 'backport'. In those cases, the work on the 8.x branch and 7.x branches, apart from the initial investigation of the bug itself, can't actually be re-used.
Then on top of that, we have a convention of allowing 'competing' issues in 8.x - where the problem might be the same, but the proposed resolution might be very different, linked together via the related issues field, sometimes one issue postponed on the other etc.. And just in general lots of unintentionally duplicate issues. This can mean that the issue tagged for backport to 7.x ends up being a duplicate of another 8.x issue, while the fix from other 8.x issue is not backportable to 7.x at all and people working on it might not know about the 7.x applicability.
And finally, some issues, while the code is similar, may be a lot more severe in 7.x than they are in 8.x (i.e. a performance improvement to a function that's used all over the place in 7.x but is semi-deprecated in 8.x and only used in a couple of places). For something like that, if a fix goes into 7.x but not 8.x, then 8.x hasn't 'regressed' from 7.x.
However, we still want to maintain the following:
- as much as possible, avoid duplicate issues and effort
- ensure that issues get fixed in all active branches as quickly as possible
- avoid some issues going 8.x -> 7.x while other issues go 7.x -> 8.x since that would be impossible to keep track of.
Proposed resolution:
Amend the backport policy to include the following:
1. Issues may be backported from the 8.x development branch directly to 7.x, skipping the current stable branch due to stricter commit policy for patch releases.
2. Where issues have the same Problem/Motivation but different proposed resolutions (either completely different approach, or significantly different code), allow separate issues for both 7.x and 8.x linked together via the 'related issues' or 'parent' field. We still might want to fix issues in 8.x first, so can always postpone the 7.x issue if it's definitely not going to get committed before the 8.x issue, but equally if there are just two similar, straightforward, bugfixes, they might be worked on independently.
Comment #24
catchComment #25
catchComment #26
catchAlso given this affecting a lot of backportable issues, bumping to major.
I don't think we should always require two issues, but in terms of metrics etc., maybe the following adjustment to the issue summary:
1. If it makes sense to work on 7.x and 8.x issues in parallel, open two issues immediately, linking the two and explaining why.
2. If the 8.x issue should be fixed first, either postpone the separate 7.x issue, or wait until the 8.x issue is being closed before opening the backport issue.
That way there are eventually two issues, but we keep things in one place until it's necessary (which will be different times for different issues).
Comment #27
xjmFor me, this is a natural extension of our allowed changes policy for minors vs. patch releases, and is acceptable for the same reasons that we agreed it was acceptable to just commit 8.x-only patches to the minor straightaway, because the fix is still guaranteed to be available in 8.x on a predictable, scheduled date in the future (e.g. April 20 in the current release cycle phase). (Previously I was concerned that we needed either #2598382: [policy] Adopt a 6-month feature release schedule for Drupal 7 (similar to Drupal 8) and use pseudo-semantic versioning or #2598662: Decide when Drupal 7 (and Drupal 8 and later) should enter the LTS and security-support-only phase in order for this to make sense, but that was before we'd sorted out the minor version backport policy.) I think this change will be easy to explain and non-controversial.
+1 for this as well. It clearly makes opening a separate issue correspond to the need for a separate solution, and is more or less in line with the last part of what @webchick says in #13.
So the risk with working on them in parallel (rather than postponing the 7.x issue) and especially with committing the 7.x issue first is that it might be unclear whether or not the 8.x fix has landed, and there's always the change that the 8.x issue might get stalled indefinitely, resulting in an implicit regression. Having a dedicated issue reference field for "this issue in other versions" could help (and it could be part of our process to check on the status of the other issue before commit), but barring that, we'd need to keep a handle on it in the issue summary or maybe with a new issue tag or something.
The other option is the postponed issue as discussed earlier, which has the advantage of making it very clear when the 8.x issue has landed, but the disadvantage that the 7.x issue gets stalled for no reason (and after all 7.x sites might want to run a patch until their fix is in core).
How do we decide when this is the case? If the code is similar in both branches? Other considerations?
Also, is this proposing always having the separate 7.x backport issue? I'm still in favor of that, and I think it would be easier to explain and document, but @webchick's concerns might still apply.
Comment #28
catchI don't think this is a problem now that 8.x is out of development, combined with not having 9.x open yet.
We have lots of bugs in 8.x and lots of bugs in 7.x, both may or may not affect production sites to various extents. Fixing an issue in 7.x doesn't introduce a regression in 8.x - because 8.x has already been released with that bug in it - and it's going to have that bug in it whether it's fixed in 7.x or not, until an 8.x patch is RTBC and committed.
Also it's not possible to update from 7.x to 8.x yet for 99% of sites, so the scenario of having a 7.x site with a bug that gets fixed, updating to 8.x and experiencing the bug coming back from the dead isn't really happening yet. As long as we always track clearly whether the bug affects both versions, we're not at risk of losing bugs because they get fixed in one version and not another. This is a big advantage of allowing parallel issues, as opposed to a forward port workflow (which I'm still resolutely opposed to).
For 8.x and 9.x it will be very different again - because we should theoretically be able to cherry-pick a lot of patches back from 9.x to 8.X.x when we get there. For ones that aren't a straight cherry-pick, we can open an 8.x backport issue and postpone it on the 9.x fix.
So we're going to have to keep tweaking this a lot, but I think it works for the situation we're in now, and we're going to be in that situation for a decent amount of time.
I do think we should always open a separate 7.x backport issue - I get very confused when I think an issue is fixed, see it CNW with updates, then find out it's the 7.x backport that's still running. This takes up a fair bit of time and doesn't help blood pressure either.
Comment #29
catchUpdating the issue summary again after more discussion in chat with xjm.
Assigning to webchick to see if the discussion and proposal here allays her concerns. I'm also going to ping David Rothstein in irc since this clearly affects what happens to 7.x issues a lot. Once this issue is RTBC we should explicitly assign to David if he hasn't fed back already.
We definitely lose something due to fragmentation, but I think it's going to be outweighed by keeping the individual issues themselves more focused. And similar issues which are documented as such are a lot less harmful than accidental duplicates which aren't.
For me personally, the ability to close 8.x issues immediately after commit would save many 'didn't we fix that three months ago wtf!??!' moments when I see the issue at CNW in my tracker, only to realise it's being ported to 7.x.
I also think that apart from very severe bugs, our "don't have regressions in the newer release" doesn't really apply any more. We currently do not have a purely-in-development version of Drupal core. So any issues that are open, are open against a version that's used in production. This means we won't forget things that were committed to 7.x and not 8.x unless they're forgettable. Similarly, there are hundreds of issues which are theoretically backportable to 7.x, but in reality are never actually going to get fixed in that version, it'd be nice if the 8.x fixes for those had a fixed issue attached to them.
Comment #30
David_Rothstein commentedSounds reasonable.
If the proposed solution is conceptually similar but the actual code changes are very different then I don't know if having two issues open at the same time is a good idea as it will just lead to duplicate discussion. The code isn't what matters much; the ideas of how to change or fix something are.
There are cases where the proposed solution is also conceptually different and for those I think it does make sense, but I also think they are pretty rare.
Why does it matter that both 7.x and 8.x are in production? Once an upgrade path between 7.x and 8.x exists (assuming one eventually does) regressions will start to matter to individual sites at that point.
And even without that, I always thought it was also about ensuring that the Drupal project itself moves forward without regressions (i.e. isn't avoiding regressions the whole reason we have automated tests?). If issues are fixed in 7.x before 8.x then that all goes away.
Wouldn't this also hurt issue credits, since people who worked on the 8.x issue could not be credited on the 7.x issue unless they comment on it?
Overall I do see some potential advantages in having separate issues, but there's also disadvantages (more work and more complicated to maintain). And I think the advantages are mostly working around drupal.org limitations (such as #66484: Allow issues to be filed against multiple versions/branches. and #2514634-15: A user's profile should clearly show how they contribute) which in theory could be fixed directly.... although I realize they may never be so this could be the next best thing.
Comment #31
David_Rothstein commentedComment #32
xjmThis is true -- #2474609: Not possible to credit people who didn't comment in an issue and #2501421: Cannot uncheck people who submitted patches from the issue credit UI will still be d.o issues, but they are d.o issues that need to be fixed separately anyway. And the separate issues per major core version also allow us to work around what I think is a trickier problem, which is #2652152: No credit granted if patch is set to "patch to be ported" and back to Drupal 7.x. Plus there is the trickle-down credit imbalance, where 8.x contributors get commit log credit for the 7.x issue even if they did not remotely help with it, whereas 7.x contributors only get mentions in the 7.x log. So I think overall it's a net gain in terms of managing issue credit.
For me this is more about the overall management and accumulation of technical debt rather than the individual regression. When 8.x was in development, the backport policy was largely a way to force a constraint on 8.x's technical debt relative to 7.x. There is still a risk, which I think we manage as committers by taking the status of the related issue into account and postponing the 7.x issue's commit on the 8.x one sometimes or all of the time. But it's not as difficult for us to manage as it was when 8.x's technical debt was spiraling out of control already.
Maybe the best thing to do is to start filing separate issues, but hold 7.x RTBCs until the 8.x issue is in? That gets most of the advantages of having separate issues (like testbot for both branches and a workable patch for 7.x that people can at least use), and allows us to make sure we can manage the issue crediting and risk of fragmentation before risking that we introduce regressions. I think this will still be okay given the higher commit and release rate for 8.x, and the overall rate of fixes for 7.x will hopefully be somewhat improved. And if that turns out to not work well, then we adapt our tools or process as needed. Does that sound like a good balance between the different concerns?
Thanks @David_Rothstein!
Comment #33
catchJust to say that @xjm's post in #32 covers exactly my own views on this as well.
Especially this bit:
This is the big difference now that 8.0.x is out, and with the new release schedule in general. 8.x being open for development for so long while only 7.x was in production was not a good situation for fixing bugs in either branch (same with previous major versions at least back to 6.x and earlier). This is the first time since I've been involved in Drupal that we've not had that situation, hence revisiting things in this issue.
Similarly we used to treat regressions as critical by default, but have changed that definition some time during 8.x beta, since some regressions are very minor bugs in the scheme of things, compared to all the other bugs that aren't fixed yet (especially all the actual release blocking issues we had at that point).
For an example from this week:
#2442383: Add the option to cache drupal_get_filename() could be backported to 7.x (although it's not tagged as such). If it was fixed in 7.x first, then drupal_get_filename() wouldn't have that feature in 8.x, so it'd be a 'regression'.
However it is postponed on #2351919: Replace uses of drupal_get_path() with __DIR__ where possible which is 8.x only (PHP 5.4 requirement, etc.) and removes the vast majority of drupal_get_filename() usages. Then there are 2-3 other issues that are trying to get rid of even more drupal_get_filename()/drupal_get_path(). #2208429: Extension System, Part III: ExtensionList, ModuleExtensionList and ProfileExtensionList will deprecate it entirely in favour of an OO equivalent, and it'll be rarely used even if it exists in some form, so no need to add caching there.
So in that case, keeping the issue against 8.x just means an extra unresolved, unactionable issue in the 8.x queue - it's essentially duplicate/won't fix for 8.x.
It also means the issue could have to wait for a chain of 2-3 (or more) interrelated 8.x issues to get fixed before it's deemed completely irrelevant to 8.x and could be moved to 7.x.
The issue is more or less completely isolated from the 8.x issues related to it, so it's not going to benefit from any of the code in them, and all that we've done is delay it until that chain of issues is fixed, which could be a couple of months or a couple of years.
Then on top of that, the performance improvement from the caching is something that cannot be tested with our testing framework, and is something that 99.99% of even Drupal developers won't even notice, because the number of people profiling between major core versions is close to zero, and the 8.x usages are going away anyway.
So even if it got committed to 7.x next week, it's unlikely that anyone would realise it's been fixed there and not in 8.x unless they were the people actually working on that issue and saw the commit message.
Comment #34
David_Rothstein commentedI think the latter would be a better route for Drupal 7 also (and could be done in a PHP-5.2-compatible way with dirname(__FILE__)) although I'm not sure either is worth it since drupal_get_filename() is already heavily cached. But I'll comment on the issue :)
To your general point, though - sure, if there is an issue that is clearly beneficial and uncontroversial for Drupal 7, but of unclear benefit and controversial for Drupal 8, then it would not make sense to hold up the Drupal 7 patch on the Drupal 8 one. I just don't think those situations are very common.
My bigger concern is still this:
I can think of plenty of examples that fall into this category where having separate Drupal 7 and 8 issues being worked on at the same time (i.e. having both simultaneously open and un-postponed) would have been a bad idea that would have led to duplicate work.
Also don't forget that even if the code to fix a bug is different, if the issue is conceptually the same then the tests are likely to be (or at least should be) very similar, and that's part of the code too.
Comment #35
catchThat's no longer the case for lots of issues. Drupal 7 is 100% simpletest. Drupal 8 most new tests are phpunit. So the tests may be conceptually the same but could be the difference between hitting a different page in the browser vs. new data in a unit test.
In those cases it'd be completely fine to postpone the 7.x issue - but we still get the benefit of marking issues fixed when they're in a major branch, commit credit etc. Also if the 7.x issue is postpone, it can have a clear issue summary explaining why that's the case, which is harder to do on a single issue where you might already be trying to fit a lot of information into the issue summary and be on the 200th comment.
Comment #36
David_Rothstein commentedSo should we add something to the proposed resolution (in the issue summary) about making the default status of the 7.x issue "postponed" except in very rare cases?
One nice thing I just realized is that even if an issue is postponed, you might still be able to run the testbot on any patches that are posted in it (I don't think that was the case with the old testbot).
Comment #37
catchAdded that note to the issue summary.
Comment #38
catchHere's another example of where the current backport policy confuses people (and by extension causes noise in the 8.x critical queue which confuses me).
#2448843-44: [regression] Themes unable to implement hook_element_info_alter().
I think with defaulting to 7.x issues being postponed, we've addressed most of David's feedback now, so moving to RTBC.
Comment #39
David_Rothstein commentedSeems worth trying; I think there's a risk that people will leave comments or do work on the Drupal 7 issue (even if it's marked postponed) that would be relevant for the Drupal 8 issue also, but we won't know how much that happens unless we try it. (Most of the mitigation steps in the "Cons of this approach" section of the issue summary that are related to that are currently not implemented, though.)
We need to think about whether this should involve renaming the "needs backport to D7" issue tag (=> "needs D7 issue created"?) although perhaps it's simpler to just keep using the same one, since it's not technically wrong and it's documented in a number of places.
We'll also need to rewrite the instructions for assigning patch credit in commit messages for backported patches. The "Credit & committing" fieldset on drupal.org issues is no longer enough for this; the procedure will usually instead have to be "copy the names from the issue that had the original commit, and make sure they are all added to the backport commit also". That's annoying, but the upside at least is that there are fewer comments on the backport issue that need to be read through to look for new names that need to be credited.
Comment #40
catchIf the patch is committed to 8.x, you get credit for that.
If you help backport it to 7.x, you get credit for that.
I don't think the people who worked only on the 8.x issue need to get two sets of issue credit where currently they only get one.
Comment #41
David_Rothstein commentedWe can't give them issue credit currently; drupal.org won't allow that unless they happen to comment on the 7.x issue.
But I'm talking about commit credit, which I think is more important.
If person A wrote a patch for 8.x and person B backported it to 7.x, then in the typical case person A did most of the work, and in a very real sense the code that was written is "their code". So they should get their name mentioned in the commit message anywhere that code is committed. (Person B should get mentioned on the 7.x commit too, of course.)
Comment #42
catchAh OK. I think the old style 'suggested commit message' in the issue summary would work for that - can be based on the actual 8.x commit message.
Comment #43
colanComing from the other direction (as a Views D7 maintainer) where we have had patches in the queue since before D8 was released, I'm fine with marking issues Postponed until someone's determined that the issue is relevant or irrelevant to D8. So I'm thinking we can add the tag "needs D8 issue created" when postponing the issue.
If it is relevant to D8, the new issue can then be linked when it gets created (and the tag removed). If not, we can un-postpone it (and remove the tag). Does that sounds like a good idea?
Until now, we've been forward porting, which is less than optimal, but we really wanted the D7 patches to get in. It's been complicated by the fact that they're actually different projects (Views vs. Core) in different major Drupal versions (7 & 8).
Comment #44
catch@colan fwiw I think it's completely fine if you want to not have the Views issues postponed on core - that's entirely up to the Views maintainers as far as I'm concerned.
However it's really useful if 7.x Views issues that are relevant for 8.x make their way into the core queue and are linked between versions/projects, so anything we can do to make that easier would be great. And two separate issues is definitely easier for core vs. contrib in the same way it is for 7 vs. 8.
Comment #46
xjmThe core committers discussed this last month in April, and webchick indicated that her largest concern is addressed by postponing the D7 issue on the D8 issue where appropriate so the discussion does not fragment too much. Since this is primarily a release management decision, @catch and I agreed that we can go forward with this now.
I think the next steps are:
Anything else?
Comment #47
xjmAlso, some of the crediting limitations for backports are lessened if committers can add issue/commit credit (which we should treat as the same thing these days) without much overhead. So adding that to the related issues too.
Comment #48
catchI don't think issue credit on d.o is a huge issue - currently you get one issue credit regardless of how many branches an issue is fixed in, with separate 7.x issues you'd get two. For commit credit / git blame it's a bit different, but also depends if the patch is a real backport or not.
I updated https://www.drupal.org/node/2462101 which still thought 8.x wasn't released yet. It's not perfect but gets it closer.
Comment #49
catchNow that the documentation is updated I think we can mark this fixed - can always re-open or open a follow-up if there's more to discuss. I've also done a change notice with a short summary of the changes.
Applying it to individual issues is going to have to happen over time.
Comment #50
David_Rothstein commentedI think we still need to update the documentation for the two things in #39. I can have a crack at that.
No one said anything one way or the other about a better name than "needs backport to D7" so I'm going to assume we'll keep that tag, which sounds fine to me.
Comment #51
David_Rothstein commentedI made these edits:
https://www.drupal.org/node/2660808/revisions/view/9341802/9624103
https://www.drupal.org/node/767608/revisions/view/9619065/9624481
For the second one (to https://www.drupal.org/core/backport-policy) I changed it to recommend the backport be a "child issue" (rather than a "related issue") - it doesn't matter much but I figured it is easier to create since the Related Issues field is a pain to use directly.
I also added back a couple sentences with things that had been removed because it looked to me like they are still relevant.
Comment #52
jhodgdonJust a note: This needs an announcement on the Core group, Planet, etc. I only became aware of it just now because David pointed me to it on an issue (one that was a backport to 7 of another issue and was affected by this policy).
Comment #53
catchThose edits look fine, thanks David!
@jhodgdon I think we should do an announcement but it might be worth giving it a day or two to have examples to point to etc.
Comment #54
jhodgdonThe example where David pointed me here:
#2715113: Watchdog logging of all searches is performance hit; need ability to turn it off (7.x) (7.x backport of another issue)
Comment #55
David_Rothstein commented#2714515: References to CSS, JS, and similar files should be root-relative URLs: avoids mixed content warnings & fewer bytes to send is another example.