Community Forums

General feedback and suggenstions about OpenCart

Coding & concept discussion for OpenCart v1.x development

General feedback and suggenstions about OpenCart

Postby cstuder » Tue May 18, 2010 5:50 am

Hello,

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');


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:
Code: Select all
return $this->data;


Then in system/engine/loader.php: Modify the language()-method in the following way:
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);
   }


Now, in your controller, you can pass all language strings to the view directly and save millions of keystrokes:
Code: Select all
$this->data = array_merge($this->data, $this->load->language('payment/' . $this->modulename));


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.

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.
cstuder
 
Posts: 6
Joined: Fri Apr 30, 2010 9:14 am
Location: Switzerland

Re: General feedback and suggenstions about OpenCart

Postby Daniel » Tue May 18, 2010 11:41 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.


Well genius you obviously don't have a clue about OOP programming! OpenCart is probably the most cleanest framework out there! Have you ever tried Zend Framework?

I have built many different types of applications using OpenCart. Its that flexibile you can build any PHP application with it!

Don't blame OC becuase you are not the programmer you think you are!

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


Kohana is not a true OOP framework. you should be able to visually design any OOP application before you even start coding. If you ewent to a good college and where taught proper OOP you would know this. This is what OOP was designed for back in the 60's! They created the applications on blackboards before touching any code. There are even applications that can generate the code for you like they do in JAVA.

Kohana uses to many static classes to pass things around and dependencies can not be mapped out visually.

You don't know OOP! go back and do more research and try and filter though the crap thats put out by idiots! Like the people at Zend.


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


They are not prone to breaking with each upgrade idiot and if you followed the SVN you can manually make the changes to upgrade. stop making stuff up.

You don't need to upgrade. if your store is running fine you leave it alone. if you have ever worked on a e-commerce site for a big company you would know that there are 1000's of changes the company would have done to the site making it impossible to upgrade without breaking things. One company I woulrked for created order a 1000 different types of reports. if you had enough experiance in the business you would know this!


cstuder wrote:Coding guidelines
The trailing ?> are also not required and always the source for potential problems.


Idiot

cstuder wrote:Better language file handling
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');





This is because i'm keeping each layer separate there by keeping the framework flexible. The language adds another layer to the application which causes longer development time. When I build other applications that don't require multiple languages which is 99% of the time.

cstuder wrote: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:
Code: Select all
return $this->data;


Then in system/engine/loader.php: Modify the language()-method in the following way:
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);
   }


Now, in your controller, you can pass all language strings to the view directly and save millions of keystrokes:
Code: Select all
$this->data = array_merge($this->data, $this->load->language('payment/' . $this->modulename));



Wrong. I have already gone down this root and unseen problems occur that I can not remember off the top of my head at the moment. Better way is to use the language object directly in the view!

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


no its not! not all applications require authorisation. Its application specific thereby shoudl not be included int he framework. I don;t know where you get this rubish from. there is no MVC diagrams that show an authorisation layer. Besides there are so many different types of authorisation that might be required its got to be done at application level.

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


ok true. not hard to fix in future releases.

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


If you want and expanding button with rounded corners you have to use CSS buttons and not the standard form buttons. Come on! this is simple stuff.

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



PHP is a template system. Smarty is slow and bloated and forces people to learn another wasted language that will disaper in time.



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


You have a lot to learn! your head has been filled up with nonsense probably from forums like sitepoint.
OpenCart®
Project Owner & Developer.
OpenCart commercial support now available!
User avatar
Daniel
Administrator
 
Posts: 5310
Joined: Fri Nov 03, 2006 5:57 am

Re: General feedback and suggenstions about OpenCart

Postby cstuder » Tue May 18, 2010 1:29 pm

I'm speechless.

I will now go, and re-evaluate my life. Thank you for your response.
cstuder
 
Posts: 6
Joined: Fri Apr 30, 2010 9:14 am
Location: Switzerland

Re: General feedback and suggenstions about OpenCart

Postby Qphoria » Tue May 18, 2010 2:47 pm

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.

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.

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.

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.


And would not require the insane and error prone patching stuff that SMB does.

Desperation leads us the wrong way sometimes :P

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.

There 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.

Coding guidelines
The trailing ?> are also not required and always the source for potential problems.

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.

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');


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:
Code: Select all
return $this->data;


