Using ORM classes directly from the controller in MVC, bad practice?

I've recently delved into using an ORM in my CodeIgniter application and one i've gone for is Propel. Now this gives me the power to basically use Propels 开发者_如何学Goclasses as the 'Model' but is this bad practive?

So my controller code would be as follows:

<?php
    class Page extends Controller {
        function __construct() {
            parent::__construct();  
        }   

        function index() {
            $foo = FooQuery::create()->limit(10)->find();
            $data['content'] = array('foo'=>$foo);
            $this->load->view('home', $foo);    
        }
    }
?>

I want to solve this issue before I carry on developing my application. An example of how I should do this would be very helpful if you do consider this to be bad practice please.

Thanks in advance

Yes, it's bad practice.

The model should contain all of your data logic and abstract it all away from the rest of the program. To the rest of the application, the models should look like black boxes out of which it gets its data. If you use an ORM as your model, you're leaking the abstraction and tightly coupling your controller to your data layer.

Instead, create your models, and deal with the ORM in there. That way if you ever need to adjust your data model, you can just change it in one place (the model layer) and know that the abstraction will hold.

With the Query classes Propel now uses, I think the difference with a more "formal" Model becomes smaller and smaller. If this will become a library that you release into the world it would be an advantage to have an abstraction layer so you can have different backends, but if it is an in-house application I would just use the Query classes as your Model.

But remember that the Query classes are created to feel like an actual object, and that they hide the relational part as much as they can. You can use this to your advantage. Check out this article about rewriting your SQL queries with Query methods, especially the third answer: move more and more into your Query class, so your Controller doesn't feel like it uses a database.

// Instead of this code in your controller,
// tightly coupled to your database logic
$books = BookQuery::create()
   ->filterByTitle('%war%')
   ->filterByPrice(array('max' => 10)
   ->filterByPublishedAt(array('max' => time()))
   ->leftJoin('Book.Author')
     ->where('Author.Name > ?', $fameTreshold);

// You would use this code in your controller
// and create small methods with the business logic in the BookQuery class
$books = BookQuery::create()
  ->titleContainsWord('war')
  ->cheap()
  ->published()
  ->writtenByFamousAuthors();

I've found this to be an occasional necessary evil when your ORM is following the Active Row pattern.

The problem I always run into is that a model only represents a single instance of the data structure. It makes no sense to add collection retrieval methods into the model.

This is where I have historically used a service layer to handle pulling in collections of models. Although to be honest lately I've simply wrote a controller helper object that just abstracts my table object.

It depends a lot on what you are doing and why. in this example you are putting a limit clause in the query - is that business or display logic? From my perspective, it's hard to argue that it's business logic - that I get back 10 elements is irrelevant to the model - that's just how many I think makes sense to using in one page. If you want that rule to be consistent across controllers, you could set some config value to enforce consistency. But putting it in the model just makes the model needless large (there's a difference between fat models and obese models)

I would say that limits, orders and offsets are often not business logic. Even a simple where might or might not be depending on the case. If there's a join there, it's a sign that something is wrong.

The example from Jan Fabry is mostly pretty good. filterByTitle looks about the same to me as titleContainsWord. filterByPublishedAt(array('max' => time())) is much worse than ->published(). In general, the less you controllers need to know about the inner data structure, the better.