OpenCart Community

Posted: **Wed Aug 25, 2010 12:25 am**

As it is now, there is a lot of repetitive code in many of the controllers. While there are lots of places it can be improved, I started with the admin/controller/setting/setting.php file, and found a way to go from 1000 lines of code to just 330 lines of code using a for loop.

There were 3 changes made
1. Let language load array merge directly with $this->data
I added a small change in 1.4.8 to return the results back from the language->load function instead of just setting it. This tiny change allows us to get rid of thousands of lines of tedious language entry defines.

So in the setting.php file I removed all language defines like these:

Code: Select all

$this->data['heading_title'] = $this->language->get('heading_title');
$this->data['text_none'] = $this->language->get('text_none');
$this->data['text_yes'] = $this->language->get('text_yes');
$this->data['text_no'] = $this->language->get('text_no');
$this->data['text_image_manager'] = $this->language->get('text_image_manager');
$this->data['text_add_store'] = $this->language->get('text_add_store');
$this->data['text_edit_store'] = $this->language->get('text_edit_store');
$this->data['text_mail'] = $this->language->get('text_mail');
$this->data['text_smtp'] = $this->language->get('text_smtp');
$this->data['entry_name'] = $this->language->get('entry_name');
$this->data['entry_url'] = $this->language->get('entry_url');	
$this->data['entry_owner'] = $this->language->get('entry_owner');
$this->data['entry_address'] = $this->language->get('entry_address');
$this->data['entry_email'] = $this->language->get('entry_email');
....
....
....
$this->data['tab_server'] = $this->language->get('tab_server');

And simply changed:

Code: Select all

$this->load->language('setting/setting')

to

Code: Select all

$this->data = array_merge($this->data, $this->load->language('setting/setting'));

Now, any new entry in the language file will be automatically pushed to the view without having to double define each entry in the controller. You can still define an entry manually as well when you want to do something with sprintf or override a setting from the language file. So it is completely transparent.

2. Made an errors array
The code for each error is exactly the same. Might as well just code it once and loop through each error:
I Removed all this:

Code: Select all

if (isset($this->error['warning'])) {
			$this->data['error_warning'] = $this->error['warning'];
		} else {
			$this->data['error_warning'] = '';
		}
		
 		if (isset($this->error['name'])) {
			$this->data['error_name'] = $this->error['name'];
		} else {
			$this->data['error_name'] = '';
		}
		
 		if (isset($this->error['url'])) {
			$this->data['error_url'] = $this->error['url'];
		} else {
			$this->data['error_url'] = '';
		}
		
 		if (isset($this->error['owner'])) {
			$this->data['error_owner'] = $this->error['owner'];
		} else {
			$this->data['error_owner'] = '';
		}

 		if (isset($this->error['address'])) {
			$this->data['error_address'] = $this->error['address'];
		} else {
			$this->data['error_address'] = '';
		}
....
....
.....
....
....
if (isset($this->error['admin_limit'])) {
			$this->data['error_admin_limit'] = $this->error['admin_limit'];
		} else {
			$this->data['error_admin_limit'] = '';
		}

And added this:

Code: Select all

$errors = array(
	'warning',
	'name',
	'url',
	'owner',
	'address',
	'email',
	'telephone',
	'title',
	'image_thumb',
	'image_popup',
	'image_category',
	'image_product',
	'image_additional',
	'image_related',
	'image_cart',
	'error_filename',
	'catalog_limit',
	'admin_limit'
);

foreach($errors as $error) {
	if (isset($this->error[$error])) {
		$this->data['error_' . $error] = $this->error[$error];
	} else {
		$this->data['error_' . $error] = '';
	}
}

3. For loop for all settings from the database
Again, the code for each field on the view is the exact same if/else with only the field name changing. But why bother hardcoding it when our current standard is to name the db fields the same as the form post fields. Instead, following that design, we can just loop through them as variables.
So I deleted all this:

Code: Select all

