model instances inside Proxy are not cached #4199

pine3ree · 2016-04-03T21:02:06Z

With the new Proxy implementation of model method calls, we end up having a new model instance
for every proxy method call.

$model = new $class($registry);

the code above lies inside the anonymous function which is the one called in the proxy model objects.
You can add a static var counter to verify that new is called multiple times for the same $class.

In my opinion model instances should be cached and shared as it was before, maybe using a new different (something like proxy_model_{route}) registry key.

Also, model $file is already checked included in loader::model() method and the classname $class is already resolved: there is no need to check and include the file again (thus generating redundant filesystem stat calls) and $class could be passed as a 3rd argument into loader::callback().

kind regards

The text was updated successfully, but these errors were encountered:

mhcwebdesign · 2016-04-11T15:43:33Z

In the Override Engine we faced the same problem and decided to replace it with decorators: Each model class instantiation is wrapped up by a decorator class which has the needed methods for the before and after event triggers. Much easier to use IMHO. E.g. something like this:

system/engine/decorator.php:

abstract class Decorator {

    protected $object;
    protected $route;


    public function __construct( $object, $route ) {
        $this->object = $object; // can be a controller or model instance
        $this->route = $route;   // route of the class, e.g. 'catalog/category'
    }


    public function __get( $key ) {
        return $this->object->__get( $key );
    }


    public function __set( $key, $val ) {
        $this->object->__set( $key, $val );
    }


    protected function before( $method, $arguments ) {
        // override this method in an extended class to implement pre-action
        return;
    }


    protected function after( $method, $arguments ) {
        // override this method in an extended class to implement post-action
        return;
    }


    public function __call( $method, $arguments ) {

        // make sure it's a callable method
        if (!is_callable(array($this->object, $method))) {
            trigger_error("Error: Unable to call public method '$method' in class" . get_class($this->object) . "'");
            exit;
        }

        // call pre-action 
        $this->before( $method, $arguments );

        // call the method
        $output = call_user_func_array(array($this->object, $method), $arguments);

        // call post-action
        $this->after( $method, $arguments );

        // return method output
        if ($this->startsWith( get_class($this->object), 'Controller' )) {
            if (!($output instanceof Exception)) {
                return $output;
            } else {
                return false;
            }
        } else {
            return $output;
        }
    }


    protected function startsWith( $haystack, $needle ) {
        if (strlen( $haystack ) < strlen( $needle )) {
            return false;
        }
        return (substr( $haystack, 0, strlen($needle) ) == $needle);
    }
}

system/library/model_decorator.php:

class ModelDecorator extends Decorator {

    // overridden method
    protected function before( $method, $arguments ) {
        // do an event trigger before calling the method
        if ($this->startsWith( get_class($this->object), 'Model' )) {
            $route = $this->route.'/'.$method;
            $trigger = 'model/'.$this->route.'/'.$method.'/before';
            $result = $this->event->trigger($trigger, array_merge(array(&$route), $arguments));
            if ($result) {
                return $result;
            }   
        }
        return null;
    }


    // overridden method
    protected function after( $method, $arguments ) {
        // do an event trigger after having called the method
        if ($this->startsWith( get_class($this->object), 'Model' )) {
            $route = $this->route.'/'.$method;
            $trigger = 'model/'.$this->route.'/'.$method.'/after';
            $result = $this->event->trigger($trigger, array_merge(array(&$route, &$output), $arguments));
            if ($result) {
                return $result;
            }
        }
        return null;
    }
}

igorokb · 2016-04-20T10:23:12Z

I am facing with this issue as well. It makes impossible to use models as actually models, but not just a library of functions. It has never been possible to pass parameters to model constructor via the load->model() method. Now any custom model setter doesn't work either.
For instance we have a model:

class ModelModuleTest extends Model {
   protected $param = 2;
   public function setParam($p){
       $this->param = $p;
   }
   public function getParam(){
       return $this->param;
   }
}

and now we want to use the model in some controller:

$this->load->model('module/test');
$this->model_module_test->setParam(3);
$this->model_module_test->getParam();

The getParam() method will return "2" despite that we have just set the "param" property to "3"

mhcwebdesign · 2016-04-20T14:46:49Z

@igorokb: Try using our

system/engine/decorator.php
system/library/model_decorator.php

Modify the system/engine/loader.php, replacing the lines

            .....
            $proxy = new Proxy();

            foreach (get_class_methods($class) as $method) {
                $proxy->{$method} = $this->callback($this->registry, $route . '/' . $method);
            }

            $this->registry->set('model_' . str_replace(array('/', '-', '.'), array('_', '', ''), (string)$route), $proxy);
            .....

with something like this:

            .....
            $instance = new Model( $route );
            $decorator = new ModelDecorator( $instance, $route );
            $this->registry->set('model_' . str_replace(array('/', '-', '.'), array('_', '', ''), (string)$route), $decorator);
            .....