Then in system/engine/loader.php: Modify the language()-method in the following way:
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);
   }


Now, in your controller, you can pass all language strings to the view directly and save millions of keystrokes:
Code: Select all
$this->data = array_merge($this->data, $this->load->language('payment/' . $this->modulename));



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.

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.

A fair point.

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.

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.


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.

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.

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.

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!
Q
ImageImage
Donate!|OpenCart Basics|GeoZones
Help me get more development cloud storage - Click Here to get DropBox
User avatar
Qphoria
Administrator
 
Posts: 19127
Joined: Mon Jul 21, 2008 2:02 pm
Donate to Qphoria

Re: General feedback and suggenstions about OpenCart

Postby Qphoria » Tue May 18, 2010 4:13 pm

Well I have to say I disagree with most of what Daniel has said.. and admittedly it was in bad form. cstuder actually does have some very good points and it was delivered in a very objective and helpful manner.

Bad Daniel, you can be more open-minded and less defensive. Extremely bad form to insult him for his feedback when he was simply offering ideas.
ImageImage
Donate!|OpenCart Basics|GeoZones
Help me get more development cloud storage - Click Here to get DropBox
User avatar
Qphoria
Administrator
 
Posts: 19127
Joined: Mon Jul 21, 2008 2:02 pm
Donate to Qphoria

Re: General feedback and suggenstions about OpenCart

Postby i2Paq » Tue May 18, 2010 4:57 pm

I was also taken by the way cstuder was beheaded while the points he made were fair and positive.

Bad hair-day Daniel? ::)
Norman in 't Veldt
Moderator OpenCart Forums

_________________ READ and Search BEFORE POSTING _________________
Our FREE search: Find your answer FAST!.

FREE manuals: ShowMe Guides OpenCart, Opencart Check List and Our Documentation section.

BUGs?: Known BUGS for All OC Versions.

[How to] OC 1.5.4.x en hoger; BTW + Verzend + betaalmethodes.
User avatar
i2Paq
Global Moderator
 
Posts: 10870
Joined: Mon Nov 09, 2009 6:00 am
Location: Winkel - The Netherlands

Re: General feedback and suggenstions about OpenCart

Postby cstuder » Wed May 19, 2010 2:11 am

Thanks Qphoria for your answers.

Good luck in further developments. I, for my part, don't really feel like commenting here anymore.
cstuder
 
Posts: 6
Joined: Fri Apr 30, 2010 9:14 am
Location: Switzerland

Re: General feedback and suggenstions about OpenCart

Postby Daniel » Wed May 19, 2010 3:25 pm

I did go over the top. I admit it. sorry!

I take offence at what you said as it is just misleading people.

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.

So we lured you in? We tricked you into thinking opencart was good but you decided it was not?

Compared to what? Magento? Whose code base is a complete mess! or oscommerce or zen-cart which uses procedural programming? or prestashop which does not use MVC at all and is a flacky structure at best.


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.

None of OpenCarts extensions are hard coded.
OpenCart®
Project Owner & Developer.
OpenCart commercial support now available!
User avatar
Daniel
Administrator
 
Posts: 5310
Joined: Fri Nov 03, 2006 5:57 am

Re: General feedback and suggenstions about OpenCart

Postby JNeuhoff » Thu May 20, 2010 6:56 am

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 been pondering over this problem for a while, and I think I have come with a solution which goes beyond what Kohana does, taking into account the possibility that different contributions which are not aware of each other, and which may modify some of the same core classes. When I have time, I'll implement a demo and test it out.
J.Neuhoff - MHC Web Design

OpenCart Override Engine (Version 6.3)
allowing addons to override and modify core methods, language files and templates (see also FAQ)
User avatar
JNeuhoff
 
Posts: 2465
Joined: Tue Dec 04, 2007 2:38 pm

Re: General feedback and suggenstions about OpenCart

Postby greetingsfromreddit » Thu May 20, 2010 8:50 am

*knip*

Edit by i2Paq: If you come here just to do that you just got yourself banned.
greetingsfromreddit
 
Posts: 1
Joined: Thu May 20, 2010 8:46 am

Re: General feedback and suggenstions about OpenCart

Postby system » Thu May 20, 2010 11:37 am

