Framework: Attributes collision using BelongsToMany + cursor()

Created on 20 Nov 2017 · 9Comments · Source: laravel/framework

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 :)

bug

Source

PGBI

👍8 😕1

All 9 comments

sisve on 20 Nov 2017

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 on 16 Feb 2018

@iceheat I don't understand why you ref this issue in https://github.com/laravel/framework/pull/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

sylouuu on 31 Jul 2018

@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,

AmmarRahman on 31 Jul 2018

👍1

@PGBI Can you resubmit your PR for Laravel 5.8?

staudenmeir on 30 Sep 2018

@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.

PGBI on 30 Sep 2018

[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.

mpyw on 30 Jan 2019

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.