laravel / framework

The Laravel Framework.
https://laravel.com
MIT License
32.45k stars 11k forks source link

Passing an Eloquent Model to an anonymous component results in expensive toJson call #39232

Closed tobyzerner closed 3 years ago

tobyzerner commented 3 years ago

Description

This blog post explains the issue nicely, so I'm basically just copying it here. Hope you don't mind, @hbgl!

Take a look at this anonymous component:

<!-- post-title.blade.php -->
@props(['post'])
<h1 {{ $attributes }}>{{ $post->title }}</h1>

And here is how it is used in, say, a post list page:

@foreach ($posts $post)
  <x-post-title :post="$post" />
@endforeach

In this example, the posts are Eloquent models that were previously loaded from the database. Now, if you run this code, you will notice nothing strange in particular. Everything works as intended. However, if you dig a little deeper, you will find some unexpected behavior:

Every time the component is instantiated, the post model is being serialized to JSON.

The cause for this behavior lies in the way the Blade component compiler processes attributes inside the sanitizeComponentAttribute function.

// CompilesComponents.php
public static function sanitizeComponentAttribute($value)
{
    return is_string($value) ||
           (is_object($value) && ! $value instanceof ComponentAttributeBag && method_exists($value, '__toString'))
                    ? e($value)
                    : $value;
}

Basically, any object that implements a __toString function will be converted to a string. In the case of an Eloquent model, this will convert it to JSON. Depending on how large your model is and how often the component is used, this will waste a significant amount of resources on constructing JSON strings that are never used.


In the blog post, @hbgl goes on to describe a solution: use a class based component and add your props to the constructor arguments. After you do that, your objects will no longer be converted to strings.

However, I think this bug is worth fixing. In my project I use a lot of anonymous components and my app's performance is suffering pretty significantly from this bug. While I may well go down the road of converting them into classes, I think this bug really limits the real-world utility and scalability of anonymous components.

I don't have a good solution in mind – my understanding of the internals here is too limited. But I do wonder, since props are defined at the top of each anonymous component's template, is there some way to read those in advance and then prevent them from being sanitized and passed into the AttributeBag?


Note: This issue is not the same as #34346, where props being passed into dynamic components were being HTML-encoded. This issue is about anonymous components and still persists.

SjorsO commented 3 years ago

I have similar issues in an application I'm working on. The blade view somehow calls toArray() on my models which triggers a bunch of accessors (which unfortunately do queries). Overriding the toArray method prevents all these queries (and this somehow causes no other problems, my views still work fine):

public function toArray()
{
    return [];        
}
driesvints commented 3 years ago

I'll ask Taylor to have a look at this. Thanks.

taylorotwell commented 3 years ago

PRs welcome.

stevebauman commented 3 years ago

This may be a dumb question -- but do we need to sanitize objects with anonymous components?

Here the compiler is outputting the sanitization of the object, causing the serialization, but the serialized post is not actually being output inside of the HTML, it's just being passed to the component bag so its attributes can be retrieved using ArrayAccess:

https://github.com/laravel/framework/blob/024067fd56b227d74ce0b70d6b78ef5ad7095d6d/src/Illuminate/View/Compilers/ComponentTagCompiler.php#L594-L603

Rendered:

<?php $component->withAttributes(['post' => \Illuminate\View\Compilers\BladeCompiler::sanitizeComponentAttribute($post)]); ?>

Unless the post's properties will be output in the case of the below blade (I also lack enough knowledge to know for sure):

<h1 {{ $attributes }}></h1>

Maybe the AnonymousComponent needs a withProperties() method utilizing a different view data key rather than attributes so we don't sanitize those but sanitize all other values?

I.e.:

<?php $component->withProps(['post' => $post]); ?>
taylorotwell commented 3 years ago

Potential fix PR here: https://github.com/laravel/framework/pull/39319