PHP advice please. (Kohana experience a plus)

Question

I've been working with PHP for about a year, but I do it as a hobby. I dont have anybody I can go to as a teacher or a mentor to give me advice on what I may be doing completely wrong, or what I could do better. I've done quite a few different things within that year, so I wouldnt consider myself a complete noob.

Anyways, I have just started using a framework (Kohana), and there really arent that many tutorials out there, so I'm not entirely sure if I'm doing things in a good way.

I have a few code snippets that I would like to post to get some feedback pertaining to what I just said.

For Starters

User Controller

class User_Controller extends Template_Controller{

    public function register()
{
    // logged in users cant register
    if($this->logged_in)
    {
        url::redirect('user/profile');
    }

    // initially show an empty form
    $form = $errors = array
    (
        'username'      => '',
        'email'         => '',
        'password'      => '',
        'gender'        => '',
        'dob_month'     => '',
        'dob_day'       => '',
        'dob_year'      => '',
        'date_of_birth' => '',
        'captcha'       => '',
        'registration'  => ''
    );

    // check for a form submission
    if($this->input->post('register'))
    {
        // get the form
        $post = $this->input->post();

        // prepare the data for validation
        $post['date_of_birth'] = "{$post['dob_year']}-{$post['dob_month']}-{$post['dob_day']}";

        // create a new user
        $user = ORM::factory('user');

        // validate and register the user. 
        if($user->register($post, TRUE))
        {
            // SEND EMAIL

            // login using the collected data
            if(Auth::instance()->login($post->username, $post->password, TRUE))
            {
                // redirect the user to the profile page
                //url::redirect("user/profile/{$user->id}");
            }
        }

        // get validation errors and repopulate the form
        $form   = arr::overwrite($form,   $post->as_array());
        $errors = arr::overwrite($errors, $post->errors('registration_errors'));
    }

    // template variables
    $this->template->title = 'Sign Up';
    $this->template->body  = new View('layout_1');

    // layout variables
    $this->template->body->left  = new View('user/registration_form');
    $this->template->body->right = 'Right Side Content';

    // registration form variables
    $this->template->body->left->form    = $form;
    $this->template->body->left->errors  = $errors;
    $this->template->body->left->captcha = new Captcha('register');
}
}

Register Function within User_Model

class User_Model extends ORM{

    public function register(array& $user, $save = FALSE)
{
    $user = new Validation($user);

    // logged in users cant register
    if(Auth::instance()->logged_in())
    {
        $user->add_error('registration', 'logged_in');
        return FALSE;
    }

    // trim everything
    $user->pre_filter('trim')

        // everything is required
        ->add_rules('*', 'required')

        // username must be 5 - 30 alphanumeric characters and available
        ->add_rules('username', 'length[5,30]', 'valid::alpha_numeric', array($this, 'username_available'))

        // email must be valid format and available
        ->add_rules('email', 'valid::email', array($this, 'email_available'))

        // password must be 5 - 15 characters and alpha dash
        ->add_rules('password', 'length[5,15]', 'valid::alpha_dash')

        // gender must be either male or female. capitalize first letter
        ->add_rules('gender', array($this, 'valid_gender'))
        ->post_filter('ucfirst', 'gender')

        // dob must be a valid date, and user must be old enough.
        ->add_callbacks('date_of_birth', array($this, 'check_dob'))

        // captcha must be entered correctly.
        ->add_rules('captcha', 'Captcha::valid');

    // add the registration date
    $this->registration_date = date::unix2mysql();  // helper function transforms the current unix to mysql datetime format

    // validate the information. an ORM function.
    $result = parent::validate($user, $save);

    // was the user info valid?
    if($result === TRUE)
    {
        // was the user saved?
        if($save === TRUE)
        {
            // add a login role
            $this->add(ORM::factory('role', 'login'));
            $this->save();
        }
    }
    else
    {
        $user->add_error('registration', 'failed');
    }

    return $result;
}
}

Mostly all my models follow the same format when validating info.

