Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow $data to be passed to view model #39

Merged

Conversation

JamesFreeman
Copy link
Contributor

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);
}

@JamesFreeman JamesFreeman changed the title All $data to be passed to view model Allow $data to be passed to view model Oct 23, 2021
@freekmurze freekmurze requested a review from brendt October 23, 2021 13:33
@brendt
Copy link
Contributor

brendt commented Oct 25, 2021

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

@JamesFreeman
Copy link
Contributor Author

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
Copy link
Contributor

brendt commented Oct 27, 2021

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.

@brendt brendt closed this Oct 27, 2021
@JamesFreeman JamesFreeman deleted the feature/allow-array-access-to-view branch November 1, 2021 09:51
@JamesFreeman JamesFreeman restored the feature/allow-array-access-to-view branch November 12, 2021 09:27
@JamesFreeman
Copy link
Contributor Author

JamesFreeman commented Nov 12, 2021

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:

  • Client Panel
  • Staff only Panel
  • Accounting panel

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
Copy link
Contributor

brendt commented Nov 24, 2021

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.

@brendt brendt reopened this Nov 24, 2021
@JamesFreeman
Copy link
Contributor Author

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 brendt merged commit 12d19e8 into spatie:master Dec 6, 2021
@brendt
Copy link
Contributor

brendt commented Dec 6, 2021

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

@JamesFreeman JamesFreeman deleted the feature/allow-array-access-to-view branch December 6, 2021 09:20
@asurcodes
Copy link

asurcodes commented Jan 12, 2022

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
Copy link
Contributor Author

Interesting, I'll look into this now.

@JamesFreeman
Copy link
Contributor Author

JamesFreeman commented Jan 12, 2022

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

@brendt
Copy link
Contributor

brendt commented Jan 13, 2022

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants