[bartMommens]

Hello,

I'm currently building an application that uses the same models in different controllers. Instead of using the normal way, being loading the model in the controller i've build a "core controller" (Foundation_controller) which can load models all other controllers extend from this controller in a way.

In those extended controllers i load my model the following way:

PHP Code:

#Controller example
class Mod_controller extends Foundation_controller
{
private $list_model;
 public function __construct()
    {
        parent::__construct();
        #function from foundation_controller
       $this->list_model = $this->load_model('list_model', true, false);
     }
private function get_full_list(){
   $this->list_model->get_list_info();
}
} 


PHP Code:

#Function to load the models from foundation controller

protected function load_model($model_id, $mandatory = false, $die_on_fail = false)
    {

        $model_data = $this->get_model_info($model_id);

        if(empty($model_data['model_alias']) || empty($model_data['model_path'] || empty($model_data['model_class_name']))
        ){
            #outputs simple json fail object..
            api_message_present_simple_failure_message('mi-error-01: ' . $model_id);
        }

        if ($this->model_did_load_check($model_data['model_class_name'])) {
            #model is already loaded return the object
            return $this->get_model_object_from_alias($model_data['model_alias']);
        }

        if ($this->check_model_php_file_exists($model_data['model_path'])) {
            #load the model here based on file path and alias.
            $this->load->model($model_data['model_path'], $model_data['model_alias']);
            if ($mandatory) {
                if (!$this->model_did_load_check($model_data['model_class_name'])) {
                    if ($die_on_fail) {
                        #outputs simple json fail object..
                        api_message_present_simple_failure_message('mi-error-02: ' . $model_data['model_alias']);
                    }
                    return false;
                }
            }
        } else {
            if ($mandatory) {
                if ($die_on_fail) {
                    #outputs simple json fail object..
                    api_message_present_simple_failure_message('mi-error-03: ' . $model_data['model_alias']);
                }
                return false;
            }
        }
        #model successfully loaded return the object
        return $this->get_model_object_from_alias($model_data['model_alias']);
    }
  
protected function get_model_object_from_alias($model_alias){
        return $this->{$model_alias};
    } 


*It' might look confusing here because i load the model in the constructor, but in some controllers i load a specific model (or get the object) inside of a function or at a certain point in a process.

Everything works but now i'm concerned this could lead to a potential security risk, or bad resource management , since the models are being loaded "somewhat" globally.

Some models are in subfolders and some versions of the app require a different version of a model. Or some app versions don't have some model files at all. (Lite version)

The main thing is that each controller in the app extends from Foundation_controller to use the load function, some controllers are being used for pages with "public" access and others are behind a authenticated users part. The main reason why I have the load_model function is that I don't want to repeat code to check if the model file exists, a particular function exists and that the class has already loaded. Also this approach has less chance for typos and if a model path changes less work to refactor.

Now I have doubts that this is secure, and I wonder if this is a valid way to load models in codeigniter and that it remains secure and stable / performant. 

All remarks and suggestions are welcome.

Best regards, Bart

[dave friend]

It seems to me that the main problem you are trying to solve is

I don't want to repeat code to check if the model file exists, a particular function exists and that the class has already loaded.

Yes, all of those concerns are valid, but you have added a bunch of complexity to solve problems that a developer will find immediately during the code writing/testing process. A non-existent (i.e. misnamed) class will error as soon as an attempt is made to load it. A valid class that is not instantiated will also error on any attempt to use it. A non-existent method is also immediately known to the developer. Resolving those issues is a development task, not a runtime task.

CodeIgniter already handles previously loaded models and libraries in ways that are quickly apparent during development.

With regard to files in subfolders. Constants are a simple, high-performance solution, e.g.

PHP Code:

$this->load->model(SOME_FOLDER . 'some_model'); 


Where the constant would be in some config file like so

PHP Code:

define(SOME_FOLDER, APPPATH . "models/somefolder/"); 


You could define 'some_model' as a constant too if you're in the habit of renaming them often. But any decent IDE has a good refactoring feature or, at the very least, an easy-to-use global search and replace. For code maintenance, readability cannot be understated, so a hard-coded file name is preferred over a constant in my opinion.

In summary, I feel that the existing model loading tools are wholly adequate and that loading should happen at the time and place where it makes the most sense. That time and place determination happens on a case-by-case basis and could be done via /config/autoload.php, in a controller's constructor (which might be in an extended class), or in a controller method.

[bartMommens]

It seems to me that the main problem you are trying to solve is

I don't want to repeat code to check if the model file exists, a particular function exists and that the class has already loaded.

Yes, all of those concerns are valid, but you have added a bunch of complexity to solve problems that a developer will find immediately during the code writing/testing process. A non-existent (i.e. misnamed) class will error as soon as an attempt is made to load it. A valid class that is not instantiated will also error on any attempt to use it. A non-existent method is also immediately known to the developer. Resolving those issues is a development task, not a runtime task.

CodeIgniter already handles previously loaded models and libraries in ways that are quickly apparent during development.

With regard to files in subfolders. Constants are a simple, high-performance solution, e.g.

PHP Code:

$this->load->model(SOME_FOLDER . 'some_model'); 


Where the constant would be in some config file like so

PHP Code:

define(SOME_FOLDER, APPPATH . "models/somefolder/"); 


You could define 'some_model' as a constant too if you're in the habit of renaming them often. But any decent IDE has a good refactoring feature or, at the very least, an easy-to-use global search and replace. For code maintenance, readability cannot be understated, so a hard-coded file name is preferred over a constant in my opinion.

In summary, I feel that the existing model loading tools are wholly adequate and that loading should happen at the time and place where it makes the most sense. That time and place determination happens on a case-by-case basis and could be done via /config/autoload.php, in a controller's constructor (which might be in an extended class), or in a controller method.

Hi Dave,

So basically the logic that I've added "overkill/complex" and not really that great, the main problem is that I'm all alone and don't have anyone to rely on for code reviews / testing. So i had doubts about my code and posted here for that exact reason. So basically it's back to refactor   

Thank you for the feedback.

Best regards,
Bart

[dave friend]

Bart, Sorry if my reply seemed harsh. That wasn't my intention and don't forget, it's only my opinion. At least you don't get shut down here for asking questions that require opinion answers as happens on Stack Overflow.

Stack Exchange (the parent site of Stack Overflow) also has a Code Review site with a CodeIgniter section at https://codereview.stackexchange.com/que...odeigniter. It would be instructive to post there.

[bartMommens]

Dave, no worries i wasn't sure about the rework to begin with.
Your opinion is highly valued!

I've just reverted all my commits on my git repo, with a little fail while doing it but .( Revert "Revert "post added ... ) I'm back 2 where i was before the model_load_v2. But i've made a local backup of the work i've done on the model loader. My question also has been posted on codereview.stack... but you are the only one who gave some feedback. Normally when i have doubts about something they turn out to be justified.

It might be better just to follow codeigniter's standards and maybe think about this strategy when porting from 3.1.10 to 4.x

Greets,
Bart