Model Design question

[solleer]

@TomBZombie In a discussion at https://www.sitepoint.com/community/t/mvc-refactor-take-2/194004/8 you mention having a success method in the model because it allows the rest to be reused. In another question you said that FormModels should only be to connect the domain model with the GUI and should have no business logic. What is the point then of separating out the success method if the FormModel just connects two things and has no main logic to reuse.
I have an interface that I implement

interface Form {
    public function main($data);
    public function submit($data);
    public function success();
}

and I am starting to question the point of the success method if really all I do in submit is call a method on the model that the FormModel was given.

[TRPB]

Essentially it gives more flexibility. For example, submit() might insert some data and success() might trigger an email response or do some kind of logging. By making this action part of the success() method there's no way in this scenario to use the data-insertion logic without also triggering the email send, which might be desirable behaviour.

Having said that, a better way to solve that might be:

interface Form {
    public function main($data);
    public function submit($data, $callback = null);
}

[solleer]

One place I have followed this is at https://github.com/solleer/User-Module. I feel like my form models are doing to much and that I am getting to much of a god class with the User class.

[garrettw]

I am getting to much of a god class with the User class.

I would agree. One way to tell is to notice how many parameters your constructor takes. It's not always a sure way to tell, but it's usually a good hint.

I would think a single instance of User should represent a single user and nothing more. If that were the case, methods like getUsers() wouldn't seem to belong in that class.

[TRPB]

I do wonder what the function 'validatePasswordConfirm` is doing in the class.

As Robert C. Martin says: A class should have a single reason to change. This class has multiple reasons. Another good test is Misko Hevery's line "Describe the class job without using the word AND". I am struggling to come up with a description of this class that encompasses all the methods and doesn't use the word AND.

There seem to be two different concerns here:

1. Interacting with the ORM
2. "Current user".

I'd separate these into two classes and I'm not sure why you need to use Respect\Validator just to check two strings are equal. Seems like an unnecessary dependency to me.

The line

$this->security->hashSecurityProperties($data);

Doesn't sit well with me. This function knows a lot about $data, Why isn't it just $this->security->hash($data->password) or similar?

[solleer]

I have now updated it to remove the validatePasswordConfirm function. I also removed the create function. Both are really just implementation specific and have nothing to do with storing or getting the user so they are now just in the form models that used them.

About the current user, I have thought about extending the class also into a CurrentUser subclass and extending would be good since it could be used anyplace a User could be used and then the getCurrentUser and updateCurrentUser functions would just override the getUser and save functions since getCurrentUser and updateCurrentUser are really just wrappers for getUser and save. The main problem was that it seemed that everywhere that would use the regular User class would also need the CurrentUser class. I'm going to look it all over again and probably make the CurrentUser subclass.

[solleer]

The places I would need both the User class and the CurrentUser subclass are the Credentials class and Authorize class. I think the reason in Credentials seems to be a break in SRP.

[solleer]

The other issue with factoring out the current user stuff is that the save function uses it here

if ($this->getUser($data->username) !== false && $data->username !== $this->getCurrentUser()->username) return false;

[garrettw]

I don't understand why a CurrentUser class needs to exist at all. Couldn't you just have a property like $this->currentUser which is just an instance of the User class that represents the current user?

Or better yet, the property you store could even be $this->currentUsername, and then the method $this->getCurrentUser() could then just be shorthand for $this->getUser($this->currentUsername);

Of course, that doesn't address whether all those properties and methods actually belong in the same class..

[solleer]

I do store the user_id and getCurrentUser is shorthand for what you said. The question is whether those current user methods belong in the user class since they have nothing to do with a regular user.

[solleer]

One problem is that is CurrentUser extends User, then if on a separate project I want to extend User to change something for that project then I would have to redefine a CurrentUser object. So I think that composing the User object would be best for the CurrentUser to do.

[TRPB]

Generally speaking extends is evil. Don't use it, use composition instead.

[solleer]

I have now factored the Current user out in solleer/Basic-User-Module@f3b62a8. I'm thinking about making an interface for User or something like that.

[TRPB]

I'd refactor authorize.php. see http://misko.hevery.com/code-reviewers-guide/flaw-digging-into-collaborators/

You'd be better off just passing the user instance into the class:

<?php
namespace User\Model;
class Authorize {
    private $model;
    private $currentUser;
    private $functions = [];
    private $defaultFunctions = [
        "user" => "\\User\\Model\\Authorize\\User"
    ];
    private $id = false;
    public function __construct(User $model, $currentUser) {
        $this->model = $model;
        $this->currentUser = $currentUser;
        foreach ($this->defaultFunctions as $key => $function) $this->addFunction($key, new $function);
    }
    public function __call($name, $args) {
		if (isset($this->functions[$name])) {
            if ($this->id) $user = $this->model->getUser($this->id);
            else $user = $this->currentUser;
			$result = $this->functions[$name]->authorize($user, $args);
            $this->id = false;
            return $result;
		}
		throw new \Exception("Method \\User\\Model\\Authorize::" . $name . " does not exist");
	}
    public function id($id) {
        $this->id = $id;
        return $this;
    }
    public function addFunction($name, Authorizable $function) {
		$this->functions[$name] = $function;
	}
}

[solleer]

Thanks, I made the change and updated the DI config. I'm trying to come up with a good way to make this system easily portable and customizable to any needs. I am working on an extension of this Module at https://github.com/solleer/Office365-User-Module in order to see how easily extensible the set up it to any needs. The problem I see is that I can't find a good way to do it without extending the User object. I will upload some code to the extension module and comment here when I do.

[solleer]

I have added a bit of code to https://github.com/solleer/Office365-User-Module. I can't figure out how to make this easily extendable.

[solleer]

@TomBZombie I am also questioning the need of the User\Model\Signin class. Should the logic in their just be put into the form models or should it stay in a separate class. I also implemented the model in the Office365 extension and it feels like I have too many things happening.

[solleer]

Also, should this function in Credentials move to the CurrentUser class

public function checkCurrentUserSecurity($username, $password) {
    $id = $this->validateUserCredential($username, $password);

    if ($id === false || $id !== $status->getSigninVar()) return false;
    return true;
}

This function just feels like it doesn't truly belong in any of the current classes.

[solleer]

The above function seems to best fit where it is or in the CurrentUser class but it doesn't seem to fit in either of those either.

[solleer]

@TomBZombie Where do you think the checkCurrentUserSecurity should go. I think I've decided that it definitely will not go in current user because that would make CurrentUser too implementation specific.

[solleer]

@TomBZombie Also I have an issue with https://github.com/solleer/Events-Module in RepeatingEvents and Events. This code is repeated in both

usort($events, function ($event1, $event2) {
      $date1 = new \DateTime($event1->start_date);
      $date2 = new \DateTime($event2->start_date);
      if ($date1 === $date2) return 0;
      else return ($date1 < $date2) ? -1 : 1;
});

I thought about factoring it out but that would require a class to be passed to both classes that only is needed for the getUpcomingEvents method.

[solleer]

Also should the class CurrentUser implement User or stay the way it is.
They are located at https://github.com/solleer/User

[solleer]

@TomBZombie Should the class CurrentUser implement User or stay the way it is.