Lured in by the nice-looking standard template and very nice looking admin template, I recently installed OpenCart 1.4.something (I am unable to identify it exactly) and started implementing two new payment methods and some features required by 'upper managment'. Due to this, I came face to face with some of the code and I have a couple of comments.
Since I'm not using the most recent version, some comments might no longer apply.
Object-oriented design
Due to the way extensions are implemented (Most of them just get hardcoded into the OpenCart core classes), my installation is now completely un-upgradable. Most of this could have prevented by cleaner object oriented design. Some structure is there (MVC), but class overriding is not easily possible. Worse, some core classes are inexplicably declared final.
Furthermore there is no autoloading which makes easier overriding, well, easier. Have a look at the PHP documentation and study the code from the hierarchical MVC-pattern in Kohana V3. This would make OpenCart much, much more flexible for extensions.
And would not require the insane and error prone patching stuff that SMB does.
Payment models
All payment models have nearly identical code. That's not very OO and also makes custom payment modules prone to breaking with each upgrade.
Coding guidelines
Comment your code, please. Any amount of supplementary documentation is helpful at this point.
The trailing ?> are also not required and always the source for potential problems.
Better language file handling
I've seen that this actually improved in the current version: English is now always loaded as default before another language is loaded. Nice.
The creation of language files requires still a lot of Copy & Paste though and could benefit largely from using a centralized system like Gettext. It almost looks ready too.
What's completely ridicolous though, is the current way of passing language strings from the file to the view. What's more boring than hundreds of lines like this:
Code: Select all
$this->data['text_shipping_address'] = $this->language->get('text_shipping_address');
$this->data['text_shipping_method'] = $this->language->get('text_shipping_method');
$this->data['text_payment_address'] = $this->language->get('text_payment_address');
$this->data['text_payment_method'] = $this->language->get('text_payment_method');
$this->data['text_comment'] = $this->language->get('text_comment');
$this->data['text_change'] = $this->language->get('text_change');
$this->data['column_product'] = $this->language->get('column_product');
$this->data['column_model'] = $this->language->get('column_model');
$this->data['column_quantity'] = $this->language->get('column_quantity');
$this->data['column_price'] = $this->language->get('column_price');
$this->data['column_total'] = $this->language->get('column_total');
In system/library/language.php: Add the following line at the end of the load()-method:
Code: Select all
return $this->data;
Code: Select all
/**
* Load language file
*
* Returns all currently loaded strings
*
* @param string $language
* @return array $data
*/
public function language($language) {
return $this->language->load($language);
}
Code: Select all
$this->data = array_merge($this->data, $this->load->language('payment/' . $this->modulename));
I stumbled across some controller code in the payment modules (Search for hasPermission) where the payment module itself is checking the required authorisation. Huh? That's not a job of the module, that's a job for the framework.
Database design
If you're already HTML-encoding all special characters before passing them into the database, then please make the varchar-fields larger: An é becomes é (7 characters) and if you're using VARCHAR(32), well, the available place runs out pretty quickly. Plus the error message is non-sensical: It complains about a string which is cleary fitting into the constraints.
HTML and CSS
The default template looks very nice, but there are some weird chunks of Javascript lying around (Why submit forms by Javascript when a simple button would do too?). Plus a lot of styling tags in the HTML source where it doesn't belong. Additionaly, class names like div1, div2 etc. are rather confusing.
The CSS is clean, though, as far as I can see.
Template code
Templates are kinda weird: They are not really treated as templates. Either switch to a real templating system like Smarty or keep them as PHP files.
All config dialog templates are especially strange: When creating new forms, a lot of Copy & Paste is required. What about creating and populating the form fields by helper methods in libraries? (Similar to the way the https()-method creates correct links).
I wish all the developers good luck. But please read up on object oriented design, it will make your and our work much, much easier.