spatie / laravel-view-models

View models in Laravel
https://spatie.be/open-source
MIT License
1.01k stars 61 forks source link

Allow $data to be passed to view model #39

Closed JamesFreeman closed 2 years ago

JamesFreeman commented 2 years ago

This PR allows you the ability to pass data into the ViewModel from the Controller, I thought this was useful if you are passing certain parts of the request into the view.

public function show(UsersShowViewModel $viewModel, Request $request, Post $post)
{
    $response = $this->validate([
        //...
    ]);

    $viewModel->view('form', $response);
}
brendt commented 2 years ago

I've been injecting the request into the VM's constructor to allow this behaviour. What's the benefit of this approach?

JamesFreeman commented 2 years ago

I think both approaches are valid. I personally like to DI the VM into the Controller params at the moment, it's not possible to pass data into the view when using this method. I felt that this would allow for a more consistent controller by allowing this instead of some being newed up vs DI.

brendt commented 2 years ago

I'm not convinced about it… I prefer one way of doing things, and personally like the solution with less magic better. I appreciate you thinking along, but I'm afraid I'll close this one for now.

JamesFreeman commented 2 years ago

Hi @brendt,

Would you be able to reconsider this, the more I use this package the more I feel this is massively missing. Imagine this situation:

We have 3 admin areas:

Each of these panels has a BaseView model that gets called in the controller I.e. ClientViewModel, AdminViewModel, and AccountingViewModel, all of these have a contract implemented so the view gets the same variables for the theme to render.

All of these panels, share the same code when it comes to authentication, I just use inheritance to change the guard based on what's going on.

class AdminMFASetupController extends BaseMFASetupController
{
    public function getViewModel()
    {
        return AdminMFASetupViewModel::class;
    }

    public function getGuard()
    {
        return 'admin';
    }
}
abstract class BaseMFASetupController
{
    public function index()
    {
         //...
        $qrURL = QRGenerate::run();

        $viewModel = new ($this->getViewModel())(qrURL: $qrURL);

        return $viewModel->view('some-view');
    }
}

In this case, I would need to create a ViewModel for every instance of our Panel which has this Setup, It would be a lot cleaner to be able to use the ClientViewModel and the one off variable into the view() call.

public function index()
{
    $qrURL = QRGenerate::run();

    return (new $this->getViewModel())->view('some-view', ['qrURL' => $qrURL]);
}

public function getViewModel()
{
    return AdminViewModel::class;
}

Thoughts?

brendt commented 2 years ago

Thanks for the example, I believe I misunderstood your first example because of the request/response injection. Passing data to the view makes a lot more sense.

I've still got one reservation though with:

I would need to create a ViewModel for every instance of our Panel which has this Setup

Yes, that was kind of my design goal: a dedicated view model for every view. Personally I wouldn't choose the level of abstraction you're introducing with the BaseMFASetupController etc. That might be a personal preference but experience teaches me that this level of abstraction will sooner or later turn into a mess.

Apart from that, I think I'm fine merging this in. Let me ask around for some opinions at Spatie, and I'll get back to it.

JamesFreeman commented 2 years ago

Thanks for the example, I believe I misunderstood your first example because of the request/response injection. Passing data to the view makes a lot more sense.

I've still got one reservation though with:

I would need to create a ViewModel for every instance of our Panel which has this Setup

Yes, that was kind of my design goal: a dedicated view model for every view. Personally I wouldn't choose the level of abstraction you're introducing with the BaseMFASetupController etc. That might be a personal preference but experience teaches me that this level of abstraction will sooner or later turn into a mess.

Apart from that, I think I'm fine merging this in. Let me ask around for some opinions at Spatie, and I'll get back to it.

Hey @brendt, have you guys had any more thoughts on this?

brendt commented 2 years ago

Tagged: https://github.com/spatie/laravel-view-models/releases/tag/1.4.0

asurcodes commented 2 years ago

This PR has broken the use of the property data in the response body using the library, this use to work before 1.4.0, now it results in some weird data duplication.

Given a ViewModel:

<?php

namespace App\Api\ViewModels\Currency;

use Spatie\ViewModels\ViewModel;
use Illuminate\Support\Collection;

class CurrenciesViewModel extends ViewModel
{
    public function __construct(Collection $data = null)
    {
        $this->data = $data;
    }

    public function data(): array
    {
        return $this->data->toArray();
    }

    public function message(): string
    {
        return __('models.currencies.retrieved');
    }
}

The response is:

{#4038
  +"message": "Currencies have been retrieved successfully!"
  +"data": array:2 [
    0 => {#4134
      +"id": 1
      +"name": "Euro"
      +"symbol": "€"
      +"iso": "EUR"
      +"created_at": "2022-01-12T13:42:45.000000Z"
      +"updated_at": "2022-01-12T13:42:45.000000Z"
    }
    1 => {#4033
      +"id": 2
      +"name": "Dollar"
      +"symbol": "$"
      +"iso": "USD"
      +"created_at": "2022-01-12T13:42:45.000000Z"
      +"updated_at": "2022-01-12T13:42:45.000000Z"
    }
  ]
  +"0": {#3589
    +"id": 1
    +"name": "Euro"
    +"symbol": "€"
    +"iso": "EUR"
    +"created_at": "2022-01-12T13:42:45.000000Z"
    +"updated_at": "2022-01-12T13:42:45.000000Z"
  }
  +"1": {#4049
    +"id": 2
    +"name": "Dollar"
    +"symbol": "$"
    +"iso": "USD"
    +"created_at": "2022-01-12T13:42:45.000000Z"
    +"updated_at": "2022-01-12T13:42:45.000000Z"
  }
}

I'm not aware if it's following semver, but a major version bump would have been nice for composer compatibility. I'm just raising awareness since its a pretty common key to use in API responses.

JamesFreeman commented 2 years ago

Interesting, I'll look into this now.

JamesFreeman commented 2 years ago

@asurcodes Just sent a bug fix PR in to fix this issue moving forward.

brendt commented 2 years ago

Fix is tagged in 1.5.1: https://github.com/spatie/laravel-view-models/releases/tag/1.5.1