Controller <-> Model: Wohin mit der Logik?

[Leichti]

Hallo,

Nachdem mir empfohlen wurde mich in ein PHP Framework einzuarbeiten, bin ich gerade dabei mit Laravel herumzuspielen. Nun habe ich eine einfache Registrierung programmiert, wobei ein "UsersController" mit der Methode "store" das speichern übernimmt.
Dieser sieht folgendermaßen aus:

class UsersController extends \BaseController {

//...
//...

    public function store()
    {
        $input = Input::all();

        if ( ! $this->user->fill($input)->isValid()) 
        {
            return Redirect::back()->withInput()->withErrors($this->user->errors);
        }

        $this->user->save();

        return Redirect::route('users.index');
    } 

Das User-Model enthält die Validation:

class User extends Eloquent implements UserInterface, RemindableInterface {

    public function isValid()
    {
        $validation = Validator::make($this->attributes, static::$rules);
        $this->hashPassword();

        if($validation->passes()) return true;

        
        $this->errors = $validation->errors();

        return false;
    }

    public function hashPassword()
    {
        // TODO Prevent from double hashing! 
        $this->attributes["password"] = Hash::make($this->attributes["password"]);
    } 

Mein Problem nun:
Wenn ein User sein Profil editiert, das Passwort dabei aber nicht ändert liegt das verhashte Passwort in $this->attributes["password"], wird also nocheinmal verhashet. Hat jemand eine Idee wie ich das elegant umgehen kann? Mein erster Gedanke war wenn die Länge > 20 ist (gleichzeitig Länge des Passworts auf 20 begrenzen) einfach zu sagen es ist schon ein Hash und nicht mehr verhashen. Aber das ist alles andere als sauber.

Sollte ich die Validation überhaupt woanders hinpacken? Ich habe zumindest gelesen, dass der Controller möglichst wenig Logik enthalten soll.

Ich bin für jeden Tipp dankbar!

lg
Manuel

[rkr]

Bei mir ist in diesem speziellen Fall das Passwort nie Teil des User-Models.
Passwort-Handling mache ich dann gesondert.

[G.Schuster]

Zum Einen hat das Hashing in der Validierung absolut nix verloren, zum Anderen halte ich von umgehbarer Validierung sowieso nichts, da somit die Möglichkeit besteht, ein invalides Model zu haben, ohne es zu merken.
Für dein konkretes Problem: hashen und validieren beim Setzen der Daten, setzen eben auch nur, wenn vorhanden/geändert.

[Leichti]

Bei mir ist in diesem speziellen Fall das Passwort nie Teil des User-Models.
Passwort-Handling mache ich dann gesondert.

Verstehe nicht ganz wie das gemeint ist. Welcher Vorteil ergibt sich wenn das Passwort nicht Teil des User-Models ist?

Zum Einen hat das Hashing in der Validierung absolut nix verloren, zum Anderen halte ich von umgehbarer Validierung sowieso nichts, da somit die Möglichkeit besteht, ein invalides Model zu haben, ohne es zu merken.
Für dein konkretes Problem: hashen und validieren beim Setzen der Daten, setzen eben auch nur, wenn vorhanden/geändert.

Edit: glaube doch dass ich verstehe was du meinst

Ist es so besser?

// Model User
public function fillAndCheck($input)
    {
        $validation = Validator::make($input, static::$rules);

        if($validation->passes()) 
        { 
            $this->fill($input)
            return true;
        }
        
        $this->errors = $validation->errors();

        return false;
    } 

Wobei es dann möglich wäre über eine magische "setPasswordAttribute" (Mutator) das Passwort automatisch zu hashen:

// Model User
    public function setPasswordAttribute($password)
    {
        $this->attributes["password"] = Hash::make($password);
    } 

Hierbei würde der Code im Controller sich nur unwesentlich ändern

