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

refresh(): Call to undefined relationship [pivot] on model #31811

Closed
LastDragon-ru opened this issue Mar 6, 2020 · 6 comments
Closed

refresh(): Call to undefined relationship [pivot] on model #31811

LastDragon-ru opened this issue Mar 6, 2020 · 6 comments
Labels

Comments

@LastDragon-ru
Copy link
Contributor

  • Laravel Version: 6.18.0
  • PHP Version: 7.2
  • Database Driver & Version: Mysql 5.7

Description:

This is a regression introduced in #31125 - all custom pivot models that uses AsPivot trait are broken now due to this line:

return $relation instanceof Pivot;

obviously it should also check if the model uses AsPivot trait. Something like:

return $relation instanceof Pivot
    || in_array(AsPivot::class, class_uses_recursive($relation), true);

Steps To Reproduce:

  1. Create new laravel 6.x app
  2. Create user_users table
    CREATE TABLE IF NOT EXISTS `user_users` (
        `my_user_id` BIGINT(20) UNSIGNED NOT NULL,
        `pivot_my_user_id` BIGINT(20) UNSIGNED NOT NULL
    )
  3. Run this test
    <?php
    
    namespace Tests\Unit;
    
    use App\User;
    use Illuminate\Database\Eloquent\Model;
    use Illuminate\Database\Eloquent\Relations\Concerns\AsPivot;
    use Tests\TestCase;
    
    class ExampleTest extends TestCase {
        public function testAsPivotRefresh() {
            $user  = MyUser::query()->findOrFail(factory(User::class)->create()->getKey());
            $child = MyUser::query()->findOrFail(factory(User::class)->create()->getKey());
    
            $user->users()->attach($child->getKey());
    
            $this->assertEquals(1, $user->users->count());
    
            $user->users->first()->refresh();
        }
    }
    
    class MyUser extends User {
        protected $table = 'users';
    
        public function users() {
            return $this
                ->belongsToMany(static::class, (new UserPivot())->getTable(), 'my_user_id', 'pivot_my_user_id')
                ->using(UserPivot::class);
        }
    }
    
    class UserPivot extends Model {
        use AsPivot;
    
        protected $table = 'user_users';
    }
@GrahamCampbell
Copy link
Member

in_array(AsPivot::class, class_uses_recursive($relation), true);

This feels kinda hacky. Shouldn't we have an interface for this?

@LastDragon-ru
Copy link
Contributor Author

This feels kinda hacky. Shouldn't we have an interface for this?

Agreed, but interface will not fix existing code

@ryangjchandler
Copy link
Contributor

in_array(AsPivot::class, class_uses_recursive($relation), true);

This feels kinda hacky. Shouldn't we have an interface for this?

I'd prefer to use the interface in all honesty, and I'm sure if people run into this problem they will find the information in the docs somewhere.

I don't believe traits should be used to enforce outside functionality on the edge of your application. The interface approach has already been taken with other checks & rules for model classes.

Happy to make a PR with the new marker interface.

@driesvints
Copy link
Member

Why aren't you extending the Pivot class?

@LastDragon-ru
Copy link
Contributor Author

LastDragon-ru commented Mar 16, 2020

Why aren't you extending the Pivot class?

@driesvints for the same reasons as described in initial PR #25851 ("own abstract Model class")

@driesvints driesvints changed the title [6.x] refresh(): Call to undefined relationship [pivot] on model refresh(): Call to undefined relationship [pivot] on model Mar 17, 2020
@driesvints
Copy link
Member

I see. Then an interface won't work here because people would have to implement it manually to solve the problem. We could maybe go for the solution you proposed above. Feel free to send in a PR.

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

No branches or pull requests

4 participants