ap1234's avatar

Actions Vs Services?

Hey! With recent videos I have watched there is this craze about Actions/Command Pattern. I wanted to try this out in one of my project (Larave + Livewire). I am not sure if I don't this the correct way. Below are two of my action classes. CreateUser and UpdateUser, 80% of my code is the same. Actions highly implements the SOLID principles. Now here if I add an if else statement it breaks the SOLID principles. I want to know if what I am doing is correct? Is there any way I can optimize this? Thank you in advance!

0 likes
19 replies
Glukinho's avatar

Your rules are the same in both classes, so the DRY principle is violated. Validation rules should be in one place, maybe adjusted for create/update case, but not fully duplicated.

You pass a simple array of data, maybe it's time to utilize DTO pattern and typed data objects?

Use spatie/laravel-data package and set your fields and validation rules there. It will give you flexible validation and many other useful things across all your app. https://spatie.be/docs/laravel-data/v4/validation/introduction

Tray2's avatar

And don't forget to never ever store passwords in plain text.

However, my take on using Repository classes to talk to the DB, or even using the model to store methods for doing things with the data in the table, is to not do it. I would use the ORM provided in Laravel, and place the code in single action controllers.

The only reason I see for breaking that code out from the controller is if you need to use the exact same code in multiple places, and then my approach is not to do it until you really need to.

There are of course other things that play a huge part in how you code, if you are a single developer, then you can do as you please, if you on the other hand work for a big company or contributing to open source project, then you need to adapt to their code standard.

ap1234's avatar

@Tray2 Yes, i don't save passwords in plain text, i have casted it in my model using hashed. Thank you!

Yes, I am a single developer, I am just confused on what I am doing is correct. Should the array be inside the action class or should it be modified from the outside? And about the duplicate code, should I extract common logic and then use this? And if i use any conditions it will violate SOLID Principles.

Tray2's avatar

@ap1234 While SOLID is a pretty good idea, I still aim for KISS first :)

2 likes
Snapey's avatar

Personally, I would not be concerned to have branching inside the action.

Personally, I would NOT have validation in my action, although this is what the Fortify starter kit does.

ap1234's avatar

@Snapey Thank you for you reply! Yes, I am using fortify in my project. I am just confused on what I am doing is correct.

  1. There is duplicate code, how should that be managed?
  2. Should the array I am passing be modified inside the action or it should be modified from the outside.
  3. If I modify the code , say add conditions for create and update it violate SOLID Principle.
  4. Like you said validation should be FormRequest or something else but not in the action, where should that be and should that be same validation for Create and Update?

I have this ecommerce project. I am using construct to stack actions which works great but the major concern is duplicate code. I have the same Create Update for customers, products, masters etc.

Sorry for silly questions.

ap1234's avatar

@Snapey By branching inside the action, you mean it should have just one function, nothing else right? So you would validate it outside before it hits the action. Correct?

Glukinho's avatar

@ap1234 Make your classes to do only one thing.

Your CreateNewUser action class should be responsible for only one action: creating a user. Validation should be handled in some other place (for example, in DTO object or Form Request class). And action class should get already validated and trusted data, without need to validate it itself.

In simple words, the flow can be like this:

  • a user submits a form from webpage
  • submitted data goes to a route and is sent to a controller
  • in controller:
    • the DTO object is created from input data
    • DTO object validates the data internally. So, when DTO object is created, you are assured it contains validated data and you can use it in any part of the app. This is DTO's purpose.
    • action class is called and DTO is passed there; it performs an action, maybe returning some result.
    • a controller sends a response to a request, based on action's result.

In this flow, every part performs only what it is responsible for:

  • a route routes a request to appropriate controller,
  • a controller receives input data, collects it into DTO object, executes an action and sends back a response,
  • a DTO object validates input data and represents the data as solid unchangeable structure,
  • an action class performs an action.

should that be same validation for Create and Update

It's up to you and your app. Basically they are the same, but there may be differences.

Glukinho's avatar
'active' => ['required', Rule::in(['1', '0'])],

boolean rule is better, it covers all cases which may be considered true/false depending on where the data came from: true, false, ,"true", "false", 1, 0, "1", "0", "yes", "no"

'email' => ['required', 'max:255', 'email:rfc,dns', Rule::unique(User::class, 'email')->ignore($user)],