if (isset($this->request->post['config_name'])) {
			$this->data['config_name'] = $this->request->post['config_name'];
		} else {
			$this->data['config_name'] = $this->config->get('config_name');
		}
		
		if (isset($this->request->post['config_url'])) {
			$this->data['config_url'] = $this->request->post['config_url'];
		} else {
			$this->data['config_url'] = $this->config->get('config_url');
		}
		
		if (isset($this->request->post['config_owner'])) {
			$this->data['config_owner'] = $this->request->post['config_owner'];
		} else {
			$this->data['config_owner'] = $this->config->get('config_owner');
		}

		if (isset($this->request->post['config_address'])) {
			$this->data['config_address'] = $this->request->post['config_address'];
		} else {
			$this->data['config_address'] = $this->config->get('config_address');
		}
		
		if (isset($this->request->post['config_email'])) {
			$this->data['config_email'] = $this->request->post['config_email'];
		} else {
			$this->data['config_email'] = $this->config->get('config_email');
		}
....
...
...
....
if (isset($this->request->post['config_error_filename'])) {
			$this->data['config_error_filename'] = $this->request->post['config_error_filename']; 
		} else {
			$this->data['config_error_filename'] = $this->config->get('config_error_filename');
		}

And replaced it with a simple for loop using the settings from the database:

Code: Select all

$settings = $this->model_setting_setting->getSetting('config');

foreach ($settings as $key => $value) {
	if (isset($this->request->post[$key])) {
		$this->data[$key] = $this->request->post[$key];
	} else {
		$this->data[$key] = $value;
	}
}

That's it! 400+lines of code replaced with 8!
This also makes it easier to add new fields. You no longer need to edit the controller to add the new field set. Simply add it to the tpl and language file and you are done. This makes development of the core as well as mods much easier and reduces the excess copy/paste code. The same changes can be made to product, category, coupons, etc. All files can benefit from this. Thousands of lines of code can be removed and development efficiency can be increased substantially.

It also stays inline with keeping the framework separate from the app. I'd like this to be the new standard to follow for future development. It also doesn't break any existing code since there are no functional changes so it is backwards compatible.

Thoughts?

Posted: **Wed Aug 25, 2010 1:22 am**

Stunning!

I'm not a coder, by far, and sometimes people make me sit back and admirer them by their work.

This is one of those rare moments Qphoria

I'm all up for a K.I.S.S. approach so I say yes!

It should be the standard, and I agree it should be used where possible.

Posted: **Wed Aug 25, 2010 3:23 am**

Sounds great, I did adopt the language array merge in my nivoslider back when I created it, but it doesn't make it backwards compatible unfortunately to 1.4.7 as I soon found out

That said, I've not created a compatibility for 1.4.7 though because I actually think it's of use to people to keep up with the store updates

Great work for the other modifications. I have to say I would like things like the language if not made to automatically run like above, to have the option of being a built in method of the controller file, since all controller files use it, and it would be nice to just have $this->loadLanguage(); and it's done just like your method above with the array merge.

I'd also like to see the admin settings being cached too, since they remain constant unless explicitly changed in the admin area

I also found out the other day that there is no way to set a minimum default cart amount, which kind of stunned me for a second, I thought that would have been in it for sure

Posted: **Wed Aug 25, 2010 3:45 am**

JAY6390 wrote: I also found out the other day that there is no way to set a minimum default cart amount, which kind of stunned me for a second, I thought that would have been in it for sure

Ya.. i have it as a mod.. but it may make it into the core soon.

Posted: **Wed Aug 25, 2010 3:56 am**

Ah right, cool

Posted: **Wed Aug 25, 2010 3:59 am**

JAY6390 wrote:I have to say I would like things like the language if not made to automatically run like above, to have the option of being a built in method of the controller file, since all controller files use it, and it would be nice to just have $this->loadLanguage(); and it's done just like your method above with the array merge.

Yea, I guess Daniel wants to keep the language outside of the controller engine, and the controller engine outside of the language class. So this 1 line work around was the best compromise I could come up with.

