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

[8.x] Fix $component not being reverted if component doesn't render #39595

Merged
merged 2 commits into from
Nov 16, 2021

Conversation

tobyzerner
Copy link
Contributor

If a component does not render due to its shouldRender method returning false, then the value of the $component variable will never be reverted back to its original value.

Example:

use Illuminate\Support\Facades\Blade;
use Illuminate\View\Component;

class Outer extends Component
{
  public $test = 'yay!';

  public function render()
  {
    return '{{ $slot }}';
  }
}

class Inner extends Component
{
  public function shouldRender()
  {
    return false;
  }

  public function render()
  {
    return 'inner';
  }
}

Blade::component(Outer::class, 'outer');
Blade::component(Inner::class, 'inner');

Route::get('/', function (){
  return view('test');
});
{{-- test.blade.php --}}
<x-outer>
    <x-inner />
    {{ $component->test }}
</x-outer>

Expected output:

yay!

Actual output:

Undefined property: Inner::$test

Fixed by moving the statements that revert $component outside of the shouldRender conditional. Tests updated accordingly. As far as I can tell, these statements aren't needed inside of compileEndComponent anyway, because the $component variable is only overwritten when starting a class component.

@taylorotwell
Copy link
Member

Feels pretty odd to just delete a lot of code from compileEndComponent? Seems like something is going to break?

@tobyzerner
Copy link
Contributor Author

tobyzerner commented Nov 15, 2021

@taylorotwell All tests passing.

The code hasn't been deleted, it's been moved. The code that has been moved is self-contained. It only has an effect if $__componentOriginal{hash} is set. $__componentOriginal{hash} is only set in compileClassComponentOpening. If compileClassComponentOpening is called, then compileEndComponentClass (where the code has been moved to) will be called too.

Here are some examples of the compiled output to demonstrate:


BEFORE: code for a class component (indented for clarity). The unset call only takes place if shouldRender has returned true.

<?php if (isset($component)) { $__componentOriginalf35a3d9ace38e6b373c8c989b27270a64979c7ad = $component; } ?>
<?php $component = $__env->getContainer()->make(App\Views\Components\Test::class, []); ?>
<?php $component->withName('test'); ?>
<?php if ($component->shouldRender()): ?>
    <?php $__env->startComponent($component->resolveView(), $component->data()); ?>
    <?php $component->withAttributes([]); ?>
    <?php if (isset($__componentOriginalf35a3d9ace38e6b373c8c989b27270a64979c7ad)): ?>
        <?php $component = $__componentOriginalf35a3d9ace38e6b373c8c989b27270a64979c7ad; ?>
        <?php unset($__componentOriginalf35a3d9ace38e6b373c8c989b27270a64979c7ad); ?>
    <?php endif; ?>
    <?php echo $__env->renderComponent(); ?>
<?php endif; ?>

AFTER: Now the unset call has been moved outside of the shouldRender condition.

<?php if (isset($component)) { $__componentOriginalf35a3d9ace38e6b373c8c989b27270a64979c7ad = $component; } ?>
<?php $component = $__env->getContainer()->make(App\Views\Components\Test::class, []); ?>
<?php $component->withName('test'); ?>
<?php if ($component->shouldRender()): ?>
    <?php $__env->startComponent($component->resolveView(), $component->data()); ?>
    <?php $component->withAttributes([]); ?>
    <?php echo $__env->renderComponent(); ?>
<?php endif; ?>
<?php if (isset($__componentOriginalf35a3d9ace38e6b373c8c989b27270a64979c7ad)): ?>
    <?php $component = $__componentOriginalf35a3d9ace38e6b373c8c989b27270a64979c7ad; ?>
    <?php unset($__componentOriginalf35a3d9ace38e6b373c8c989b27270a64979c7ad); ?>
<?php endif; ?>

BEFORE: code for a non-class component (indented for clarity). The isset condition will always be false and that block of code is redundant.

<?php $__env->startComponent('components.test'); ?>
<?php if (isset($__componentOriginal30db0f1643a6503c95bd8eba93e50f3610c2071c)): ?>
    <?php $component = $__componentOriginal30db0f1643a6503c95bd8eba93e50f3610c2071c; ?>
    <?php unset($__componentOriginal30db0f1643a6503c95bd8eba93e50f3610c2071c); ?>
<?php endif; ?>
<?php echo $__env->renderComponent(); ?>

AFTER:

<?php $__env->startComponent('components.test'); ?>
<?php echo $__env->renderComponent(); ?>

@taylorotwell
Copy link
Member

There is likely a breaking change in that I could do something like this manually in a Blade component:

@component(SomeClass::class)
   ...
@endcomponent

In this case, compileClassComponentOpening would be called but only compileEndComponent would be called - not compileEndComponentClass.

I know this isn't explicitly documented as a way to use components but some packages could be using them this way.

@taylorotwell
Copy link
Member

Is there a way to solve this problem without those breaking changes?

@tobyzerner
Copy link
Contributor Author

Ah I didn't realise that. I'll have a think 👍

@tobyzerner
Copy link
Contributor Author

tobyzerner commented Nov 16, 2021

@taylorotwell The case you've mentioned above doesn't actually compile properly on 8.x in the first place. This is the result of compileClassComponentOpening being called and opening a shouldRender condition, but then compileEndComponent never closes it. The output showing the unbalanced if statement is below (indentation added):

<?php if (isset($component)) { $__componentOriginal6efa67299e2643b286abf9beb6b681a6c243977f = $component; } ?>
<?php $component = $__env->getContainer()->make(SomeClass::class, []); ?>
<?php $component->withName(); ?>
<?php if ($component->shouldRender()): ?>
    <?php $__env->startComponent($component->resolveView(), $component->data()); ?>
    ...
    <?php if (isset($__componentOriginal6efa67299e2643b286abf9beb6b681a6c243977f)): ?>
        <?php $component = $__componentOriginal6efa67299e2643b286abf9beb6b681a6c243977f; ?>
        <?php unset($__componentOriginal6efa67299e2643b286abf9beb6b681a6c243977f); ?>
    <?php endif; ?>
    <?php echo $__env->renderComponent(); ?>

Even if you change the code to:

@component(SomeClass::class)
   ...
@endcomponentclass

It fails with the error:

Too few arguments to function Illuminate\View\Component::withName(), 0 passed

Can't be a breaking change if the functionality is already broken? 😅

@taylorotwell taylorotwell merged commit 40f2150 into laravel:8.x Nov 16, 2021
@taylorotwell
Copy link
Member

OK - I'll give it a shot.

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.

2 participants