[development] Reviewing patches and making decisions

[Nathaniel Catchpole]

On Tue, Nov 4, 2008 at 7:30 PM, Anselm  wrote:

> Hello,
>
> Having followed this thread for a while, I decided to do my bit, and start
> reviewing patches. I didn't get very far ; I stopped at the first patch.
>
> Not because of the code - I could understand what the original code did,
> and
> what the patched code did - but because there are two decisions to be made
> :
>
> 1. In that case - and I guess this is not uncomon - the old behaviour might
> be
> a bug, but it might have been by design. I have no way of telling.


A lot of core patches are trying to improve the API (or some other
improvement) rather than fix a bug per se.


>
>
> 2. Changing the old behaviour to the new one could break old modules that
> implicitely relied on the old behaviour. Again, this is probably not
> uncomon.
> How do I weight the importance of the patch vs stability of old modules ?


In general we don't maintain backwards compatibility of code between major
releases (although neither do we break it just for fun) - every release has
detailed upgrade instructions for module maintainers as
http://drupal.org/update/modules

So changing function names, function parameters - this is all fair game as
long as there's a tangible improvement to doing so.





> So what do I do next ? I don't have a sufficient overview of that part of
> Drupal to make such decisions myself. Should I mark it as reviewed, and add
> as a note what the potential implications are ?


Making a note of what the potential implications are is sensible where
you're not sure where something should go - this will help the next person
looking at the issue to make sense of what specific questions need to be
answered in relation to the patch.

The other thing you can do is join #drupal on irc and try to find someone
more familiar with that part of Drupal - if you say 'I'm reviewing a patch
about x but I have some questions' you'll usually get help.

Nat
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.drupal.org/pipermail/development/attachments/20081104/d23f141d/attachment-0003.htm