1
\$\begingroup\$

Model:

class Task extends Model {
}

with fields

protected $fillable = ['message', 'due_time', 'status'];

I've added special functions public function getEditableStateForAttributeName().

In my helper function I check that if method_exists($class, 'getEditableStateForFieldName'), then I check the boolean value returned by this method to decide whether to allow editing this field.

Example:

if( ! $cls->getEditableStateForDueTime() ) {
    return "You can not edit field due time";
}

Real code:

private function isCreator() {
     $user = Auth::user();
     if($user) {
         return $user->id === $this->creator_id;
     }
     return false;
}   

public function getEditableStateForMessage() {
     return $this->isCreator();
 }

public function getEditableStateForDueTime() {
     return $this->isCreator();
}

public function getEditableStateForStatus() {
    return $this->isCreator() && $this->status === 'CREATED';
}

Is this a good way to do it or it is very bad design because of hidden dependency on Auth::user()?

What is a better way?

I do not want to put this logic inside controllers because this logic propagates to another models and is universal across application.

I've written some helper functions which facilitates validation of my models

Inside controller I simply write

createValidatorForModel(Task::class)

and it returns new validator with custom errors/custom attributes names/additional checks/etc.

\$\endgroup\$
  • \$\begingroup\$ It's working code. Part of it. I agree that question is mostly about design, not code review. \$\endgroup\$ – changer Feb 14 '17 at 21:32
0
\$\begingroup\$

Just like you say, you are hiding dependencies if you have new or call a non-pure function with the x::y($z) syntax.

One way to go around this, is to acknowledge the dependency and take it as a constructor argument, so that the current object doesn't care where it gets its user from, but it knows that it can call isCreator() or such. Some would advice you to take dependencies through setUser(User $u)-like methods, but that would make the object useless between its constructor is called, and until the setUser() method is called, because getEditableStateForMessage() and friends will fail in the meantime.

You should consider attacking problems like this from a TDD point of view: how do I test the current code without having to resort to real data?

| improve this answer | |
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.