Strategies for far-reaching core patches

subscribe

@catch ... I think we first need to define "big" here...

50k? 100k? 500k?

10 lines? 100 lines? 1000 lines?

A patch that just "feels" big due to its complexity, etc?

..or just, this patch touches a LOT of core (ex. a 3 line patch that reactors the l() function)

bleen: it's not so much the size of the patch, as the number of other issues' patches that are in the review queue that would break, I think?

I think 30K + - is a big

Yeah, let's not base this on size. This is more about "far-reaching" patches. Adjusting title.

What we mean by that is patches that change something in one place that has ripple effects in lots of other places (or potentially every other place).

Here are some examples:

#22336: Move all core Drupal files under a /core folder to improve usability and upgrades
#225450: Database Layer: The Next Generation
#402896: Introduce DrupalCacheArray and use it for drupal_get_schema()
#81461: Clean up the cache API (stop overloading function arguments, remove procedural wrappers)
#1109202: [meta] Fix coding standards violations across Drupal core

IMO all of the proposed approaches are valid in their own way. I'm not really sure what resolution we need here beyond 'use these when needed'.

.

I agree with moshe, I think that depending on the patch any of the above approaches (or even some in combination) may be the preferred solution. It just depends.

We should also consider doing these major initiatives in a sandbox branch that merges into core, in addition to a patch workflow. We may not be doing it now but hopefully we will be some day. This can obviously allow a much more iterative workflow without a lot of the downsides above, assuming we can get the testbot into the sandboxes at some point.

sigh at double-posting

subscribing ...

My strategy so far in the Drupal 8 Multilingual Initiative is to try and avoid big patches as much as possible. As-in break them down to smaller ones. As the fate of many-many big patches shows, even if people agreed on some sprint or a sandbox or something, it will be debated and dissected in the issue queue anyway. That is my fear with the sandboxes for Drupal 8 that some of us are working with, and that their patches / git pulls would touch many many things at once. I know, I know small patches get comments like "this does not solve x, y and z, let's add those too", while big patches get comments like "this solves a, b and c in wrong ways, let's rewrite half of it". So both cases need "issue management" with an over-arching plan to show that changes will happen as needed later. In many cases, we lost belief that real follow-up work happens for stuff and its hard to predict the commit order for smaller changes, so our workflow moved to bigger patches in many cases.

.

Where possible I would favor BC layers marked @deprecated. Smaller patches are easier to digest. It also results in lots of on-ramp tasks for new contributors, something we lost with the testbot. The follow ups do not need to be critical, but the final bc removal issue should be major, probably.

subscribing ...

I would also note that "if it breaks stuff, roll it back" is much easier to do if we do it using Git. Git should make it easier to "undo" a commit if you don't go through patches. That is, if we decide to back out a commit it's something that Drieschick can/should do directly, NOT wait for someone to roll an "undo" patch.

thx for this issue. subscribe. sorry, nothing to add at this time.

Closed as duplicate: #1294298: Patch size and complexity thresholds should be discussed, decided, and documented.

Copied "Proposed resolution" and "Remaining tasks" from duplicate issue #1294298.

I propose that all pre-scheduled commits of "far-reaching core patches" be published as events on the Drupal core and/or Issue Triage group calendars.

I further propose the following thresholds for "far-reaching", any one of which should be sufficient by itself:

• Touches at least half the lines of a required core file.
• Changes the signature of a function called from at least half of all required core files.
• Changes an API used by at least half of all required core files.
• Provably conflicts with at least half of all currently RTBC core patches.

Note: By "required core files" I mean files that are loaded by a full bootstrap after installing the Standard profile.

Delayed for review sometime after November 1: #1290258: The data type should be explicitly stated for all Drupal core @param and @return documentation.

+1 on a narrow window for mega-patches rather than specific days, and +1 on increasing RTBC throughput. :-)

Also, for some of these epic patches couldn't they be broken up or done piecemeal over time? Eg, removing the cache BC layer can be done after everything else is converted over to the new API model, and that can be done bit by bit. That's a lot of potential Novice patches for newbies, or just boring stuff for when you need a break from hard patches. Ibid for changing how we document hooks.

That would let us potentially dovetail a lot more patches, as long as we can keep the throughput up, and give more on-ramp opportunities for new contributors.

Eh, if it's exactly the same change in 10,000 files, I'd prefer a mega-patch to 10,000 patches, personally. Each patch has to be reviewed, then has to be RTBCed, then has to be committed. Even if these commits are quick, it vastly increases the size of the RTBC queue, ensuring more important patches are missed, and the overhead of doing that in 10,000 issues wastes a lot of valuable time.

