Post by Qphoria » Wed May 19, 2010 3:47 am

cstuder wrote: 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

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'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:First of all, since you're already using PHP in the templates, they could actually do this job for themselves. Otherwise, I suggest the following improvement: When loading a language file, return its contents directly. I.e.:In system/library/language.php: Add the following line at the end of the load()-method:Then in system/engine/loader.php: Modify the language()-method in the following way:Now, in your controller, you can pass all language strings to the view directly and save millions of keystrokes:

Security in custom modules

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.

There is one dynamically extensible area in OpenCart, and that is the Extensions system. This includes:- Modules (sideboxes)- Payment integrations- Shipping Integrations (rates)- Order Totals (discounts, fees, etc)- Product Feeds (rss, gbase, etc)These should never affect the core code. But you are correct that there are many other mods that do affect the core. This is an ongoing discussion, and while kohana has been brought up in the past in regards to this, after looking at it breifly, I fail to see how it will really solve our problems. While it might allow subclassing, it will still have issues with 2 mods changing the same function since subclassing only overrides whole functions and methods. But there may be more to it than what I've seen.I have thought about adding this and it was brought up early on by Daniel but he decided it against it. But I agree that the ability to dynamically load classes would make things easier.Desperation leads us the wrong way sometimesThere is a hell of a lot of identical code which is unfortunate. From what I've seen, it is a common draw back to MVC in general. The pro is that you have a more structured design, but the con is that it comes with multiple files. But I'm sure there are improvements that could be made.It seems more to me like leaving it out would be a more lazy approach. While not necessary, it has a value of marking the end of the file and not wondering if something got cut off. I suppose the potential problem you refer to is having extra space or carriage returns after the closing tag causing php warnings, which has crept up a few times as well.I am in 100% agreement with you here. I proposed a method of doing this to Daniel 2 weeks ago but he was unhappy with the way that it would mix the framework with the application. Perhaps your method is a better one by returning the language results to the controller. But I agree, this is extremely unnecessary and redundant. Loading of the language should automatically push it to the template for use.A fair point.Yea the error messages have historically been off.Must be greater than 1 and less than 255 doesn't cover 1 and 255. They should be changed to "between 1 and 255" or something of that nature. It's on the backburner for a change but not a main focus.Most of the forms are submitted by a link, which is done to allow nice button-style links with images, without using the an actual image with text. You will note that buttons are all selectable so that a simple language file can change them rather than having to have multiple button images for each language. So there are no simple buttons (Because they are ugly). Agreed tho that the inline css needs to be pushed to the stylesheet.Well as you pointed out before, php is embeddable. There is no need and has never been a need for a template engine. Smarty is just a waste of space and uses more characters than just php calls IMO. It's an answer to a problem nobody had but now thinks they had. I think the major bad design here is in the way that template files are really hard-linked to the controllers.A system like joomla allows you to take ANY html page and add the available tags to it, resulting in thousands of templates being made and promoting popularity. However, opencart uses a system that requires the template to be designed around the controller and framework, which results in only tens of templates being created, and the slightest change breaks the rest of it. This is especially true for repeating code in the modules for the main box structure. That should be pulled from a common place and only the specific code for the module itself should be added. The tpl structure is just extremely limited from the start and really needs a redesign but that won't happen for a while yet while features & stability are the main focus.All in all, while OpenCart does have some improvements that can be made, comparing it to some of the other popular options like ZenCart and Osc, it is miles ahead in 2 years than they are in 10. While it can be tedious and repetitive in places, it is extremely clean looking and something that is missing from almost every other cart I've tried, proper tabbing. OpenCart uses the basic 4 space tab for ALL php and 2 space tab for tpl files resulting in a much easier to read process than most other scripts I've worked with. That alone made me get involved enough with it to join the team and still Daniel has created an extremely impressive cart overall. Sometimes it just comes down to preference of design, and you can't always please everyone all of the time, but we are a progressive cart and your ideas and opinions will certainly be discussed and are appreciated.Thanks!