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