Personally, I find Daniel's way of using a proxy with the callback methods a bit cumbersome to read its code, I find using the decorator design pattern for this more natural.

pine3ree · 2016-04-20T15:06:02Z

I agree, the decorator pattern fits perfectly in this scenario.
Whichever way it will be implemented, the final goal should be to have one and only one service instance per model class.
kind regards

igorokb · 2016-04-20T15:15:31Z

Thanks @mhcwebdesign,
I've already resolved it in a bit dirty but faster way:
So I left in place:
$this->load->model('module/somemodel');
which handles correct including of the corresponding source file
and once the class has been included I can use it as I want to:
E.g.

$model = new SomeModel($this->registry);
$model->doSomeStuff();

It is not the best practice to instantiate an object and abandon it though (I am referring to the one which is created inside load->model()). However the solution should be compatible with OC1.5 and OC 2 including the latest version.

However by adding a comment here my goal was not to get a solution for my particular case but attract the attention to this issue, since there are lots of extensions, which now have to be adjusted in sake of compatibility with 2.2.0.0

pine3ree · 2016-04-20T16:23:10Z

I am closing the issue since the proxy is now cached in the registry container as the model service and the model instances are cached in a static var inside the callback method.

MrClone420 · 2021-01-08T21:54:12Z

I have an issue on this

Undefined property: Proxy::getConfigArray

So after I tried to upload a AMP module ( actually it was just 2 folders ) to opencart version 2.2.0.0 I got the errors I will list below. I then deleted the files to try and clear the error, but ended up still having them and now I can not login to admin, nor can I see my site. Here are the error codes that I am getting. ( also I am new to this so please explain like you would to a child )

at my sites homepage I get this

Notice: Undefined property: Proxy::getConfigArray in /home/s5ri4g25zhe7/public_html/system/storage/modification/system/engine/action.php on line 44

In error files I get these

[08-Jan-2021 13:17:40 UTC] PHP Fatal error: Uncaught Error: Class 'Proxy' not found in /home/s5ri4g25zhe7/public_html/system/storage/modification/system/engine/loader.php:43
Stack trace:
#0 /home/s5ri4g25zhe7/public_html/catalog/controller/startup/startup.php(37): Loader->model('localisation/la...')
#1 /home/s5ri4g25zhe7/public_html/system/storage/modification/system/engine/action.php(44): ControllerStartupStartup->index()
#2 /home/s5ri4g25zhe7/public_html/system/engine/front.php(34): Action->execute(Object(Registry))
#3 /home/s5ri4g25zhe7/public_html/system/engine/front.php(19): Front->execute(Object(Action))
#4 /home/s5ri4g25zhe7/public_html/system/framework.php(99): Front->dispatch(Object(Action), Object(Action))
#5 /home/s5ri4g25zhe7/public_html/index.php(28): require_once('/home/s5ri4g25z...')
#6 {main}
thrown in /home/s5ri4g25zhe7/public_html/system/storage/modification/system/engine/loader.php on line 43

Here is what the files are/say that first location

Uncaught Error: Class 'Proxy' not found in /home/s5ri4g25zhe7/public_html/system/storage/modification/system/engine/loader.php:43
Stack trace:

$proxy = new Proxy();

Here is #0

	$this->load->model('localisation/language');

Here is #1

	return call_user_func_array(array($controller, $this->method), $args);

Here is #2 and #3

foreach ($this->pre_action as $pre_action) {
		
$result = $action->execute($this->registry);

Here is #4

$controller->dispatch(new Action($config->get('action_router')), new

Here is #5

require_once(VQMod::modCheck(DIR_SYSTEM . 'framework.php'));

lastly here is #6

	$proxy = new Proxy();

I am having a hard time finding a fix for my specific issue. Any help would be very much appreciated.

pine3ree closed this as completed Apr 20, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

model instances inside Proxy are not cached #4199

model instances inside Proxy are not cached #4199

pine3ree commented Apr 3, 2016

mhcwebdesign commented Apr 11, 2016

igorokb commented Apr 20, 2016

mhcwebdesign commented Apr 20, 2016 •

edited

Loading

pine3ree commented Apr 20, 2016

igorokb commented Apr 20, 2016

pine3ree commented Apr 20, 2016

MrClone420 commented Jan 8, 2021

model instances inside Proxy are not cached #4199

model instances inside Proxy are not cached #4199

Comments

pine3ree commented Apr 3, 2016

mhcwebdesign commented Apr 11, 2016

igorokb commented Apr 20, 2016

mhcwebdesign commented Apr 20, 2016 • edited Loading

pine3ree commented Apr 20, 2016

igorokb commented Apr 20, 2016

pine3ree commented Apr 20, 2016

MrClone420 commented Jan 8, 2021

mhcwebdesign commented Apr 20, 2016 •

edited

Loading