Posted: **Wed Aug 25, 2010 4:47 am**

so when will all this start? Is 1.4.9 going to be updated with this standard? seems like we could shave quite a bit off the download size using this, not to mention it will make it much easier for average people making mods if they can just add something to the language file and then call it directly in the .tpl file without having to go digging into the controller file since it's more hardcore php and scares non coders.

Posted: **Wed Aug 25, 2010 4:56 am**

1.4.9 is done and on the way out so it won't happen there, however, it does support the language merge already since 1.4.7. And the other options are simply coding methods that have always been available. But as Jay pointed out, backwards compatibility for mods is affected by the language merge so I figured I'd slip it in silently and let it persist for a few versions. But since 1.5.x is no holds barred anyway, this would be a good time to start officially pushing it in the core and modders would likely support only 1.4.7 and later.

Daniel still has to agree of course, but hopefully 1.5.0.
I do have it running with my 1.4.9 test store and I could release a "modpack" of all the files that I edit for people to use with their 1.4.9 stores as I planned on it for my own. These would have no change in functionality, just a smaller codebase. Just to be sure there are no problems that I've overlooked. But I used this method in ChromiumCart and never had a problem there.

Posted: **Wed Aug 25, 2010 4:57 am**

Agree, a lot of people are waiting on a stable and bug-free 1.4.9x so my suggestion is to wait until 1.5.0

A modPack would be interesting.

Posted: **Thu Feb 03, 2011 7:24 pm**

My, my, my!

This is sweet!
Is there a performance reduction or increase versus the older way?

Did this make it into 1.5?

Also, wouldn't this be a great cache victim?

Posted: **Thu Feb 03, 2011 7:34 pm**

The language difference would certainly mean a performance increase, since it's removing hundreds of lines of code and just copying it directly in one swoop to the data variable. As for caching, there's very little reason to do so. I would argue that the speed of both loading the language file into memory and loading a cache file are going to be almost identical, since they're essentially the same operation. Cache files would be duplicating the data so it would take up more hard disk space for no reason at all

Posted: **Fri Feb 04, 2011 4:04 am**

Qphoria wrote:I started with the admin/controller/setting/setting.php file, and found a way to go from 1000 lines of code to just 330 lines of code using a for loop.

I just implemented this on my MAMP install and everything is working beautifully. Thanks, Q! I, too, hope this makes it into the core soon.

Posted: **Mon Feb 14, 2011 6:13 am**

I used these ideas to create a standard admin module controller, where you only have to edit a few lines when editing/creating a module. Basically, it defines the name of the module once and sets up the settings in the database using the 1.4.9 install() function. Then, using the code improvements outlined by Q, it array_merge's the language file and loops through the settings to load them for the view. Here's the code:

Code: Select all

<?php
class ControllerModuleMymodule extends Controller {
    private $error = array();
    private $module_name = 'mymodule';
    private $module_settings = array(
        'mymodule_status'     => '',
        'mymodule_position'   => '',
        'mymodule_sort_order' => ''
    );
    
    public function install() {
        $this->load->model('setting/setting');
        $this->model_setting_setting->editSetting($this->module_name, $this->module_settings);
    }
    
