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

Ability to destructure properties and methods #21

Closed
wants to merge 1 commit into from

Conversation

jpmaga
Copy link

@jpmaga jpmaga commented Jan 3, 2020

This has come in handy more than once, so figured it might be useful to others as well.

@brendt
Copy link
Contributor

brendt commented Jan 10, 2020

Can you explain a little bit about when you'd use this?

@jpmaga
Copy link
Author

jpmaga commented Jan 10, 2020

Can you explain a little bit about when you'd use this?

Sure. Most recently I was working with an old codebase and the blade templates for some pages had variables already defined as $title, $testimonials, $gallery, ... and they were all coming straight from an Eloquent Model. I did want to refactor the controllers, but not really the templates. I also didn't want to define the view model with something like this:

public function __construct() {
    $this->page = Page::find(1);
     
    $this->title = $this-page->title;
    $this->testimonials = $this-page->testimonials;
    $this->gallery = $this-page->gallery;
    // ...
}

Seemed much easier to do it like this:

protected $destructure = ['page'];

public function page() {
    return Page::find(1);
}

(sorry about the syntax in the example, wrote this on mobile)

@brendt
Copy link
Contributor

brendt commented Jan 10, 2020

So by destructuring 'page', it would make title, testimonials and gallery directly available as variables on the viewmodel, is that correct?

@jpmaga
Copy link
Author

jpmaga commented Jan 10, 2020

So by destructuring 'page', it would make title, testimonials and gallery directly available as variables on the viewmodel, is that correct?

Yes, that is correct.

@brendt
Copy link
Contributor

brendt commented Jan 10, 2020

I'm not sure if we want to add this feature. I realise it might be useful in some cases, like you said you didn't want to change the view, but in my opinion the correct usage would be to use {{ $page->title }} in the view, or explicitly define the data you want to expose in the view model.

I realise there might be cases where this is useful, but I don't think we should add this functionality to the package for these edge exceptional situations. Luckily the base view model is extensible, so you're free to add this functionality in your own ViewModel base class in a project when you really need it.

So while I appreciate your effort to help improve this package, I'm not going to merge this PR.

@brendt brendt closed this Jan 10, 2020
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.

None yet

2 participants