An example of where a mega-patch might not be preferred, even though it's the same type of change in all files, is something like #711918: Documentation standard for @param and @return data types. Because each of those individual changes needs to be carefully reviewed as to whether it's correct or not. So in that case, I'd do it on something like one patch per module (and then maybe another one for includes/*).

However, things like these:
* #1295456: Add missing '$' character in docblock for DrupalWebTestCase::drupalGetNodeByTitle() function.
* #1295460: Add missing '$' character to docblock for taxonomy_get_term_by_name() function.
* #1295462: Add missing '$' character to docblock for _theme_load_registry() function.

... are really easy to glance at quickly to make sure the change is consistent, so there's no reason to split them apart into multiple patches.

@webchick:
FWIW, I actually *did* find and fix those separately, although in quick succession. I suppose when I found the second one I could have appended to the first instead of opening a new issue, but the new issue seemed more likely to get reviewed/accepted, and saved me the difficulty of having to compose a title that adequately described the expanded scope. Note that the following would not fit into a single title line:
Add missing '$' character in docblock for DrupalWebTestCase::drupalGetNodeByTitle(), taxonomy_get_term_by_name(), and _theme_load_registry() functions.
And the last time I submitted an issue that included "... in various functions" you shot it down as having too vague a scope statement.
EDIT: retracted the last; can't find the issue right now; it probably wasn't webchick and may not even have been Drupal core.

pillarsdotnet: You seem unnecessarily stuck on discerning a Single And True Golden Rule Of Unbreakableness that can apply to any and all situations. I just don't think that's possible. The entire crux of this discussion is that big changes are far more nuanced than that, and some classes of changes call for different classes of strategies.

I have no absolutely no idea what the context was of me "shooting you down" in an issue with the title including the words "in various functions." That doesn't sound like me, but hey, maybe I was having a bad day. But in this case, I would've just used the title "Add missing '$' character in docblock variables." or similar, and rolled a single patch that made the correction across core.

a Single And True Golden Rule Of Unbreakableness

Ah, the Holy Grail of computer programming...

Maybe a guideline though is "If it can be scripted, do it in a single patch. If it can't, it must need more careful review, so split it up by module/include file."

Seems like it should be possible to search for and identify all instances of "@param something_without_a_$_before_it\n" in core. Just as it should be possible to script a mass-change of Implements to @implements. Breaking those types of changes up into smaller patches simply adds more work to everyone's plates, and wastes valuable energy saying "Yes this is also a find/replace. RTBC." multiple times.

Adding datatypes to params/returns, OTOH, requires a human to actually go through and check the values and document them, and then another human to verify that they are correct, and also check for typos and the like. Multiple, per-module/file patches make a lot of sense for that. (The patches I've seen lately have been per-function though, which is way, way too granular IMO.)

In terms of patches that introduce new APIs, I'd still prefer to stick to "once the API's in, all new code must conform to it regardless of the presence of the BC layer" mostly because then we eventually whittle the places the BC layer is used down to zero. It's easy to envision the DBTNG conversion taking at least 1/3 longer if we hadn't stuck to that rule. However, I don't feel super adamant about it if you think allowing old uses of the APIs is super valuable.

Seems like it should be possible to search for and identify all instances of "@param something_without_a_$_before_it\n" in core.

Exactly what I was doing -- only I was looking for type-hinting, not missing dollar-signs. Only found the latter when I did a "sort -u" on the results and said, "Wait a minute -- that's not a legal datatype..."

Found 'em one at a time 'cuz I was sure each time that it *must* be the only one.

(The patches I've seen lately have been per-function though, which is way, way too granular IMO.)

See #1295500-7: Add missing '$' character to docblock for install_profile_info() function.

Can you PLEASE limit your changes to the doc of the function mentioned in the issue statement? Cleaning up a whole file like this , AS YOU WELL KNOW pillarsdotnet, is not OK at this time.

Things like that are why I seek a Single And True Golden Rule Of Unbreakableness.

Y'all argue amongst yourselves; lemme know the conclusion. Preferably in writing. On a docs page.

I agree with the heuristics proposed in both #27 and #31. I'd probably be a little more lenient than webchick on existing patches. If a patch is already in the queue when the API change happens, and it's not doing anything to that particular API, then I'd be inclined to not force a reroll just for that. If it's changing that logic, though, it should use the new API.

Eg, a patch that was already in the queue and moved an insert statement isn't really touching the query, just wrapping it in an if statement or something, so no sense beating up on that poor patch writer for it. If the patch is changing the query though, then it may as well get rewritten to the new API while we're at it since we're messing with that query anyway.

A bit of a gray area, I admit, but I don't know that we should force a patch to also be an API update just because it happens to be close to a routine that changed, only if it's actually changing the code in question.

Yeah, that seems reasonable. So if the +/- in the patch is identical, just in a different location, you're fine. But if the +/- line changes, you're on the hook for implementing the new API.

Yeah?

Sounds like we're all on the same page.

Regarding the "single golden rule" for when patches are too big/too small, I wrote up this:
http://drupal.org/node/1295500#comment-5068622
and filed a new issue so it could get put into the doc:
#1298194: Standard needed for what is a "disruptive" patch

Please comment there and I'll write up the page sometime soon (but not today, too much going on...).

Could we not just have that discussion here, since it's quite related, and we already have a few people participating?

Sure. I'd like to keep the other issue there though, since I need to remember to make the page. :) Added a comment there to discuss here.

And so as to avoid extra clicking, here's the text of the comments that I'm recommending as a starting point to the "golden rule" page discussion:

a) Patches that do small standards updates to a whole file or to most of a file would tend to make it so that other patches in the queue cannot be applied, if they are not committed. So they can only be committed when the committers are receptive to such patches, such as the first week in November when every D8 patch in the queue is going to break anyway because the core files are being moved into a 'core' directory.

b) Patches that fix the same real issue in about 10 functions are not as disruptive as, and are easier to review and commit than, 10 smaller patches fixing the same real issue in the 10 functions separately.