    public function index() {
        $this->data = array_merge($this->data, $this->load->language('module/' . $this->module_name));
        $this->document->title = $this->language->get('heading_title');
        $this->load->model('setting/setting');
        
        $token = $this->session->data['token'];
        $this->data['token'] = $token;

        if (($this->request->server['REQUEST_METHOD'] == 'POST') && ($this->validate())) {
            $this->model_setting_setting->editSetting($this->module_name, $this->request->post);
            $this->session->data['success'] = $this->language->get('text_success');
            $this->redirect(HTTPS_SERVER . 'index.php?route=extension/module&token=' . $token);
        }
                        
         if (isset($this->error['warning'])) {
            $this->data['error_warning'] = $this->error['warning'];
        } else {
            $this->data['error_warning'] = '';
        }
        
        $this->document->breadcrumbs = array();
        
         $this->document->breadcrumbs[] = array(
            'href'      => HTTPS_SERVER . 'index.php?route=common/home&token=' . $token,
            'text'      => $this->language->get('text_home'),
            'separator' => FALSE
         );
        
         $this->document->breadcrumbs[] = array(
            'href'      => HTTPS_SERVER . 'index.php?route=extension/module&token=' . $token,
            'text'      => $this->language->get('text_module'),
            'separator' => ' :: '
         );
        
         $this->document->breadcrumbs[] = array(
            'href'      => HTTPS_SERVER . 'index.php?route=module/' . $this->module_name . '&token=' . $token,
            'text'      => $this->language->get('heading_title'),
            'separator' => ' :: '
         );
        
        $this->data['action'] = HTTPS_SERVER . 'index.php?route=module/' . $this->module_name . '&token=' . $token;
        $this->data['cancel'] = HTTPS_SERVER . 'index.php?route=extension/module&token=' . $token;
        
        $settings = $this->model_setting_setting->getSetting($this->module_name);
        foreach ($settings as $key => $value) {
            $this->data[$key] = (isset($this->request->post[$key]) ? $this->request->post[$key] : $value);
        }
        
        $this->template = 'module/' . $this->module_name . '.tpl';
        $this->children = array(
            'common/header',
            'common/footer'
        );
        
        $this->response->setOutput($this->render(TRUE), $this->config->get('config_compression'));
    }
    
    private function validate() {
        if (!$this->user->hasPermission('modify', 'module/' . $this->module_name)) {
            $this->error['warning'] = $this->language->get('error_permission');
        }
        
        if (!$this->error) {
            return TRUE;
        } else {
            return FALSE;
        }
    }
}
?>

The nice thing is that you only have to change the $module_name and $module_settings to create a new module. Everything else is standard. It might be useful to use this in the core in some way in the future.

Posted: **Mon Feb 14, 2011 6:26 am**

Nice although I'd like to see things like

Code: Select all

         $this->document->breadcrumbs[] = array(
            'href'      => HTTPS_SERVER . 'index.php?route=module/' . $this->module_name . '&token=' . $token,
            'text'      => $this->language->get('heading_title'),
            'separator' => ' :: '
         );

being changed for

Code: Select all

$this->document->addBreadcrumb()

not really just aimed at your module, but in standard OC modules as well that are shipped by default since the methods are there for exactly that purpose. The same also for script and stylesheet additions using addScript and addStylesheet

Posted: **Mon Feb 14, 2011 10:43 am**

NIce Jonathan, Yea i agree we need more generic module code where we only need to supply the 2 variables and it uses that common code. A lot of the code that is in modules needs to be pushed into the core. Things like the geozone check in the payment model files. That is not something the module should be controlling. That should be controlled by the store level as this is a common conformance that has to work for all payment modules. If a 3rd party developer leaves it out it breaks the conformity, and that isn't something that the mod maker should be in control of. All conformance factors should be moved to the core. Once that is done, we can have more control of things like which payments show up with which shipping and disabling certain payment/shipping based on other factors. The way it is now is just wasted code. I have the same model file for all my payment modules, and just change the name. I actually have a modification that puts the universal model code into the checkout/payment controller and removes the need for the payment model file completely. But that is another topic.

For your example, we could maybe take it to the next step and subclass it properly and the admin controller files could simply be:

Code: Select all

<?php
class xxxmod extends ControllerModuleMymodule {
    $mymodule = 'xxxmod';
    $module_settings = array(
        'xxx' => $this->config->get('xxxmod_xxx');
    );
}
?>

And that's it! That would make 3rd party mods much more manageable and have to upgrade much less often.

Regarding the breadcrumb.. daniel has made a rather unfavored move in 1.5.x from what I can see. Breadcrumbs were pulled out of the document class and now each controller has its own breadcrumb build. I dislike this move personally because it's just more repetitive code that each controller will have the same way and we need to back away from that. So I am pushing for putting it back into the document class and properly using the addBreadcrumb functions like we have already