Use DNS validation check with caution, it executes a network request on every validation which can bring unpredictable delays to your app. I would use regexp only to validate email addresses.

'mobile' => ['required', 'numeric', 'digits:10', Rule::unique(User::class, 'mobile')->ignore($user)],

It is incorrect to validate a phone number as numeric, it can contain plus sign (+), parentheses, whitespaces, dashes, pound signs and even more non-numeric symbols. Treat phone number as a string, not a number, this is about database column too (VARCHAR, not INT). Better cast it using propaganistas/laravel-phone package: https://github.com/Propaganistas/Laravel-Phone?tab=readme-ov-file#attribute-casting

martinbean's avatar

@ap1234 Service classes are classes that have methods for interacting with a particular thing, so you might have a UserService class with methods for creating, updating, and deleting users.

Action classes are just self-handling commands that Spatie gave a new name, every one fell head over heels about them as if it were some novel new concept, and basically extracts each individual method that would have been in a service class, to its own class.

There doesn’t seem to be an agreed convention of whether validation should be done inside or outside of a service class method/action class, but my personal opinion is you should only be passing pre-validated data to a service/action, and that should be in a strongly-typed parameter class and not something like a “dumb” array that could hold any values, or even none at all, and led to errors when trying to access non-existent keys from that array, or trying to use data that isn’t of the type you expect (i.e. an email key holding a boolean value instead of a string, or the email just not being a well-formatted email address at all). This makes it impossible to instantiation the action or call the method with “wrong” or “bad” data as it’s enforced by the parameters class:

use Illuminate\Contracts\Hashing\Hasher;

class CreateUserAction
{
    public function __construct(
        protected Hasher $hasher,
    ) {}

    public function __invoke(CreateUserParameters $parameters): User
    {
        return User::query()->create([
            'name' => $parameters->name,
            'email' => $parameters->email,
            'password' => $this->hasher->hash($parameters->password),
        ]);
    }
}

The CreateUserParameters class can then only be instantiated with valid data:

readonly class CreateUserParameters
{
    public function __construct(
        public string $name,
        public string $email,
        public string $password,
    ) {
        if (! filter_var($email, FILTER_VALIDATE_EMAIL)) {
            throw new InvalidArgumentException(sprintf('Email [%s] is invalid.', $this->email));
        }
    }
}

So it’s now completely impossible to invoke your “create user” action with invalid parameters, and in turn it’s also impossible to create your parameters class with bad data such as an invalid email address.

You could even take the above one step further by making the email a value object, which again can only be instantiated with a valid value:

readonly class EmailAddress
{
    public function __construct(
        public string $value,
    ) {
        if (! filter_var($this->value, FILTER_VALIDATE_EMAIL)) {
            throw new InvalidArgumentException(
                sprintf('Value [%s] is not a valid email address.', $this->value),
            );
        }
    }
}

If you have an instance of this EmailAddress value object, then you know its value is correct and well-formed; it would be impossible to instantiate it otherwise.

1 like
ap1234's avatar

Here is what I did, Please guide me if I am doing this correctly.

Livewire component:

    /*
    * Submit user form
    */
    public function submitForm(CreateUser $createUser, UpdateUser $updateUser): void
    {
        $validated = $this->validate(new UserRequest()->rules($this->user));

        if ($this->user instanceof User) {
            $updateUser->update($this->user, $validated);
            session()->flash('success', 'User Updated Successfully!');
        } else {
            $createUser->create($validated);
            session()->flash('success', 'User Created Successfully!');
        }

        $this->redirectRoute('users.index');
    }
  1. Validation for both update and create
final class UserRequest extends FormRequest
{
    /**
     * Get the validation rules that apply to the request.
     *
     * @return array<string, \Illuminate\Contracts\Validation\ValidationRule|array<mixed>|string>
     */
    public function rules(?User $user = null): array
    {
        return [
            'name' => ['required', 'min:3', 'max:255'],
            'active' => ['required', Rule::in(['1', '0'])],
            'password' => ['required', 'max:255', Password::defaults()],
            'email' => ['required', 'max:255', 'email:rfc,dns', Rule::unique(User::class, 'email')->ignore($user)],
            'mobile' => ['required', 'numeric', 'digits:10', Rule::unique(User::class, 'mobile')->ignore($user)],
            'role' => ['required', Rule::exists(Role::class, 'name')],
        ];
    }
}
  1. Update User Action and Create User Action
