Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ENHANCEMENT] autoload models #215

Closed
ghost opened this issue Sep 1, 2013 · 10 comments
Closed

[ENHANCEMENT] autoload models #215

ghost opened this issue Sep 1, 2013 · 10 comments

Comments

@ghost
Copy link

ghost commented Sep 1, 2013

hi,

I'm just learning MVC and I wonder why you don't autoload the models... like so

function autoload($class) {

if (file_exists(LIBS . $class . ".php")) {

    require LIBS . $class . ".php";
} elseif(file_exists(MODELS . $class . ".php")) {

    require MODELS . $class . ".php";
}else{...}

}

I don't know if this is better but I guess If you register an autoload function you better use it if you can.

peace

@panique
Copy link
Owner

panique commented Sep 1, 2013

Hey, this is exactly what the script does, but maybe I'm misunderstanding you here:

https://github.com/panique/php-login/blob/master/4-full-mvc-framework/libs/Controller.php

/**
* loads the model with the given name.
* loadModel("test") would include models/test_model.php and create the object $this->model in the controller
*
* @param string $name The name of the model
*/
    public function loadModel($name) {

        $path = 'models/' . $name . '_model.php';

        if (file_exists($path)) {
            require 'models/' . $name . '_model.php';

            $modelName = $name . '_Model';
            $this->model = new $modelName();
        }
    }

@ghost
Copy link
Author

ghost commented Sep 1, 2013

yes, you don't need the loadModel($name) function if you extend the autoload($class) function. these functions do the same thing so it is duplicate code.

edit: by "extend function" I mean add an extra elseif to handle the model autoloading

@ghost
Copy link
Author

ghost commented Sep 1, 2013

Well, you do need the loadModel($name) function but only for creating a model object, not for loading (requiring) the class. This can be handled by the autoload($class) function.

@panique
Copy link
Owner

panique commented Sep 1, 2013

Ahh I think I really misread you ;)

You are talking about index.php !? Well, I think you are totally right, but it looks cleaner to let the model load from controller.php, not index.php. The basic MVC behind the script was not written by me (have a look into the readme, I used a popular framework codebase for this). The idea behind MVC is that the Controller loads the model (afaik?), and I've seen projects where the controller loads multiple models or loads a model that is not what you would expect (Login controller loads shopping model for example). Afaik the definiton of MVC says that there's always just one model, and only this one model object is passed in whole or partly to the view.

I'm currently working on a Groovy project where the controller creates an empy model object and then passes data into it by calling "Services", like

$model->myUserName = $userService->getUserName();
$model->myTotalPrice = $shoppingService->getTotalPrice();

and so on. This shows a model (is it really a model ?) that gets information from 2 other models/services. In this case a model autoloader in index.php would be a problem.

But I'm really not deep into Design Patterns, MVC etc, so feel free to discuss this further ;)

@ghost
Copy link
Author

ghost commented Sep 1, 2013

I'm just starting to learn MVC based on the same youtube series your project is based on. I really like it :-)
However, if you make the changes I suggested, the model will still be loaded by the controller when it creates a model object (the spl_autoloader can handel the modelclass loading). So what I mean is that you don't need to require classes through the entire script. It all can be done with the spl_autoloader. I think if you use an autoloader you better use it always/when possible.

@ghost
Copy link
Author

ghost commented Sep 1, 2013

also an enhancement would be to wrap the autoload function into a class (see link)

http://stackoverflow.com/questions/4765424/php-autoload-mvc

@panique
Copy link
Owner

panique commented Sep 1, 2013

Please note: We should keep the project SIMPLE and HUMAN-READABLE. The problem with "professional" software is usually that it's completely unreadable and extremely complex, so nobody really knows whats going on and how to change things. I believe that this is a major problem in IT: "Good", but unintuitive code. PHP has got some very interesting coding standard rules in the last years, but some of these rules make it harder to read code for non-professionals (as those rules are usually made by the coding elite, not by the casual coder).

Anyway, I'm losing the point here ;)

Is it really better to create a new class, just for an autoloader (and will people realize what going on there ?) ? Seems legit, as we already have other classes that consist of just a few lines of code (database.php, auth.php). What do the others think ?

@ghost
Copy link
Author

ghost commented Sep 1, 2013

The point is that you use OOP through out the whole project and then suddenly in the main index file there is a solitary function autoload($class) which does not belong there. Its nicer wrapped in a class. Also OOP enforce the possibility of code reuse so why not just do it. you can get rid of every required function in the entire project. This is possible because of the MVC architecture. All files are named and organised in a particular way.

@panique
Copy link
Owner

panique commented Sep 1, 2013

Touché ! Excellent point! I'll do that (wrapping autoloader in class) when there's time...

@panique
Copy link
Owner

panique commented Dec 1, 2013

There's a new version of 4-full in the develop branch, that goes into another direction: allowing multiple models per controller by loading them manually. Have a look if you like (this was requested by some people).

@panique panique closed this as completed Dec 1, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant