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