On Mon, Apr 7, 2008 at 4:41 PM, Chris Morgan chmorgan@gmail.com wrote:
Hey John.
Looks like you've gotten the nice urls working, great job man. It will be great to have this before 1.0 is shipped since it will make our urls a lot nicer looking.
I had a few comments about the code changes. You don't follow the variable naming convention of the appdb. We have some prefixes for variables that we use to keep the types of variables straight, you can find these in the coding standards file in the root of the appdb tree.
Right, oops.
The code in 0005-matchByVerbAndTerm.php-view-that-calls-into-the.patch.txt looks like it is using query_appdb() to perform a direct query. The indexing code also appears to do this. We prefer using the query wrapper function query_parameters() as this performs escaping of input parameters and helps to reduce the possibility of sql injection vulnerabilities.
Well I can certainly use that. For the params I do call appdb_escape_query => calls mysql_real_escape_string which I had thought was the "your injects stop here" escaping func =P
I noticed the code to perform the grouped inserts. What is the performance difference between perform the inserts individually vs. grouped together? If we run the indexer daily and its relatively quick we may be able to drop the complexity of worrying about how many inserts we can group together.
Well the runIndexer.php is just a one time thing to create the intial index. If it were run twice there would be doubles of every previous entry. Once we have it for the existing data any new data can be indexed when its state becomes 'accepted' or however the add new app flow goes. I have not implemented the "add index values on data creation" functionality yet though.
Regarding multiple inserts I think this is what was in my head when I wrote that:
http://dev.mysql.com/doc/refman/5.0/en/insert-speed.html Right where it reads "You can use the following methods to speed up inserts"
If the multiple inserts are a concern its easy to run them one at a time just set the config switch APPDB_MYSQL_MAX_INSERTS_PER_QUERY to 1 in the config.php file.
0005-matchByVerbAndTerm.php-view-that-calls-into-the.patch.txt appears to be generating output for display. It would be useful to have some comments at the head of the file to describe what the file is used for and why it would need to conditionally output lists when don't find a object page match.
Sure Ill write out the code paths in comments at the top, that file does do a bit.
Thanks for the feedback! =))
John