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

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

Closed
tobyzerner opened this issue Oct 17, 2021 · 5 comments · Fixed by #39319
Closed

Comments

@tobyzerner
Copy link
Contributor

  • Laravel Version: 8.64.0
  • PHP Version: 8.0.11

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
Copy link
Contributor

SjorsO commented Oct 18, 2021

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
Copy link
Member

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

@taylorotwell
Copy link
Member

PRs welcome.

@stevebauman
Copy link
Contributor

stevebauman commented Oct 21, 2021

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:

protected function attributesToString(array $attributes, $escapeBound = true)
{
return collect($attributes)
->map(function (string $value, string $attribute) use ($escapeBound) {
return $escapeBound && isset($this->boundAttributes[$attribute]) && $value !== 'true' && ! is_numeric($value)
? "'{$attribute}' => \Illuminate\View\Compilers\BladeCompiler::sanitizeComponentAttribute({$value})"
: "'{$attribute}' => {$value}";
})
->implode(',');
}

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
Copy link
Member

Potential fix PR here: #39319

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

Successfully merging a pull request may close this issue.

5 participants