After reading the OP and the subsequent replies, I too am disheartened by the state of Open Cart. It appears that the code base has some way to go before it uses a true MVC framework concept: the views (somewhat) and controllers are evident, but the use of coherently constructed models within the application is poor.

For a start, using database queries in the main directory index code file is not good practise by any stretch. Use a database model class for the relevant table that can be used by a class that gets the information that you require, e.g. a Language class. Having an error handling function defined in this same file that uses global variables is another example of things that should be eliminated - they will cause issues further down the line.

I hope that this topic will create productive discussion within the Open Cart community - there is great potential for this product. It would be a shame for it to die under the cold, dark shade of a personal fiefdom. Open Source means just that: open.
system
 
Posts: 1
Joined: Thu May 20, 2010 11:30 am

Re: General feedback and suggenstions about OpenCart

Postby JNeuhoff » Thu May 20, 2010 12:00 pm

Hi system,

Welcome to the OpenCart forum. And thank you for your feedback.

I agree with you that DB queries shouldn't be in the in main index.php file, but should be delegated to an appropriate model. These queries only got added in recent releases as part of adding the multi-store feature from what I can see. The multi-store feature is still somewhat work in progress though its basic functionality works fine.

As regards the model: OpenCart does not use a strict ORM, rather it has model classes grouped by their major intended functionality. Quite often it is not possible to easily do an ORM especially when more complex DB queries are used involving multiple DB tables, JOINS, and GROUP BYs, and nested SELECTs. I think OpenCart project has done quite a good job in general when it comes to the model. I have used ORMs in the past, e.g. in Kohana, or Java JBoss, and I can assure you I always found them quite cumbersome to use.
J.Neuhoff - MHC Web Design

OpenCart Override Engine (Version 6.3)
allowing addons to override and modify core methods, language files and templates (see also FAQ)
User avatar
JNeuhoff
 
Posts: 2465
Joined: Tue Dec 04, 2007 2:38 pm

Re: General feedback and suggenstions about OpenCart

Postby Daniel » Thu May 20, 2010 8:01 pm

system wrote:After reading the OP and the subsequent replies, I too am disheartened by the state of Open Cart. It appears that the code base has some way to go before it uses a true MVC framework concept: the views (somewhat) and controllers are evident, but the use of coherently constructed models within the application is poor.


First off you can not have true MVC in PHP because its not the same as a desktop language where you use the observer pattern to catach the user interactions like onclick, dbclick etc..

At best its just a seperation of concerns.

As for ORM's they are flacky at best. you will never get the full flexibilty with an ORM as you do with RAW SQL. You might as well just use an active record pattern for basic CRUD functionality.

system wrote:For a start, using database queries in the main directory index code file is not good practise by any stretch. Use a database model class for the relevant table that can be used by a class that gets the information that you require, e.g. a Language class. Having an error handling function defined in this same file that uses global variables is another example of things that should be eliminated - they will cause issues further down the line.


There is nothing wrong with putting db queries in the index.php. You are allowed to setup whatever you need for what could be called a bootstrap to get your applications going. I used a db query to build up a list of config settings. depending on the application I'm building it could be loadeed from a file.

If everything has to be done from the MVC stand point then I could just use add a pre action called startup to run before the user request is executed.

As for the error handling function using "globals" well this could not be avoided. Contact the PHP.net to complain why they gave us such a bad error handling function. I hate globals as well. I rally don't want to have to add another class just for error handling when the log class does virtually the same thing.
OpenCart®
Project Owner & Developer.
OpenCart commercial support now available!
User avatar
Daniel
Administrator
 
Posts: 5310
Joined: Fri Nov 03, 2006 5:57 am

Re: General feedback and suggenstions about OpenCart

Postby gurubob » Fri May 21, 2010 6:51 am

Woo, fun thread. Good comments both ways. I have one thing to add - I'm new to OpenCart and am in the process of getting my hands dirty with a custom module. I notice that in the database drivers (mmsql and mysql) that a failed query is handled with an exit() which immediately terminates the application and outputs the query that was run.

e.g. mysql.php, line 49:
Code: Select all
exit('Error: ' . mysql_error($this->connection) . '<br />Error No: ' . 
    mysql_errno
($this->connection) . '<br />' . $sql); 


Can I suggest that these exits are turned into exceptions so that developers can catch these and act appropriately?, e.g.:

Code: Select all
throw new Exception('Error: ' . mysql_error($this->connection) . '<br />Error No: ' . 
    mysql_errno
($this->connection) . '<br />' . $sql); 


Finally, displaying the query in the browser is very useful to a developer but also gives a hacker a lot of ideas about what's going on too - I would be inclined to omit these or log them to an error log instead. While the OpenCart core may be pretty tight, contributions may not enjoy the same level of robustness.

OpenCart v1.4.7 by the way. Excuse me if I've missed something obvious :)

-B.
--
Bob Brown, Lead Web Developer
Turboweb Limited, www.turboweb.co.nz
Twitter: @gurubobnz, Facebook: facebook.com/turboweb
User avatar
gurubob
 
Posts: 10
Joined: Fri May 21, 2010 6:09 am
Location: Dunedin, NZ

Re: General feedback and suggenstions about OpenCart

Postby Qphoria » Fri May 21, 2010 9:00 am

I agree the exit is a bit neanderthalic. Especially for little things like language loading. A simple throw would suffice rather than taking out the entire site for 1 missing language file. I will look into this. It probably won't happen until 1.4.9 so that I can spend more time with a more robust option.

Thanks!
ImageImage
Donate!|OpenCart Basics|GeoZones
Help me get more development cloud storage - Click Here to get DropBox
User avatar
Qphoria
Administrator
 
Posts: 19127
Joined: Mon Jul 21, 2008 2:02 pm
Donate to Qphoria

Re: General feedback and suggenstions about OpenCart

Postby Daniel » Fri May 21, 2010 3:44 pm

it might be better to use php's bulit in error_log function.
OpenCart®
Project Owner & Developer.
OpenCart commercial support now available!
User avatar
Daniel
Administrator
 
Posts: 5310
Joined: Fri Nov 03, 2006 5:57 am

Re: General feedback and suggenstions about OpenCart

Postby Xsecrets » Fri May 21, 2010 4:32 pm

also the fact that it displays the error will actually make it fail some pci compliancy scanners if they can force an error. I know mcafee will fail if it sees anything like that.
Xsecrets
 
Posts: 5027
Joined: Sat Oct 24, 2009 2:51 pm
Location: FL US

Re: General feedback and suggenstions about OpenCart

Postby Daniel » Fri May 21, 2010 5:42 pm

I know. but from a framework perspective i want to make things as smple as possible. having another argment to say display db errors or not is annoying.
OpenCart®
Project Owner & Developer.
OpenCart commercial support now available!
User avatar
Daniel
Administrator
 
Posts: 5310
Joined: Fri Nov 03, 2006 5:57 am

Re: General feedback and suggenstions about OpenCart

Postby gurubob » Mon May 24, 2010 5:11 am

Daniel wrote:it might be better to use php's bulit in error_log function.

Yep, that'd be perfect for a production environment - use error_log and throw an exception. The developer can then set a global exception handler that stores the exception in the session, redirects to a page showing someone tripping over a power cord or somesuch, and then pulls the exception from the session again and does something with it (email, log, blah...)

Code: Select all
// Store the exception in the session so we can get it when we redirect to an error page
set_exception_handler(function($exception){
  $_SESSION['data']['exception'] = $exception;
  header('Location: ' . HTTP_SERVER . '/index.php?route=error/error');
});
 


Note: The above uses PHP5.3's anonymous function syntax so won't work on older versions of PHP.

-B.
--
Bob Brown, Lead Web Developer
Turboweb Limited, www.turboweb.co.nz
Twitter: @gurubobnz, Facebook: facebook.com/turboweb
User avatar
gurubob
 
Posts: 10
Joined: Fri May 21, 2010 6:09 am
Location: Dunedin, NZ

Re: General feedback and suggenstions about OpenCart

Postby Xsecrets » Mon May 24, 2010 11:59 pm

bowlpiccom wrote:In response to the thread below, WHY on Earth is Daniel still allowed to interact at all with the general public?? He is a 100% LIABILITY to your company. What the hell? This is NOT how business is done.

what company? what business? OpenCart is a free open source program there is no company or business.
Xsecrets
 
Posts: 5027
Joined: Sat Oct 24, 2009 2:51 pm
Location: FL US

Next

Return to Concepts

Who is online

Users browsing this forum: rph and 15 guests

Hosted by Arvixe Web Hosting