I have some other things I would appreciate feedback on as well, but I dont want to overwhelm anybody.

Thanks a lot for your time

EDIT: I'm sorry, I should've posted both the user controller and model. I've been reading alot about how models should be fat, and controllers should be skinny. Thats why I created a register function in the model to validate the info instead of doing so within the controller. The register function takes an array, but turns that array into a validation object so that I can retrieve the user input, and the errors. I've seen a few tutorials on Kohana where it was done this way.

Answer 1

I dont know Kohanna so im not sure what the lay of the land is on their MVC separation but typically i would make register an action on a controller. The main thing i disagee with in your code is that the Model is coupled to the Authentication system internally. The authentication check should be made outside the class and the control flow decision should be made outside as well, OR the result of the authentication check should be passed in to the Model for use in its internal operation.

Typically i might do something like the following pseudo code:

// in my controller class for User or whatever

public function registerAction()
{
   // get the form data from the request if its POST, ortherwise a blank array
   $userData = $this->getRequest('user', array(), 'POST');

   // create a user
   $user = new User($userData);

   if(Auth::instance()->logged_in())
   {
      // we are logged in add an error to the user object for use by the view
      $user->getValidator()->add_error('registration', 'logged_in');
   }
   elseif($user->is_valid())
   {
     // user data is valid, set the view with the success message
     $user->save();
     $this->setView('register_success');
   }

   /**
    * render the designated view, by default this would be the one containing the
    * registration form which displays errors if they exist - however if we success-
    * fully registered then the view with the success message we set above will be
    * displayed.
    */
   $this->render();
}

Answer 2

First, I would not put the register() method into the User model. A model should be a representation of the object in the database and generally only contains your "CRUD" methods (create, retrieve, update, delete), getter and setter methods, and maybe some static helper methods related to the model. By putting your register() method into the model, you're making the model do presentation logic that should really be done by a User controller, since this is a response to a user action. A controller handles user actions, validates those user actions, then updates the model if the validation is successful.

In your example, the user is attempting to create a new account. He fills out a form and clicks submit. The form's POST action should point to a controller's method, like /user/register, and that method will use the Validation library to validate the form data sent by the user. Only if that data validates successfully should you create a User model instance, set the properties of that model to what the user input, and then use the model's save() method to save to the database. If the validation fails, you report the error to the user and you don't create a User model at all since you don't have a valid data set to create a model with yet.

Next, you are checking to see if the user is logged in. Again, this should be in the controller, not the model. Besides that, the user should not be able to get to this register process in the first place if he is already logged in. The controller method that creates the user registration form view should check to see if the user's logged in, and if he is, then he should be redirected to another page. Even if the user is playing tricks and manages to submit the form (maybe he logged in via another window while having the form open in an old window), your register method should check for that first and not create a $user Validation object yet.

I can see in your code that there are some confusing items based on your model set up. For example, you're passing the $user array into the method, which I presume is the form data. But you're using the "pass by reference" operator (&) which is unnecessary in PHP5 since all objects are now passed by reference. But after that you're recasting $user as a Validation object. Are you using the $user Validation object elsewhere and require it to be passed by reference? If so, that's another flaw in the logic as all of this processing needs to be in the controller and the $_POST values can be used directly in the controller instead of having to pass around a Validation object.

Later on, you're validating the user information with parent::validate($user, $save). Why is the validate() method being called on parent as a static method? If this is a model, it should be extending Kohana's core Model class, and "parent" references the Model class. Is your model extending the Validation class? Also, why are you passing in the $user Validation object to the validation() method? Doing that is required if you need to do recursion (to validate elements again after making changes from previous filters), but it looks like you're not doing anything to require recursion. You should be calling validate() on the $user Validation object:

$user->validate();

without any arguments. The validation errors will become part of the $user object, so you can check for errors using

$user->errors();

Finally, while Kohana allows you to use method chaining, I would not use one long chain to set up the rules and other items for the validation. It's confusing and may cause debugging to be difficult. Put each of those on its own line and perform each directly on the $user object.