On Tuesday 20 June 2006 5:03 pm, Tony Lambregts wrote: > Chris Morgan wrote: > > On Tuesday 20 June 2006 4:12 pm, Tony Lambregts wrote: > >>Chris Morgan wrote: > >>>Make the code more consistent by using the appdb coding standards. > >>> > >>>Change $result = query_appdb(...) to $hResult = ... etc. > >>> > >>>Also fix some odd indenting due to spaces vs. tabs. > >>> > >>>There are still a ton of variables that are integers or strings that > >>>should be prefixed with 'i' or 's' if anyone is interested in cleaning > >>>some of that up. > >>> > >>>Chris > >> > >>I am not opposed to doing this but I think that the patch is way too big. > >>At the momment we still have issues with the last "big patch" that are > >> not cleared up [1]. On the whole unless we cannot avoid it I really do > >> not think that we should be making such large changes in one go like > >> this. Please break up the patch into smaller patches. That way it is > >> easier to find and fix any issues that arise. > >> > >>[1] New versions end up as orphans with current setup. > >> > >>-- > >> > >>Tony Lambregts > > > > Would per-directory patches be easier to read? Like one for /include, > > one for /admin, one for / ? > > > > This patch is a precursor to a patch that will close up a bunch of > > security holes that could result in the db being destroyed, then on to > > finding a more appropriate solution to the makeSafe() patch. > > > > Chris > > No That is not what I would prefer I would prefer a single patch for each > file that is changed (ie one for addcomment.php and one for > include/vendor.php. If the changes to one file need changes in other in > order for the appdb to continue to function then they should be together in > one patch. >
There is no way I'm going to send a patch in per-file. Its a huge pain in the ass and doesn't change how the monolithic patch is reviewed since you'll end up reviewing the same code anyway. Breaking the patch up given that its a atomic noop change is also not necessary. > Now you may think that is overboard but I have seen these "big patches" > break the AppDB too many times. > > As far as "security holes that could result in the db being destroyed" we > do have backup. I dont want you to think I do not care about security. That > is simply not true. I am however very much against breaking the AppDB for > regular use. The fact is that so far we have lost more data through bad > patches then through security holes. > I agree. I'll fix the existing issues before applying any more patches like this one. > We have preaty good security in place already, We check that id's are > numeric and escape date before running it. Right now if you ask me we would > be better off making a audit of our querys than what you are advocating. > Our security is awful. You should really re-read the email sent to the appdb list by Mortiz Naumann and see how bad it is. Chris