[development] Splitting up patches
drupal-devel at webchick.net
Fri Aug 25 02:47:12 UTC 2006
Somehow this turned into quite a long mail. ;) If you're not
interested in the blah blah, please proceed to the "---" thing at the
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/
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.)
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. ;)
More information about the development