Quoting Thomas Zahreddin <tz@it-arts.org>:
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.
If it is by design then the code should be documented as to why. If no comments then we can assume that the patch is either fixing a bug or create a new feature. If this is a new feature and the issue is marked as a bug, change the information to feature request from bug report. Feature requests always go to the next release so be sure to change the version and ask the patch be rolled for HEAD as well as give information about the patch itself. Bug reports usually go to HEAD, you may find an instance of an API call that has been removed so HEAD can't contain the patch. Once applied to HEAD then the patch can roll backward to other versions.
(caution: Ironical!) Oh, I can't count how often i heard: "the code tells it all!" or "read my code first" or "only code counts, not the words about it" ..
Don't forget about common sense.
Yes I can read the code but the question above can not be answered by reading the code - much more helpful would be a comment what the _aim_ of some lines of code is.
But it can be answered by looking at the source. If no comment is present then there is no reason to think that anything special was being done by the previous code.
And maybe the group working on and with that particular module has a list of decisions (not a mailinglist) and why they where made - so one could go back to the original decision (should be referenced in the code) and look up what the original intention was.
Again, common sense. If decisions aren't documented in an issue somewhere based on the policies already set forth for Drupal core then a patch isn't really warranted or the decisions have to be stated again in the issue. The issue is the communication point for the patch. And as a matter of policy Drupal core patches are never committed without an issue.
Do you agree that these information could help a lot in similar situaltions?
Not always.
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 ?
IMHO persons, interested in a module (like programmers and users of this or a compatible module), shall have the possiblity to be heard with their requirements. It is up to the maintainer (or moderater) to listen to all and their arguments what is urgent and /or important and to come up with suggestions to which most (if not all) people can agree. The final decision is up to the maintainer (keeping in mind the long term goals, all persons interested in the module and the available resources) in cooperation with the person willing to contribute (e.g. code).
It is also up to the interested persons to provide patches for the modules. Everyone can be a patch creator.
If the group of persons working on a module keeps a logbook about their decisions, then everybody is able to follow the decisions regarding a modul.
Contrib modules are of a different breed of issue. There is no enforcement of Drupal policy on the contrib module.
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 ? Or is there another process to follow in such situations ?
Leave it as needs review but leave your comments.
I agree absoluty: We do not have approriate structures in the drupal community to work efficently, make decissions traceable, keeping the teamspirit up and stay open for new developers to join.
I highly disagree. Yes, people will get their feelings hurt as we paint the bike sheds our favorite color but we need to understand that in a community that is supporting an open source package that not everything will be accepted. New developers can join by being participating as well as learning. Test out the new patches and give review.
So I suggest to read a little about Sociocracy and start discussing our decision making process.
The decisions have been made. Now it is time to review, test, patch again, re-review and retest. Find an interesting patch to review. Be notified of a patch waiting for review by subscribing to the review tag at r-feed.com. -- Earnie http://r-feed.com Make a Drupal difference and review core patches.