Skip to content

Conversation

@mjauvin
Copy link
Member

@mjauvin mjauvin commented May 14, 2024

I have model that has a belongsToMany relation on itself and somehow, calling the relation on the model (i.e. $model->$relationName()) creates an infinite loop.

Using only the model relation definition solves the issue.

@mjauvin mjauvin added this to the 1.2.7 milestone May 14, 2024
@mjauvin mjauvin requested a review from LukeTowers May 14, 2024 16:58
@mjauvin mjauvin self-assigned this May 14, 2024
@LukeTowers
Copy link
Member

somehow, calling the relation on the model (i.e. $model->$relationName()) creates an infinite loop.

Not a fan of "somehow" issues being solved without understanding exactly how 😜 Can we dig further into why exactly an infinite loop is caused when attempting to access the relation method at this point in time? I would prefer to address that problem (or at least fully understand it) before just giving up and resorting to the relationship definition properties.

Does this support relations defined via methods instead of the property approach?

@mjauvin
Copy link
Member Author

mjauvin commented May 14, 2024

Not a fan of "somehow" issues being solved without understanding exactly how 😜 Can we dig further into why exactly an infinite loop is caused when attempting to access the relation method at this point in time? I would prefer to address that problem (or at least fully understand it) before just giving up and resorting to the relationship definition properties.

It works for all my models except one that has a relation on itself, so we need to look there.

Does this support relations defined via methods instead of the property approach?

What do you mean "relations defined via methods" ?

@mjauvin
Copy link
Member Author

mjauvin commented May 14, 2024

@LukeTowers I was able to reproduce with a MUCH simpler model and a trait specifically written to only trigger this behavior:

trait Test
{
    public function initializeTest() : void
    {
        $relation = $this->myrelation();
    }   
}

And the model:

class MyModel extends \Model
{
    use Test;

    public $table = 'mytable';

    public $belongsToMany = [
        'myrelation' => [
            MyModel::class,
        ],
    ];
}

@mjauvin
Copy link
Member Author

mjauvin commented May 14, 2024

Not sure EXACTLY what is triggering the loop, but the trait initializes itself then for each relation instanciated (which uses the same model), the trait is probably triggered again when the relation is instanciated...

This can easily be reproduced in php artisan tinker by instanciating the model.

# new MyModel

PHP Fatal error:  Allowed memory size of 134217728 bytes exhausted (tried to allocate 262144 bytes) in /var/www/vhosts/iet.studioazura.com/vendor/winter/storm/src/Database/Concerns/HasRelationships.php on line 224
PHP Fatal error:  Allowed memory size of 134217728 bytes exhausted (tried to allocate 262144 bytes) in Unknown on line 0

@mjauvin
Copy link
Member Author

mjauvin commented May 16, 2024

@LukeTowers I did a bit more digging and it turns out Laravel's HasRelationships concern adds a call to newRelatedInstance() method for all relation types which basically create a new instance of the related model... so if the related model is itself (with the HasSortableRelations trait), then that's where the loop gets created.

@mjauvin
Copy link
Member Author

mjauvin commented May 16, 2024

So in conclusion, I think the fix in this PR is the right way to address the issue.

And to address your question about relations defined in a method (I assume you meant what @bennothommo introduced in this PR, the getRelationDefinition() method in his PR works for both way to define relations.

@mjauvin mjauvin requested review from LukeTowers and bennothommo May 16, 2024 13:42
@LukeTowers
Copy link
Member

@mjauvin Sounds good, is this something we can add a unit test for?

@mjauvin
Copy link
Member Author

mjauvin commented May 16, 2024

@LukeTowers What exactly do you want to test ?

@LukeTowers
Copy link
Member

Self relations not causing infinite loops :)

@mjauvin
Copy link
Member Author

mjauvin commented May 17, 2024

How do you test for that? AssertNoInfiniteLoop() ?

🤣

- make sure models using this trait with a relation to self will not
cause infinite loop.
@mjauvin
Copy link
Member Author

mjauvin commented May 17, 2024

I added a unittest and tested outside of this branch and it throws a fatal php error as shown below:

$ ./vendor/bin/phpunit tests/Database/Traits/HasSortableRelationsTest.php
PHPUnit 9.6.19 by Sebastian Bergmann and contributors.

PHP Fatal error:  Allowed memory size of 134217728 bytes exhausted (tried to allocate 262144 bytes) in /var/www/vhosts/wn.r4l.com/winter/storm/vendor/laravel/f
ramework/src/Illuminate/Database/Eloquent/Model.php on line 323
PHP Fatal error:  Allowed memory size of 134217728 bytes exhausted (tried to allocate 262144 bytes) in /var/www/vhosts/wn.r4l.com/winter/storm/vendor/spatie/ra
y/src/helpers.php on line 55

When running under this PR, it runs as expected:

$ ./vendor/bin/phpunit tests/Database/Traits/HasSortableRelationsTest.php 
PHPUnit 9.6.19 by Sebastian Bergmann and contributors.

.                                                                   1 / 1 (100%)

Time: 00:00.057, Memory: 28.00 MB

OK (1 test, 1 assertion)

@mjauvin
Copy link
Member Author

mjauvin commented May 17, 2024

@bennothommo @LukeTowers Do we really need to use phpstan? I lose so much time just trying to figureout/fix bogus issues it is not really helping...

@LukeTowers LukeTowers changed the title Only use model relation definition, no need to instanciate the relation Fix infinite loop when using HasSortableRelations on a model with a self-referencing relationship May 17, 2024
@LukeTowers LukeTowers merged commit fb60938 into develop May 17, 2024
@LukeTowers LukeTowers deleted the fix-has-sortable-relations branch May 17, 2024 19:10
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.

3 participants