final readonly class CreateUser
{
    /**
     * Create action.
     */
    public function create(array $input): User
    {
        $user = User::query()->create([
            'name' => $input['name'],
            'is_active' => $input['active'],
            'password' => $input['password'],
            'email' => $input['email'],
            'mobile' => $input['mobile'],
        ]);

        $user->syncRoles($input['role']);

        return $user;
    }
}

final readonly class UpdateUser
{
    /**
     * Update action.
     */
    public function update(User $user, array $input): User
    {
        $data = [
            'name' => $input['name'],
            'is_active' => $input['active'],
            'email' => $input['email'],
            'mobile' => $input['mobile'],
        ];

        $user->update($data);
        $user->syncRoles($input['role']);

        return $user;
    }
}

Is this flow correct? Should I change anything? Also I have another question, Let's say in the future I just want to update the email of the user then this action would not work as this will need the entire array with the keys. Then what should I do?

Thank you so much for you effort! You have no idea how much this helps!

@glukinho @martinbean

Glukinho's avatar

@ap1234 this seems bad to me:

public function submitForm(CreateUser $createUser, UpdateUser $updateUser): void
{

A frontend component should call different endpoints:

public function createUser($data) { } // in case of you want to create new user;
public function updateUser(User $user, $data) { } // in case of you want to update existing user;
public function deleteUser(User $user) { } // 
public function blockUser(User $user) { } // etc... in other cases.

Validation for both update and create

Your validation rule set always expects all data including password. But what if a user wants to change only email address in his profile?

I would say some fields shouldn't be required in particular cases. Some useful thoughts here: https://laracasts.com/discuss/channels/general-discussion/l5-validate-request-rules-for-both-create-and-update

Let's say in the future I just want to update the email of the user then this action would not work as this will need the entire array with the keys. Then what should I do?

Updating user's email is just a special case of updating a user. So, just call the same action:

$user = User::find(123); // some user just for example

(new UpdateUser)->update($user, [
		'email' => '[email protected]',
]);

As long as your 'update' rule set doesn't require you to provide all fields, you can update any field one at a time.

I think you should just begin to build your app, the practice itself will tell you what is wrong and what is right. Just don't be afraid of rethinking and rewriting "bad" parts.

martinbean's avatar

@ap1234 I didn’t get past the first line of code I’m afraid:

public function submitForm(CreateUser $createUser, UpdateUser $updateUser): void

Why on earth are you injecting both the “create” and “update” user actions in a single method? You should have separate forms for these as they’re two completely different use cases.

This just smells of trying to “DRY” code unnecessarily.

newbie360's avatar

@ap1234 If this is Livewire component

    /*
    * Submit user form
    */
    public function submitForm(User $user): void
    {
        $action = $user->exists ? 'Updated' : 'Created';

        // ...

        $user->fill($validated);
        $user->save();
        
        session()->flash('success', "User {$action} Successfully!");
        $this->redirectRoute('users.index');
    }

if you call submitForm() the $user is a blank model and equal to User:make()

if you call submitForm(5) you will get the corresponding model

and one things, on a blank model calling a relation as property is not trigger any query, so it means

for create

    submitForm()

    public function submitForm(User $user): void
    {
        // $user is a blank model
        $user->posts->pluck('id')->toArray();

        // You wouldn't see the query in Debugbar
    }

but for edit

    submitForm(5)

    public function submitForm(User $user): void
    {
        $user->posts->pluck('id')->toArray();

        // You will see the query in Debugbar
    }
ap1234's avatar

@newbie360 Yes, this is a livewire component, but here the problem is I can use $validated directly as the variables I have set are camel case and the database columns are snake case. Hence, using the above approach. Thank you for your answer!

martinbean's avatar

@ap1234 Why do you think “many” people are using a single Livewire component to handle multiple use cases? That’s just trying to DRY stuff out to the point that it’s gone too far the other way, and you’re now violating the single responsibility rule, or the ‘S’ (the very first letter) in ‘SOLID’.

If you can create and update users, they’re two separate use cases, and therefore should be handled by two separate components; not amalgamated into some “save new or existing user” god-component.

Please or to participate in this conversation.