c) When you are fixing up a docblock for one function, any patches around that area in the file would already be disrupted. So if there is one doc problem in a function doc, it's not more disruptive to, at the same time, fix up the rest of the doc problems in that one function at the same time, and it also doesn't make the patch much more time consuming to review or commit. So I advocate fixing up each function to comply with our current doc standards whenever a doc problem is being fixed for that function.

Also somewhat related: #1298304: [Meta] Move all externally-developed files to core/vendor/{vendorname}/ directories
Such files should also be excluded from automatically-generated clean-up patches.

On #40.a), the only time I can see stalling/scheduling a patch is under the following conditions:

a) It's an automated change that can be scripted, which means it will impact the entirety of core files (since we don't want automated patches split up file-per-file since that's annoying to review). Also means it's easy to re-roll. (Or at least easier than patches that can't be scripted.)

and

b) The ratio of things changed in the patch is sufficient to break a significant number of patches in the queue. We could try and codify this somehow (> 100K patch? > 50 files touched?), but basically #22336: Move all core Drupal files under a /core folder to improve usability and upgrades and #1109202: [meta] Fix coding standards violations across Drupal core would qualify, "Fix variables without $ in front of them" would not.

If it is not automatable, that means we need to split it up per-module anyway, because it requires human creation/review. And those are far less likely to break everything in the world so could just be committed as they're ready (assuming there aren't more important fixes vying for core maintainer attention in the RTBC queue and we're below issue thresholds).

RE #42 - In the past, you haven't been receptive to other far-reaching docs patches, such as a human-generated per-module effort to bring docs headers up to standards (e.g., first lines 80 character full sentence with right verb followed by blank line, space between param and return). At least during the d7 cycle, those issues were postponed by you (webchick) as being too disruptive, and I thought I was being told that the post-"move everything to core directory" was a good time to do those kind of human-done wide-impact patches?

i.e. (b) is satisfied (lots would break) but not (a) (automated). So we split it up into modules, but it wasn't acceptable to do then anyway.

So maybe this does need some further clarification? Otherwise, I'll schedule the "fix up doc headers" sprint for some other time that is more convenient for me...

I remember having a discussion about that at DrupalCon, but I don't remember that being any kind of formal mandate. That was also when the RTBC queue was about 100 issues higher than it is right now, and we were still fire-fighting critical D7 bugs.

I think the key there is "more important fixes vying for core maintainer attention" which is hard to codify.

Ah, good point. That was late-ish in the D7 cycle and things were insane.

So how about this as an outline:

a) Patches that do doc/coding standards updates to a whole file or to most of a file would tend to make it so that other patches in the queue cannot be applied, if they are not committed. So they can only be committed when the committers are receptive to such patches.

The committers are less likely to be receptive to disruptive patches when:
- It is late in the development cycle for a particular Drupal release.
- There are a lot of RTBC patches in the queue that would need to be rerolled.
- A large and complex patch is close to RTBC and would need to be rerolled.

