[development] Splitting up patches

[Angela Byron]

Somehow this turned into quite a long mail. ;) If you're not  
interested in the blah blah, please proceed to the "---" thing at the  
bottom.

I think all the points you make are sound. Large patches are indeed  
more difficult to review, reviews take longer because of their  
potential for breaking stuff, and since so much is going on, there  
tends to be a lot of back-and-forth on the issue queue, which make  
reviews even more difficult.

Arguments against it would be:

1. Every patch, even something seemingly simple and no-brainerish, is  
a possible victim for "analysis paralysis" which will hold up  
dependent patches and dependent functionality.

For example, say merlinofchaos splits off the .meta files part of his  
new module administration patch page feature (http://drupal.org/node/ 
76340), which one could conceivably say is a separate chunk of  
functionality easily submitted as a patch on its own. The whole issue  
could stagnate just because people can't agree on whether .meta files  
should instead become .install files or whether they should be  
written as .ini files or XML files or PHP files or... In the  
meantime, we miss this great piece of functionality that will bring  
Drupal administration to the next level because of these kinds of  
details which can always be changed later.

2. What to do about dependencies?

In my fix signatures patch (http://drupal.org/node/10938), for  
example, the crux of the bug is just that forums should get  
signatures like their replies do. This is a one-liner fix. It  
cascades from there though, that comment's implementation of  
signatures is fundamentally buggy and if we copy that same behaviour  
to node module, now we've made two messes.

Theoretically, each of the essentially 4 bugs that patch fixes could  
probably be moved into separate patches/issues. However, they're all  
inter-dependent. If nodes get comments too, then comment logic should  
be moved to user module, which is where the signature field come  
from. If we move signature appending to the form creation process to  
get around the current horrid "signature sitting in the add form"  
behaviour, it only follows that it should be in a themeable function.

So then you get into rolling 4 different patches and either putting  
stuff like, "In order to test this patch, make sure you have applied  
#00000," or writing "brain-dead" patches that aren't at all what you  
have in mind for the final thing, just so they'll apply, and having  
to re-roll at least one of them when/if the other gets committed,  
etc. This also seems like it adds extra headaches all around, for the  
patch creator, the patch reviewers, AND the patch committer.

3. The code freeze draws very near.

Patches that started back in July or so were much easier to break up  
into smaller chunks. You could have that 3-4 days of "review, needs  
work, review, needs work, review, RTBC" for every inter-dependent  
part of your patch and not have to worry that the "master plan"  
wouldn't get in. However, there have been some major API changes in  
just recently which have opened entirely new realms of possibilities  
for some patches, and going through that process on 18 smaller  
patches is a very dangerous thing for the prospective feature-adder,  
yet a lot of the functionality we've added will be only 1/2 or 3/4  
implemented without these. I'm thinking particularly of patches like  
drupal_render() for profile display: http://drupal.org/node/79227 and  
Install-time and run-time requirements checking: http://drupal.org/ 
node/75002

---

I think a sensible rule of thumb would be "split up patches where you  
can," for the reasons you specify. But there are some places where it  
either just doesn't make sense, or worse makes things more difficult,  
and it would be great if core committers could keep that in mind when  
making these requests.

I think another thing that would help *a lot* is to be given some  
more kind of concrete guidelines about what exactly CAN get in after  
the code freeze. It seems logical that relatively "minor" stuff like  
improved wording changes, some spiffy graphics to spruce things up,  
and so forth would be ok. "Usability" is a broad term, though:

* Would something like http://drupal.org/node/80574 which would  
provide $form_values['preview'] and $form_values['submit'] be  
considered "usability"? (it certainly makes life easier for developers.)
* Would Dries's patch for adding menu items to view 404s/403s: http:// 
drupal.org/node/79622 be considered "usability"? (It certainly makes  
life easier for site admins.)
* Would a new default core theme be considered "usability?" (It  
certainly makes life easier for users, and for site designers who  
need a good Drupal CSS theme to start from.)
etc.

Then I think we would be able to draw better conclusions on where to  
focus our reviewing efforts, and we'd feel more at ease splitting up  
major patches, because we'd all be kind of on the same page as to  
what needed to get done by the 1st.

If you got through all that, congrats and thanks for reading. ;)
-Angie