laravel / ideas

Issues board used for Laravel internals discussions.
938 stars 28 forks source link

Can't set default component attributes from class #2104

Open ryangjchandler opened 4 years ago

ryangjchandler commented 4 years ago

Description:

Opening this issue after I broke the initial 7.x release 😢 . The fix that I had put in place would successfully pass the attributes that were being added from the component class constructor, but it would not successfully merge the attributes coming from the actual component tag itself.

Steps To Reproduce:

Component class

class Test extends Component
{
    public function __construct()
    {
        $this->withAttributes(['class' => 'bg-red-500 px-4']);
    }

    public function render()
    {
        return view('components.test');
    }
}

Component view

<div {{ $attributes }}>
    Testing
</div>

Parent view

<x-test class="border border-black py-4" />

The expected HTML would be:

<div class="bg-red-500 px-4 border border-black py-4">
    Testing
</div>

but the actual output is:

<div class="bg-red-500 px-4">
    Testing
</div>

Investigation

Whilst investigating this, I followed the problem down to the compiled Blade file. I believe the changes from my PR laravel/framework#31676 were not working correctly as the $component->withAttributes() call was taking place after the $component->data() call, so the compiler was not performing the merge at all.

Would like to know what others think. I have a few ideas for fixes that work now that I've written some decent tests, but I'm not sure if we should just leave this and instead tell users to invoke the $attributes variable with their default values instead.

driesvints commented 4 years ago

Moving this to the ideas repo as it's more a feature request. Feel free to attempt a new pr with tests once you've figured thinks out.

matijakovacevic commented 4 years ago

@driesvints, if you wouldn't mind opening my eyes to the following :)

is this the intended behaviour

$img->withAttributes(['foo' => 'boo']);
$img->withAttributes(['bar' => 'car']);
dd($img->attributes);
// EXPECTED: foo&bar
// GOT: only bar

// ----------------------------------

$img = app()->make(Img::class, ['src' => 'some-image.jpg']);
$img->withAttributes(['foo' => 'boo']);

// presumably, this functionality is for component view, because it
// returns new ComponentAttributeBag
$img->attributes->merge(['bar' => 'car']);

dd($img->attributes);
// EXPECTED: foo&bar
// GOT: only foo

What I've got from this is that withAttributes basically sets/resets the whole ComponentAttributeBag. Tested on Laravel version 7.1.0

The solution would be to replace

public function setAttributes(array $attributes) 
{
    $this->attributes += $attributes;
}

in ComponentAttributeBag:line 138

But I don't know if that would break the logic/vision of current implementation.

ryangjchandler commented 4 years ago

The withAttributes() function completely overwrites the attributes on the component. I think it should be changed to merge them instead but I imagine this might be considered a breaking change now.

ianjamieson commented 3 years ago

I assumed that withAttributes would merge attributes. I was able to access the merge method like this:

For me, $this->attributes is always empty on this line:

https://github.com/laravel/framework/pull/31676/files#diff-1005052c893d685dfa5b9c5c7431e2a880260f9a16cba75e16689ec33549755cR215

So there is always a new ComponentAttributeBag

And even when I call withAttributes() they are still not passed to the view, eg.

testing.blade.php

attributes: <br>{{ $attributes }}

Testing.php

<?php
class Testing extends Component
{
    public function render()
    {
        $this->withAttributes(['new' => 'attr']);
        return view('components.testing');
    }
}

Actual when rendered:

attributes:

Expected when rendered:

attributes: new="attr"

Note:

If I call the component like:

<x-testing hello="world"></x-testing>

Then the output is:

attributes: hello="world"
ianjamieson commented 3 years ago

I thought that perhaps this, https://laravel.com/docs/8.x/blade#using-attributes-slots-within-component-class

Would give access to the $attributes object, which is does:

<?php
class Testing extends Component
{
    public function render()
    {
        return function (array $data) {
            $data['attributes'] = $data['attributes']->merge(['foo' => 'bar']);
            return 'components.testing';
        };
    }
}

The above works if you print the attributes in that render. However, the rendered view remains unaffected.

I am able to overwrite or set attributes not merge, if I do this:

<?php
class Testing extends Component
{
    public function render()
    {
        return function (array $data) {
            $data['attributes']['foo'] = 'bar';
            return 'components.testing';
        };
    }
}