MrTech wrote:Don't hold your breath then, my understanding is this is a real tight group and they don't look kindly to strangers trying to tell them the way they operate is less than desirable. Bad code, not standards, non-existent and out of date documentation...
Well the code is a little messy and inefficient in places, but that's understandable given the complexity of the queries. The OpenCart GetProducts() query could be cleaned up, but it's still dramatically better than the equivalent in VirtueMart (shop_browse_queries.php for anyone who's familiar with it). Now THAT'S bad code, several hundred lines of code just to generate a single SQL statement. We've been working with VirtueMart for a few years now, and compared to that OpenCart is an absolute dream. My web designer colleagues are able to install, configure, skin and modify an OpenCart store with almost no help from me (and even then my help is only needed when adding a custom vQMod or new payment method). The Extensions community is far better than anything VirtueMart has, at least everything is version tagged (so you're not scrolling through an endless forum topic to find out if an update was released).
Documentation is something that can be very hard to keep up to date when you're busy developing software, adding new features, fixing bugs etc. This is a problem even with much bigger software, IP.Board being a good example (commercial company with a good size staff, great at fixing bugs and adding functionality but the documentation always lags behind).
rph wrote:Development in OpenCart is pretty closed (which has its bad points and good points) but you can always lobby for specific changes.
I'm happy to post suggestions, chunks of code etc to the forums. I just worry about submitting something and then it not being seen until the code has already been changed by the developers. I'm not asking for direct SVN access, but a way to make sure I can help these guys out would be great. Even if they want to "outsource" some work to me (they write the functionality & rough code, I go through and tidy it up for them...the boring stuff
)
rph wrote:Your tertiary assignment example seems more a matter of style than anything. I know they're already used in array value assignments which is a good fit but that's about it.
That's exactly right, and yes it's definitely a personal choice of individual developers. But I noticed both styles being used in OpenCart, and I feel like it would make vQMods much easier to work with if the code was condensed.
The best example I can think of is the SQL query in /catalog/models/catalog/product.php, inside the GetProduct() function. It's one LOOONG SQL query! When you want to modify the code in vQMod to add a single new field, it's a real pain to see exactly what this query is doing (especially because there are so many subqueries to pull out reward points, discount prices etc).
Code: Select all
$query = $this->db->query("SELECT DISTINCT *, pd.name AS name, p.image, m.name AS manufacturer, (SELECT price FROM " . DB_PREFIX . "product_discount pd2 WHERE pd2.product_id = p.product_id AND pd2.customer_group_id = '" . (int)$customer_group_id . "' AND pd2.quantity = '1' AND ((pd2.date_start = '0000-00-00' OR pd2.date_start < NOW()) AND (pd2.date_end = '0000-00-00' OR pd2.date_end > NOW())) ORDER BY pd2.priority ASC, pd2.price ASC LIMIT 1) AS discount, (SELECT price FROM " . DB_PREFIX . "product_special ps WHERE ps.product_id = p.product_id AND ps.customer_group_id = '" . (int)$customer_group_id . "' AND ((ps.date_start = '0000-00-00' OR ps.date_start < NOW()) AND (ps.date_end = '0000-00-00' OR ps.date_end > NOW())) ORDER BY ps.priority ASC, ps.price ASC LIMIT 1) AS special, (SELECT points FROM " . DB_PREFIX . "product_reward pr WHERE pr.product_id = p.product_id AND customer_group_id = '" . (int)$customer_group_id . "') AS reward, (SELECT ss.name FROM " . DB_PREFIX . "stock_status ss WHERE ss.stock_status_id = p.stock_status_id AND ss.language_id = '" . (int)$this->config->get('config_language_id') . "') AS stock_status, (SELECT wcd.unit FROM " . DB_PREFIX . "weight_class_description wcd WHERE p.weight_class_id = wcd.weight_class_id AND wcd.language_id = '" . (int)$this->config->get('config_language_id') . "') AS weight_class, (SELECT lcd.unit FROM " . DB_PREFIX . "length_class_description lcd WHERE p.length_class_id = lcd.length_class_id AND lcd.language_id = '" . (int)$this->config->get('config_language_id') . "') AS length_class, (SELECT AVG(rating) AS total FROM " . DB_PREFIX . "review r1 WHERE r1.product_id = p.product_id AND r1.status = '1' GROUP BY r1.product_id) AS rating, (SELECT COUNT(*) AS total FROM " . DB_PREFIX . "review r2 WHERE r2.product_id = p.product_id AND r2.status = '1' GROUP BY r2.product_id) AS reviews, p.sort_order FROM " . DB_PREFIX . "product p LEFT JOIN " . DB_PREFIX . "product_description pd ON (p.product_id = pd.product_id) LEFT JOIN " . DB_PREFIX . "product_to_store p2s ON (p.product_id = p2s.product_id) LEFT JOIN " . DB_PREFIX . "manufacturer m ON (p.manufacturer_id = m.manufacturer_id) WHERE p.product_id = '" . (int)$product_id . "' AND pd.language_id = '" . (int)$this->config->get('config_language_id') . "' AND p.status = '1' AND p.date_available <= NOW() AND p2s.store_id = '" . (int)$this->config->get('config_store_id') . "'");
If you open the file, you'll notice it's all on a single 2408 character line! Makes me want to cry
rph wrote:One thing I would disagree with you on is moving input sanitation outside SQL queries. Having it in the queries keeps you from having to search all over the place to make sure everything is cleaned up.
I guess this is the Joomla developers' preferences rubbing off on me. Instead of calling values directly from $_REQUEST, $_POST, $_GET; they use custom functions to sanitize the values before being used.
For example:
Code: Select all
$product_id = JRequest::getInt('pid',false);
if(!$product_id) {
return false;
}
$sql = "SELECT * FROM product WHERE product_id = ".$product_id." LIMIT 1";
(I'm writing this from memory, I may have it wrong).
So it's basically saying that $product_id must be an integer, the getInt() function cleans up the variable, and if it's not set it returns false (or a default value of your choice).
CodeIgniter does something similar, to prevent XSS attacks. Of course if the SQL is laid out nicely in a readable fashion then this isn't as important, it's just when you've got:
Code: Select all
...WHERE lcd.language_id = '" . (int)$this->config->get('config_language_id') . "' AND ...
that it can be hard to follow. And in the above getProduct() query, config_language_id is used several times in the same query, so if we're typecasting it as an integer it would make sense (at least to me) to assign it to a variable before using it in 3 different subqueries:
Code: Select all
$language_id = (int)$this->config->get('config_language_id');
$sql = "...WHERE lcd.language_id = ".$language_id." AND ...";
There's even a few variables in the product search query which are passed through 2-3 different inline functions in the query:
Code: Select all
$sql = "...WHERE product_name LIKE '%".dbclean(escape(utf8_clean($keyword)))."%' ";
(Again, typing this one from memory, the function names aren't real but you get the idea).
rph wrote:Definitely agree with standardization on the SQL statements (backtick use seems to be random outside required areas).
I'm not sure which way I lean with MySQL backticks. I initially only used them for escaping, then I used them all the time in every single query...but now I'm thinking I was right the first time and the SQL is easier to read without them. I think the developers should pick one style and then I can clean up all the code to fit their chosen standard
rph wrote:As much as I'd love to not have to type DB_PREFIX all the time I'm not sure how you would move it without making things more complicated and potentially difficult for developers who add their own tables.
How about the Joomla method? For security reasons they use random database prefixes (it used to be jos_tablename but now it's a random string).
When you write a Joomla SQL query, it looks like this:
Code: Select all
$sql = "SELECT * FROM #__users WHERE id = ".$id." LIMIT 1";
The Database class automatically replaces the #__ with the correct prefix in the configuration file ("jos_users" in older versions, "kjov3_users" in newer versions using random prefixes).
VirtueMart had its own custom prefix, so queries would look like this:
Code: Select all
$sql = "SELECT * FROM #__{vm}_products WHERE product_id = ".$pid." LIMIT 1";
It would be converted automatically to:
Code: Select all
$sql = "SELECT * FROM jos_vm_products WHERE product_id = ".$pid." LIMIT 1";
It definitely shortens the length and complexity of the queries, but I'm open to suggestions too.
I'm making this offer to try to make everyone's lives easier, including that of the main developers. They've done an absolutely brilliant job, I'd like to help ease the strain a bit for them so they can continue working on the "fun stuff" rather than the tedious boring aspects of development