Attributes collision using BelongsToMany + cursor()

[PGBI]

• Laravel Version: 5.5.21
• PHP Version: 7.1
• Database Driver & Version: mysql 5.7

Description:

When looping through a belongsToMany association using cursor(), the models aren't correctly hydrated: they get filled with values of the pivot table. If the pivot table have columns with the same name as columns in the related model table, pivot values are used to hydrate the related model.

Steps To Reproduce:

DB setup

• users table: id, name, timestamps
• groups table: id, name, timestamps
• group_user table: id, name, timestamps, user_id, group_id, approved (boolean)

Relationship defined as such:

class User extends Model
{
    public function groups()
    {
        return $this->belongsToMany(Group::class)
            ->withTimestamps()
            ->withPivot('name', 'approved');
    }
}

Actual code showing bug:

$john = User::create(['name' => 'John']);
$group = Group::create(['name' => 'subscribers']);

$john->groups()->attach($group, ['name' => 'john_subscription', 'approved' => true]);

foreach ($john->groups as $group) {
    dump(get_class($group));
    dump($group->getAttributes());
}

foreach ($john->groups()->cursor() as $group) {
    dump(get_class($group));
    dump($group->getAttributes());
}

Outputs:

"App\Group"
array:4 [
  "id" => "1"
  "name" => "subscribers"
  "created_at" => "2017-11-20 11:17:51"
  "updated_at" => "2017-11-20 11:17:51"
]
// ^ using $john->groups as $group produces a correct $group model.

"App\Group"
array:7 [
  "id" => "1"
  "name" => "john_subscription" // <-- wrong, that's the pivot name
  "created_at" => "2017-11-20 11:17:52" // <-- wrong, that's the pivot creation date
  "updated_at" => "2017-11-20 11:17:52" // <-- wrong, that's the pivot update date
  "user_id" => "1" // <-- should be in pivot, not in $group attributes
  "group_id" => "1" // <-- should be in pivot, not in $group attributes
  "approved" => "1" // <-- should be in pivot, not in $group attributes
]
// ^ using $john->groups()->cursor as $group produces a corrupted $group model.

I had actually proposed a PR to fix this (#22126), but it was rejected because: "breaking change". So instead of attempting to fix it myself, I figured I would post a bug report :)

[sisve]

Related:
laravel/ideas#347
#16704
#21774
#22127

[sylouuu]

A dirty workaround:

foreach ($john->groups()->cursor() as $group) {
    $group->id = $group->attributesToArray()['group_id'];
    dump($group->getAttributes());
}

@taylorotwell Is it possible to consider a fix for a future version?

[sylouuu]

@iceheat I don't understand why you ref this issue in #25015 if your PR is not related to #22144?

@PGBI has already reported an issue with BelongsToMany & cursor here with the steps to reproduce.

Could you try it on a fresh install? :)

Best

[AmmarRahman]

@sylouuu My apologies for referencing this issue on PR that does not fix it. I blindly assumed that the issue you were complaining about was about hasManyThrough and cursor() since you were asking about it in a pr addressing hasManyThrough and chunk().

I had a quick look at the code for BelongsToMany. I believe I have a grasp of what is causing the issue. I will try to dedicate some time over the evenings/weekend to write a few integration tests before introducing changes because this can easily introduce breaking changes.

Cheers,

[staudenmeir]

@PGBI Can you resubmit your PR for Laravel 5.8?

[PGBI]

@staudenmeir Feel free to cherry-pick my commits and resubmit the PR. Given how my PR was rejected without the slightest start of explanation, I'm not willing to submit it again.

[mpyw]

[5.7] Fix each() on BelongsToMany relationships by staudenmeir · Pull Request #25832 · laravel/framework

This is exactly the same type of issue and has been already merged. So the cherry-picked PR must be going to be merged.

[Werelds]

We just ran into this issue and fortunately in our case, we didn't modify related models' attributes whilst using a cursor. But should one do so, it could very well update nothing or completely unrelated records.

I don't understand why the original PR was rejected by @taylorotwell, because while it is technically a BC breaking change, the current behaviour is not only broken, it is outright dangerous and this should be considered a critical issue.

I've set up a quick and dirty example of this in a PHPUnit test case; the test case actually tests the current, broken behaviour on purpose. See https://github.com/Werelds/laravel-broken-cursor-demo

I'm going to create a fresh PR for this, because this has to be fixed. In its current implementation, one can not use cursors on at least BelongsToMany because it is inherently broken. It seems like it was fixed on some of the other relationship types, but for my own peace of mind I will test this against any relationship type that uses pivot tables.

[driesvints]

This seems to be fixed in the current release. Thanks @mpyw and @PGBI for the original pr.