Posted: **Mon Feb 14, 2011 11:43 am**

Qphoria wrote: Regarding the breadcrumb.. daniel has made a rather unfavored move in 1.5.x from what I can see. Breadcrumbs were pulled out of the document class and now each controller has its own breadcrumb build. I dislike this move personally because it's just more repetitive code that each controller will have the same way and we need to back away from that. So I am pushing for putting it back into the document class and properly using the addBreadcrumb functions like we have already

what was his logic behind that move?

Posted: **Mon Feb 14, 2011 12:05 pm**

Erm logic?

Posted: **Mon Feb 14, 2011 7:17 pm**

ffs that seems like a step backwards $:-\$

Posted: **Mon Feb 14, 2011 10:28 pm**

Qphoria wrote:For your example, we could maybe take it to the next step and subclass it properly and the admin controller files could simply be:
Code: Select all
<?php
class xxxmod extends ControllerModuleMymodule {
    $mymodule = 'xxxmod';
    $module_settings = array(
        'xxx' => $this->config->get('xxxmod_xxx');
    );
}
?>
And that's it! That would make 3rd party mods much more manageable and have to upgrade much less often.

Yes, this is the idea I had in mind but didn't know what would be the easiest way. Here's hoping to the getting to this stage someday soon!

Posted: **Mon Feb 21, 2011 11:29 pm**

I had played with this a bit and got it working in its simplest form.

Currently OpenCart has:

Code: Select all

class Controller
    |_ class ControllerModuleLatest extends Controller

This means that ControllerModuleLatest inherits all the stuff inside controller, and adds its own stuff

Since Controller is very generic, it really doesn't have a lot in it specific to any type of code base.
Some different code bases would be:
- Modules
- Payment
- Shipping
- Order Totals
- Controller Pages
Each of those share similar bases with others of their kind. Things like Latest, Specials, Bestseller, Featured, etc.. almost all module share the exact same code, only with different names. This makes it an incredible waste of code duplication for each module. If we made common "base" modules for each type, those could be subclassed and only the finer details would be needed, like Jonathan showed above.

So I tried this. I took the module code from jonathan above and named it ControllerModuleBase
Then I changed the admin/controller/module/latest.php file from
class ControllerModuleLatest extends Controller
to
class ControllerModuleLatest extends ControllerModuleBase

That gives a hierarchy of:

Code: Select all

class Controller
    |_ class ControllerModuleBase extends Controller
         |_ class ControllerModuleLatest extends ControllerModuleBase

I had to include that new file at the top of the latest module, which may show good reason why we should rethink about adding the php autoload function.

So now, the admin/controller/module/latest.php looks like:

Code: Select all

<?php
include('module.base.php');
class ControllerModuleLatest extends ControllerModuleBase {
    $this->module_name = 'latest';
}
?>

Thats it! And it works!
I moved the position and status stuff to the base since they could be looked up by $module name, but obviously there will be other parameters so more values can be supported. But the concept is clean and if we had something like this, making mods would be a lot simpler.

OpenCart Community

Coding Standards Improvement Proposal

Coding Standards Improvement Proposal

Re: Coding Standards Improvement Proposal

Re: Coding Standards Improvement Proposal

Re: Coding Standards Improvement Proposal

Re: Coding Standards Improvement Proposal

Re: Coding Standards Improvement Proposal

Re: Coding Standards Improvement Proposal

Re: Coding Standards Improvement Proposal

Re: Coding Standards Improvement Proposal

Re: Coding Standards Improvement Proposal

Re: Coding Standards Improvement Proposal

Re: Coding Standards Improvement Proposal

Re: Coding Standards Improvement Proposal

Re: Coding Standards Improvement Proposal

Re: Coding Standards Improvement Proposal

Re: Coding Standards Improvement Proposal

Re: Coding Standards Improvement Proposal

Re: Coding Standards Improvement Proposal

Re: Coding Standards Improvement Proposal

Re: Coding Standards Improvement Proposal