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

Relationships defined in parent class + ignore abstract classes #11

Open
vpratfr opened this issue Jul 9, 2018 · 9 comments
Open

Relationships defined in parent class + ignore abstract classes #11

vpratfr opened this issue Jul 9, 2018 · 9 comments

Comments

@vpratfr
Copy link

vpratfr commented Jul 9, 2018

Some of our models extend a common base class which defines a relationship for all of them.

These relationships are not taken into account in the graph.

For instance:

class Participant extends Model {
    public function details() : MorphOne 
}

abstract class ParticipantDetails extends Model {
    public function participant() : MorphOne 
}

class PlayerDetails extends ParticipantDetails {
    // inherits the participant relationship
}

class RefereeDetails extends ParticipantDetails {
    // inherits the participant relationship
}

I think that any abstract class could be safely ignored.

And the inherited relationship should be pulled in each concrete class.

@mpociot
Copy link
Member

mpociot commented Jul 9, 2018

I’m using PHPs reflection class to list the class methods. This should also return all inherited methods.
To determine if a method is a relationship, I need to evaluate the return type. So I’m actually invoking every method. Maybe the method returns an error?
Can you try and add a simple “dd” call in one of the relationship methods to see if they do get called?

@vpratfr
Copy link
Author

vpratfr commented Jul 9, 2018

I can check that later in the week. What I have tried before posting the issue: I copy/pasted the base method from the base class in the child classes. And that worked properly.

@mpociot
Copy link
Member

mpociot commented Jul 15, 2018

Did you find some time to test this?

@vpratfr
Copy link
Author

vpratfr commented Jul 17, 2018

Coming back from holiday. Will try to make a simple case to reproduce this before July end.

@vpratfr
Copy link
Author

vpratfr commented Jul 19, 2018

I could reproduce this. When putting dd inside the relationship method, it does not get hit when generating the diagram.

@vpratfr
Copy link
Author

vpratfr commented Jul 19, 2018

To reproduce that on a fresh install

A migration to create the required tables

    public function up()
    {
        Schema::create('meetings', function (Blueprint $table) {
            $table->increments('id');
            $table->string('title')->nullable();
            $table->timestamps();
        });

        Schema::create('participants', function (Blueprint $table) {
            $table->increments('id');
            $table->string('full_name')->nullable();
            $table->unsignedInteger('meeting_id')->nullable();
            $table->string('details_type')->nullable();
            $table->unsignedInteger('details_id')->nullable();
            $table->timestamps();
        });

        Schema::create('player_details', function (Blueprint $table) {
            $table->increments('id');
            $table->integer('skill_level')->nullable();
            $table->timestamps();
        });

        Schema::create('referee_details', function (Blueprint $table) {
            $table->increments('id');
            $table->date('nominated_at')->nullable();
            $table->timestamps();
        });
    }

Corresponding model classes

class Meeting extends Model
{
    public function participants(): HasMany
    {
        return $this->hasMany(Participant::class);
    }
}
class Participant extends Model
{
    public function meeting(): BelongsTo
    {
        return $this->belongsTo(Meeting::class);
    }

    public function details(): MorphTo
    {
        return $this->morphTo('details');
    }
}
abstract class Details extends Model
{
    public function participant(): MorphOne
    {
        return $this->morphOne(Participant::class, 'details');
    }
}
class PlayerDetails extends Details
{
}
class RefereeDetails extends Details
{
}

A test to check models are working as expected

    /** @test */
    public function check_model_setup()
    {
        $meeting = Meeting::create();

        $playerDetails = PlayerDetails::create();

        $player = Participant::create();
        $player->meeting()->associate($meeting)->save();
        $player->details()->associate($playerDetails)->save();

        $this->assertCount(1, Meeting::get());
        $this->assertCount(1, PlayerDetails::get());
        $this->assertCount(1, Participant::get());

        $this->assertTrue($player->fresh()->details->is( PlayerDetails::first()));
    }

Diagram that gets generated

image

@vpratfr
Copy link
Author

vpratfr commented Jul 19, 2018

Found the code responsible for this:

->reject(function (ReflectionMethod $method) use ($model) {
                return $method->class !== $model;
            })

That means that all relationships defined in the base abstract get rejected because they are not directly attached to the current class.

@vpratfr
Copy link
Author

vpratfr commented Jul 19, 2018

Removing that is not enough though. Something else is preventing the methods to get classified as relationships.

On top of that method, I think you could safely exit when finding an abstract class.

Or perhaps better, maybe you could represent the class inheritance on the diagram and keep the abstract class.

@ahren-condos-ca
Copy link

Anybody find a work around here?

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

No branches or pull requests

3 participants