[zipzap]

Hello, in order to analyze the structure of the yii'm a little confused with the division between the model and controller concept seen for some times to find logic in the methods of the model such as the model 'LoginForm' have the login method that is then called by the controller 'SiteController'. Why is not all done on the controller?

I am somewhat in doubt where to put the methods.

[redguy]

business logic of application should be placed in models. Controllers just map user actions to business logic and pass the results to view.

[andy_s]

There is a simple principle: fat model, skinny controller. You can read about it there: http://survivethedee...n/1.0/the.model (especially 3.4. part).

[Alpin]

Fat models are preferable for a good design.
Make models as fat as you can and in controllers use model functions, render, access session etc.

[jacmoe]

In other words: put as much as you can in the model.

It also eliminates the strange need to call another controller from a controller..
If you develop that need, you know you need to re-factor your code: controller code needs to be moved into a model.
You can instantiate a model from practically anything.

[Karl Zilles]

As much as you can you should put the logic into models. But the code in the models should be generic enough that it doesn't reference anything related to a HTTP interaction. You should, for example, be able to create models and use them from a console application.

Put any code that references HTTP interactions in controllers. So code that reads variables from form inputs, uses sessions, uses cookies, and code that calls for renders or redirects goes in the controller.

[jacmoe]

Whether you are using a testing framework or not, testing your models in a console app is great - agree with Zilles.
This also means that you can automate your app through cron scripts, commands, etc. later if need be.

[andy_s]

By the way... For quite a long time I'm saving uploaded files in CActiveRecord.afterSave() method using

$file = CUploadedFile::getInstance($this, 'file');
// other manipulations...

But it is an indirect access to $_FILES array, and like accessing $_GET or $_POST values inside a model it should be a bad practice? CFileValidator does the same. Any thoughts?

[sidewinder]

That works great. I'm also doing file operations in model events. However, my code goes into beforeSave and beforeDelete functions so in case of filesystem error I can simply set error message and return false to abort operation.
Accessing $_FILES in model makes testing much harder. It's better to pull data from global arrays in controller, and then copy relevant informations into model. Not as efficient but easier to maintain.

[jacmoe]

$_GET and $_POST should be kept out of the model, IMO.
A perfect example of what a controller should handle.

[seb]

andy_s, on 08 February 2012 - 01:58 AM, said:

By the way... For quite a long time I'm saving uploaded files in CActiveRecord.afterSave() method using

$file = CUploadedFile::getInstance($this, 'file');
// other manipulations...

But it is an indirect access to $_FILES array, and like accessing $_GET or $_POST values inside a model it should be a bad practice? CFileValidator does the same. Any thoughts?

I prefer to get uploaded file instance in the controller and then pass it to the model, something like this:

//controller
$uploaded = CUploadedFile::getInstance($model, 'attachment');
if ($model->validate() && $model->createAttachment($uploaded)) {
   $model->save();
}

This way file processing code is still in the model, but access to $_FILES becomes more indirect.
For tests, for example, I can extend CUploadedFile and emulate uploaded file.
And then this mock object can be passed to the $model->createAttachment(...).

[andy_s]

I agree moving the creation of CUploadedFile instance into a controller is a good idea, but what makes it senseless is using that instance inside a model. I think the model should receive a filename or some CFile object not associated with a particular http request. Same for CFileValidator which also uses CUploadedFile and even tries to create a new instance if it's not created yet. But all this stuff only makes things complicated, and many people don't care at all

[seb]

andy_s, on 08 February 2012 - 12:03 PM, said:

I agree moving the creation of CUploadedFile instance into a controller is a good idea, but what makes it senseless is using that instance inside a model. I think the model should receive a filename or some CFile object not associated with a particular http request...

I understand your point, but I still think that manipulation with CUploadedFile makes sense, because it is the model should decide where the file should be saved. For example, User model can save logo file to the files/logos/{user_id} folder and Post model can save file to the files/posts/attachments and so on.

[andy_s]

seb, on 08 February 2012 - 01:43 PM, said:

I understand your point, but I still think that manipulation with CUploadedFile makes sense, because it is the model should decide where the file should be saved. For example, User model can save logo file to the files/logos/{user_id} folder and Post model can save file to the files/posts/attachments and so on.

No doubts, only the model should know where its files are stored and how to save/get them. I just don't like the fact I should rely on CUplodedFile class inside models, because it's always associated with $_FILES array

[jacmoe]

I prefer to deal with CUploadedFile in the controller. It definitely belongs in the http logic department. Agree with Andy.
The model should be able to deal with any file, not only uploaded files.

[seb]

andy_s, on 08 February 2012 - 01:47 PM, said:

No doubts, only the model should know where its files are stored and how to save/get them. I just don't like the fact I should rely on CUplodedFile class inside models, because it's always associated with $_FILES array

Yes, but as I mentioned above the reference to $_FILES becomes indirect, so we can extend CUploadedFile class and have, for example, TestUploadedFile which takes it's source file not from php temp folder, but from a folder with test files.
I can not say that I feel that CUploadedFile in the model is absolutely correct, but I see no valuable disadvantages and think it is OK.

jacmoe, on 08 February 2012 - 02:15 PM, said:

I prefer to deal with CUploadedFile in the controller. It definitely belongs in the http logic department. Agree with Andy.
The model should be able to deal with any file, not only uploaded files.

I think that it is better to keep code which saves uploaded file in the model, because controller should now know where and how to save uploaded file. Actually I usually use a bit more complex approach then I described above.
I have a db table 'files' with files metadata (user name, system path, size, etc) and a model File. All direct file operations (file system level functions like create, copy, move file) are performed only through the File model.
So file upload process is:

• controller creates CUploadedFile instance and passes in to the model (for example, Post)
  
• Post model has 'file' relation (File model) knows where to save file, creates new File instance, configures it and calls $file->upload($uploadedFile)
  
• File model performs real file operation (moves temp file to apermanent storage).

This way code in controllers and models to handle files becomes 'slim' and the main file handling logic is in the 'fat' File model. Also with such approach it is easy to replace file storage (for example, switch from a local file system to Amazon S3) - I need to modify only File model to do this.

[jacmoe]

Passing a CUploadedFile instance is acceptable. I think.
A CUploadedFile doesn't make much sense outside of a submitted form, though. Which ties it completely to the controller.
It just feels wrong to me to let the model handle it.
So for brevity, I would just put the logic in a component instead. Like a component wrapping a component.

[andy_s]

Actually I use a more complex approach too. I have a CActiveRecordBehavior which handles file/image uploads in it's "afterSave" and "afterDelete" methods, also it provides methods like "getFileUrl" and "getFilePath". Very handy I think, and it looks like "a component wrapping a component", but it also uses CUploadedFile class internally. Thanks seb and jacmoe for sharing your thoughts, I don't feel alone anymore with this issue