They are more likely to be receptive to disruptive patches when:
- It is earlier in the development cycle for a particular Drupal release.
- The changes can be automated by a script (in which case, the patch can be generated for all of Drupal in one shot, and regenerated at any time by the script and committed when convenient).
- If they can't be automated, the changes are broken up by module, so they can be reviewed by patch reviewers and committers in smaller chunks.
- The changes are submitted just after another disruptive patch that will require rerolling of a lot of patches anyway.

b) Patches that fix the same small issue in about 10 functions are not as disruptive as, and are easier to review and commit than, 10 smaller patches fixing the same issue in the 10 functions separately. [EDIT: added the word "small" here]

c) When you are fixing up a docblock for one function, any patches around that area in the file would already be disrupted. So if there is one doc problem in a function doc, it doesn't add any additional disruption (or much review time/difficulty) if the patch also, at the same time, fixes up the rest of the doc problems in that one function. So, the standard practice is that when you are fixing a doc problem in one docblock, you should also bring the docblock up to the current doc standards in the same patch (e.g., make sure the first line is a sentence with the right verb in 80 characters, there is a blank line between param/return, lists are formatted correctly, the grammar/spelling is correct, params are all there, and the doc is generally clear).

It helps me to understand better with an example. For instance, it appears that one of the things going into D8 core is type hinting for @param and @return doc strings. Under the proposals above, how would one like to manage simply this type of docblock change for files like common.inc or system.module?

All at once changes to such files will require significant manual work to create and review over some period. Meanwhile, other patches will be both prepared and committed that touch these files. Should this type of patch be applied all at once for these file? Or say 10-15 docblock changes at a time?

I ask this because I am a big fan of type hinting and realize it will require significant work. However, I also believe it will help the Drupal community much going forward.

As outlined above, I feel these should be done one patch per-module, one patch for /themes, a one patch for /includes.

Rationale:

Each one of these issues needs to go through phases of development, then review, then revision, then review again, then promotion to a core maintainer, then possibly more revision and more review again, and then finally commit. (As long as issue thresholds are met, and as long as there aren't more pressing things in the RTBC queue which would be invalidated by these changes.)

Clearly, doing this in one patch for all of core is way, way too much (unless it can be scripted, which this one can't). It would break too often and fall out of date too quickly, unless we just took an entire week off core development and said that's the only thing we're getting done.

One patch per-function, likewise, is obviously way, way too granular. Drupal 8 has 4,000+ functions which is 4,000 issues that need to go through that workflow, and clutter up the RTBC queue from more pressing fixes.

One patch per-file might seem like a good breakdown, but there are still more than 400 files, which is more than 400 issues that need to be separately reviewed and committed. And worse, they do not break down under logical lines. If I know the Shortcut module really well, I need to track 4-5 separate issues to keep on top of.

If we kept it to one patch per-module, that'd break it up into 40 logical chunks so that people who know, say, Shortcut module could review all of the changes to it at once instead of over 4-5 issues. Plus another for themes (+1) plus another for /includes (+1) means fewer than 50 issues to track for such an initiative, which seems a lot more doable to me.

I asked the above question because I thought it would be helpful to start with a smaller 'test' issue to gain experience with. Since many other patches seem to touch common.inc and/or system.module, I thought we might start with just those files and see what the experience and bike shedding is like in moving to final commitment. Does this make sense?

Yes. So I would start with something like Poll or Shortcut module, which is both user-facing and much easier to wrap your head around, and also unlikely to be conflicting with other patches.

Note that #711918 is still undecided. We lack sufficient buy-in from core developers, according to this comment.

So just to clarify ... I had been planning to set up an API docs sprint in early November (after the "move to core" patch lands) to clean up some docs standards core-wide. Are you saying we don't need to wait for then to do this work, but could do it now (and potentially for d7/8 both)?

This is not the "add type hinting to docs" cleanup, but a less ambitious (and less difficult to review) sprint to get first line descriptions up to standards (verbs, 80 characters), and blank lines between params and returns, and possibly other cleanup stuff (indentation in params and stuff like that that may be lingering).

On the "one per module, one for themes, one for includes" idea... For modules and themes, that is probably fine. For includes... We have about 58,000 lines of code in about 90 include files. So the "all of includes" issue and patch would be huge. But I guess that's only a factor of two or so bigger than modules/system (22K lines, 32 files). Themes have a lot of files, but not that many lines of code... So, OK. Or we could perhaps decide to do "includes A-L", "includes M-Z", "includes subdirectories database and filetransfer", and break it up into three slightly more manageable chunks?

Updated "Remaining Tasks" per IRC discussion.

Did any of those remaining tasks happen? Are any no longer relevant?

Adding parent to help find related work.