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

[9.x] Fix loadAggregate not correctly applying casts #41050

Merged
merged 4 commits into from
Feb 16, 2022
Merged

[9.x] Fix loadAggregate not correctly applying casts #41050

merged 4 commits into from
Feb 16, 2022

Conversation

aalyusuf
Copy link
Contributor

Reopening #41025 with tests and better explanation about the bug.

There's a bug in loadAggregate in Eloquent Collections specefically when running LoadExists where running something like

$user->articles->loadExists("comments") will add comments_exists to all existing articles but only the first model will have bool casts automatically added to it. Resulting in something similar to:

.. "comments_exists" => true
.. "comments_exists" => '1'
.. "comments_exists" => '1'  

This PR fixes that and adds casts to all items instead of only the first one. I highly doubt it'll be breaking change to anyone.

@taylorotwell
Copy link
Member

Can you provide a simple example of how I can recreate this issue in a fresh Laravel application with code examples? Please mark this as ready for review when you have done so otherwise I won't see it.

@taylorotwell taylorotwell marked this pull request as draft February 16, 2022 17:34
@aalyusuf
Copy link
Contributor Author

Hey @taylorotwell

Here's a simple code example ( Also the PR contains a test which would fail on a fresh laravel installation ):

Models

class User extends Authenticatable
{

    public function posts()
    {
        return $this->hasMany(Post::class);
    }

}
class Post extends Model
{
    use HasFactory;
    protected $guarded = [];

    public function comments()
    {
        return $this->hasMany(Comment::class);
    }
}
class Comment extends Model
{
    use HasFactory;
}

Some data

    $user = \App\Models\User::find(1);
    $user->posts()->createMany([
        ['content' => 'Test'],
        ['content' => 'Test'],
        ['content' => 'Test'],
    ]);

The test
Now, running loadExists on comments

    $user = \App\Models\User::with('posts')->find(1);
    $user->posts->loadExists("comments");
    return $user->posts;

This will result in the following ( notice comments_exists ) , loadExists here will only cast the first Post we created
image

and

 dd($user->posts->pluck('comments_exists')); 

image

@aalyusuf aalyusuf marked this pull request as ready for review February 16, 2022 18:52
@taylorotwell
Copy link
Member

I created a user with 3 posts and each posts has 3 comments... this is what I get:

CleanShot 2022-02-16 at 14 12 32@2x

@aalyusuf
Copy link
Contributor Author

I believe that's the response because of tinker & it's not casting there. But after executing loadExists the next line you can try can be:

$user->posts->pluck('comments_exists');

and this should reproduce the issue

image

or even directly

$user->posts->map->getCasts();

should give you:

image

@taylorotwell taylorotwell merged commit e765afc into laravel:9.x Feb 16, 2022
@taylorotwell
Copy link
Member

Thanks

@GrahamCampbell GrahamCampbell changed the title [8.x] Fix loadAggregate not correctly applying casts [9.x] Fix loadAggregate not correctly applying casts Feb 17, 2022
GrahamCampbell pushed a commit that referenced this pull request Feb 18, 2022
* Fix loadAggregate not correctly applying casts

* Added tests to "Fix loadAggregate not correctly applying casts"

* Style fix

* Update Collection.php

Co-authored-by: Taylor Otwell <taylor@laravel.com>
taylorotwell added a commit that referenced this pull request Feb 18, 2022
* Fix loadAggregate not correctly applying casts

* Added tests to "Fix loadAggregate not correctly applying casts"

* Style fix

* Update Collection.php

Co-authored-by: Taylor Otwell <taylor@laravel.com>

Co-authored-by: A. Alyusuf <ahmed@vioat.com>
Co-authored-by: Taylor Otwell <taylor@laravel.com>
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.

None yet

2 participants