    public function store()
    {
        $input = Input::all();

        if ( ! $this->user->fillAndCheck($input)) 
        {
            return Redirect::back()->withInput()->withErrors($this->user->errors);
        }

        $this->user->save();

        return Redirect::route('users.index');
    } 

Gefällt mir eigentlich schon viel besser

[jack88]

Gefällt mir eigentlich schon viel besser

Man kann es sicherlich so machen, aber schön finde ich es nicht.

Wieso schmeißt Du checkAndFill() nicht komplett aus dem Modell raus und validierst stattdessen außerhalb? Auf die User-Properties greifst Du dann ausschließlich über Setter/Getter zu. Die Setter sind dabei so abgesichert, daß bei ungültigen Werten eine z.B. InvalidArgumentException geworfen wird. Wenn Du mehrere Werte auf einmal einfügen willst, dann könntest Du dem Modell entweder eine setByArray($properties) spendieren (die intern das Modell über Setter befüllt!) oder (vielleicht besser) einen UserArrayMapper::update($user, $properties). So ist die Konsistenz des Modells immer gewährleistet und das Modell wird nicht als Validator zweckentfremdet.

Vg
jack

[Leichti]

Wieso schmeißt Du checkAndFill() nicht komplett aus dem Modell raus und validierst stattdessen außerhalb?

Das ist natürlich die Frage. WO wäre die Validierung der Daten deiner Meinung nach am Besten aufgehoben? Warum nicht dem Model überlassen? Ich finde es eigentlich gut wenn das Model direkt weis was es erlaubt und was nicht, die Validierung selbst geschieht ja in einer eigenen Klasse!

[nikosch]

Die Validierung ist Teil des Eingabeprozesses, also der Komponente, die Form-View, Form-Action etc. abhandelt. Ich würde die Validierung konfigurierbar gestalten und ggf. diese Konfiguration verbindlich für Validierung und Datenbankstruktur machen.

[G.Schuster]

Die Setter sind dabei so abgesichert, daß bei ungültigen Werten eine z.B. InvalidArgumentException geworfen wird.

Und das ist keine Validierung?

In meinen Augen hat eine Validierung immer dort stattzufinden, wo anschließend (auf "normalem" Weg) keine unvalidierte Änderung mehr stattfinden kann, andernfalls macht es keinen Sinn, sich überhaupt Gedanken über Datenkonsistenz zu machen.
Ein von mir entwickeltes Model-Konzept setzt dazu eben auf Settern auf, wobei die Validatoren beim Init des Models angelegt werden und damit auch für eine "Array-Befüllung" problemlos zur Verfügung stehen.
Nebenbei spare ich mir damit Code-Duplication: egal woher Daten kommen, ich pack sie als Array-Param in den Constructor-Aufruf und kann sicher sein, dass alles korrekt ist.
Das schließt auch z.B. "Pflichtfelder" ein, die bei einer externen Validierung niemals zuverlässig sein können.

[nikosch]

Das Problem ist die Abhängigkeitvon der Validator-Klasse.

[G.Schuster]

Ich seh da kein Problem.
Für mich zählen valide Daten durchaus auch zur Geschäftslogik und für die Erfüllung von Zusicherungen (=definierter Zustand der Models) nehme ich Design-Kritik gern in Kauf.

[Leichti]

Jetzt bin ich etwas ausgestiegen.

Die Validierung ist Teil des Eingabeprozesses, also der Komponente, die Form-View, Form-Action etc. abhandelt. Ich würde die Validierung konfigurierbar gestalten und ggf. diese Konfiguration verbindlich für Validierung und Datenbankstruktur machen.

Form-View und Form-Action wird ja vom Controller (in Laravel, In PHP Design Patterns heißen die glaubich "Commands") abgehandelt, oder verstehe ich das falsch?

In meinen Augen hat eine Validierung immer dort stattzufinden, wo anschließend (auf "normalem" Weg) keine unvalidierte Änderung mehr stattfinden kann, andernfalls macht es keinen Sinn, sich überhaupt Gedanken über Datenkonsistenz zu machen.

Was verstehst du unter "keine unvalidierte Änderung"?

Vielen Dank!!

[rkr]

Ich seh da kein Problem.
Für mich zählen valide Daten durchaus auch zur Geschäftslogik und für die Erfüllung von Zusicherungen (=definierter Zustand der Models) nehme ich Design-Kritik gern in Kauf.

Daten und Datenvalidität sind zwei unterschiedliche Concerns und sollten auch getrennt behandelt werden (können).
Zum Thema: Was spricht denn gegen meinen Vorschlag?

[Leichti]

Daten und Datenvalidität sind zwei unterschiedliche Concerns und sollten auch getrennt behandelt werden (können).
Zum Thema: Was spricht denn gegen meinen Vorschlag?

Ich sehe den Vorteil nicht welcher sich ergibt wenn das Passwort außerhalb des Models gehandelt wird. Aber ich freue mich über jeden Input, eure Tipps helfen mir sehr!

[rkr]

Weil das eine Modell-Eigenschaft ist, die nicht zu den anderen passt - bzw. ein special treatment benötigt. Welcher Nachteil ergibt sich denn das Passwort gesondert zu handhaben?

Bitte beachte zudem, dass das Passwort eigentlich keine richtige User-Eigenschaft ist.
Es ist ein Zugangsschlüssel. Wie beispielsweise ein SingleSignOn-Token oder ein LDAP-DN. Muss nicht mal zwangsläufig in der gleichen Tabelle gespeichert werden. Nicht mal in der gleichen Datenbank. Nicht mal auf dem gleichen Server.

[G.Schuster]

Was verstehst du unter "keine unvalidierte Änderung"?

Setter ohne Validierung bzw. public properties - da kann ich dann lustig reinschieben, was ich möchte.
Über den Umweg mit Reflections wäre das zwar prinzipiell immer möglich, aber das wäre kein Grund, die anderen einfachen Möglichkeiten zu unterbinden.

Im Übrigen sind zwei Validierungen per se nichts Schlechtes, der Kontext einer Form-Validierung ist immerhin ein entscheidend anderer als der zur Sicherstellung von Datenintegrität.
Bei einem Formular komtm es z.B. darauf an, ob ein Geburtsdatum einem gewissen Alter entspricht, für das Model ist dagegen eher entscheidend, dass es ein valides Datum ist.
Oder ein Formular erfasst Daten im Zusammenhang, die auf mehrere Models abgebildet werden - dennoch muss jedes Model in meinen Augen sicherstellen, dass der Datenteil, den es beherbergt, den Regeln entspricht.
Ich glaube, das kann man jahrelang diskutieren, von daher klink ich mich an der Stelle aus.