Component attributes with boolean values are rendered differently when merge() is used

[perifer]

• Laravel Version: 7.0.8
• PHP Version: 7.2.26
• Database Driver & Version: n/a

Description:

With a component that simply prints it's attributes:

<input {{ $attributes }} />

If the component is used like this:

<x-input :disabled="false" />

We would expect the rendered template to be:

<input />

But the current implementation renders:

<input disabled="" />

But if the component uses merge() like this:

<input {{ $attributes->merge() }} />

The output is what one would expect:

<input />

It's debatable how an attribute with a boolean value should be handled, but at least $attribute and $attribute->merge() should give identical output, right?

[perifer]

@mfullbrook do you have any suggestions or input? One idea I had was that the logic that is used in merge() should always be applied to the output, but I couldn't figure out how to do that.

Here is a test btw for the ComponentAttributeBag:

public function testAttributeBag()
{
    $bag = new ComponentAttributeBag(['test' => true]);

    $this->assertSame('test="test"', (string) $bag); // Fails, actual output: test
    $this->assertSame('test="test"', (string) $bag->merge());

    $bag = new ComponentAttributeBag(['test' => false]);

    $this->assertSame('', (string) $bag); // Fails, actual output: test=""
    $this->assertSame('', (string) $bag->merge());
}

[mfullbrook]

With respect to the first two assertions in your test I think merge() is behaving incorrectly to html standard. A boolean attribute that is truthy should be rendered like: <input disabled />.

https://html.spec.whatwg.org/multipage/common-microsyntaxes.html#boolean-attributes

To solve the problem in the second two assertions ComponentAttributeBag::__toString needs to handle the case of $value === false.

[perifer]

I think it can be debated whether the output should be <input disabled /> or <input disabled="disabled" /> - the standard actually doesn't say anything about that:

This could be equivalently written as this:

<label><input type=checkbox checked=checked name=cheese disabled=disabled> Cheese</label>

I personally prefer <input disabled /> but for example Vue uses the longer form: https://github.com/vuejs/vue/blob/ae788136069bbf4f04de773aa0aa390a236342ed/test/ssr/ssr.sync.spec.js#L183

You don't think it duplicates logic that is already in merge() to have this in __toString as well? But it's definitely and option! Thanks for the input.

[mfullbrook]

Yes, you're right. Okay, how about this:

     foreach ($this->attributes as $key => $value) {
            if ($value === false) {
                continue;
            }
            $string .= $value === true
                    ? ' '.$key.'="'.$key.'"'
                    : ' '.$key.'="'.str_replace('"', '\\"', trim($value)).'"';
        }

Your tests pass now 😄

[perifer]

Excellent! I'm preparing a PR with some additional changes.

[perifer]

@mfullbrook the PR is up. Feel free to review it! Thanks again.

[alesvaupotic]

Having a similar problem myself:

Consider the following x-number-field component:

<label for="{{ $name }}" class="text-sm pl-2">{{__($module.".".$name)}}</label>
<input type="number" 
    id="{{$name}}" 
    name="{{$name}}" 
    value="{{ old($name, $record->$name ) }}"
    placeholder="{{___($module.".".$name.".placeholder")}}"
    {{ $attributes->merge(['class' => 'mb-2']) }}
> 

and using it in a Blade template like so:

<x-number-field name="age" min="0" max="18" class="w-10c" />

Notice the min="0" attribute which gets removed on merge by the array_filter in the following function

framework/src/Illuminate/View/ComponentAttributeBag.php

Line 133 in eed1fd2

return new static(array_merge($attributeDefaults, array_filter($attributes)));

I just removed the array_filter here and it works for me.

[alesvaupotic]

Sorry for the clutter, just found another reference to the same problem: #31942 (comment)