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

Blade slots no longer accept "falsy" values #38542

Closed
browner12 opened this issue Aug 25, 2021 · 8 comments · Fixed by #38546
Closed

Blade slots no longer accept "falsy" values #38542

browner12 opened this issue Aug 25, 2021 · 8 comments · Fixed by #38546
Labels

Comments

@browner12
Copy link
Contributor

  • Laravel Version: 8.56.0
  • PHP Version: 8.0.5
  • Database Driver & Version: MySQL 8.0

Description:

Blade component slots no longer accept "falsy" values (null, '', 0, false, []).

For example:

  • @slot('enabled', null)
  • @slot('enabled', '')
  • @slot('enabled', 0)
  • @slot('enabled', false)
  • @slot('enabled', [])

This causes the templating/layout system to break, which results in a messed up HTML output.

Reverting to Laravel 8.55.0 fixes the problem. My guess is this is caused by the changes to Blade in PR #38372.

Steps To Reproduce:

In a view, use a component that has a falsy value in a slot.

@extends('layouts.master')

@section('content')

    <div>Some Header</div>

    @component('some-component')
        @slot('key', false)
    @endcomponent

@endsection
@driesvints
Copy link
Member

Ping @danharrin

@driesvints driesvints added the bug label Aug 25, 2021
@Jubeki
Copy link
Contributor

Jubeki commented Aug 25, 2021

I have setup a Repo for the falsy values: https://github.com/Jubeki/laravel-bug-report-38542

The dump output is as follows:

@component('components.component-with-slots')
    @slot('falsySlot', null)
    {{-- Dump: "" --}}
@endcomponent

@component('components.component-with-slots')
    @slot('falsySlot', 0)
    {{-- Dump: "" --}}
@endcomponent

@component('components.component-with-slots')
    @slot('falsySlot', '')
    {{-- Dump: "" --}}
@endcomponent

@component('components.component-with-slots')
    @slot('falsySlot', [])
    {{-- Dump: "" --}}
@endcomponent

@component('components.component-with-slots')
    @slot('falsySlot', 'TEST')
    {{-- Dump: "TEST" --}}
@endcomponent

@component('components.component-with-slots')
    @slot('falsySlot', 5)
    {{-- Dump: 5 --}}
@endcomponent

It seems to be converting the values to empty strings.

Jubeki referenced this issue Aug 25, 2021
* Update slot pattern

* Add attributes params to existing tests

* Create ComponentSlot class

* Pass attributes from slot tag to Blade directive

* Compile slot attributes into slot object

* Add compilation tests for attribute support

* Remove unused exception

* Reorder arguments

* Fix dynamic components with slot attributes

* Escape bound attributes for slots

* Update BladeComponentTagCompilerTest.php

* formattinG

Co-authored-by: Taylor Otwell <taylorotwell@gmail.com>
@danharrin
Copy link
Contributor

Checking now and will submit another PR. One issue we have is that many parts of Blade are not currently testable, and this is one of them.

@danharrin
Copy link
Contributor

danharrin commented Aug 25, 2021

@browner12 @Jubeki can you please check if #38546 fixes this?

@Jubeki
Copy link
Contributor

Jubeki commented Aug 25, 2021

Yes can confirm that it now is working as intended.

@browner12
Copy link
Contributor Author

It does.

It seems like a good hotfix for now, but at first glance it seems like this may be prime for other issues in the future.

I'll admit I'm not super familiar with this code, though, so I'll have to dig in more.

Thanks for figuring this out!

@danharrin
Copy link
Contributor

It seems like a good hotfix for now, but at first glance it seems like this may be prime for other issues in the future.

Eh it's not much of a "hotfix" as this is how the framework did this check before my PR - unless you're referring to the lack of tests.

@browner12
Copy link
Contributor Author

hah, yah mostly referring to the fact that relying on the number of arguments passed is probably gonna cause us issues in the future....

I've got no other solution right now, so I'm happy with this solution!

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

Successfully merging a pull request may close this issue.

4 participants