Skip to content

Conversation

Pr3d4dor
Copy link
Contributor

@Pr3d4dor Pr3d4dor commented May 15, 2020

This pull request adds support for polymorphic relations.

This approach can be optimized.

The problem is that we need to add the morphTo method to the related model on the morphOne and morphMany.

Example, given the following definition:

models:
  Post:
    name: string:400
    relationships:
      morphMany: Image

  User:
    name: string:400
    relationships:
      morphMany: Image

  Image:
    url: string:400
    imageable_id: integer
    imageable_type: string:400

We need to add morphMany to Post and User models, but we also need to add the morphTo method to the Image model. Using this approach we solve the need to add morphTo: null to the definition, like the #202 pull request.

I think that, adding the morphTo relationship before the output, to the referenced model, is a "cheat". But I could not come up with a cleaner way to do this.


Supercedes #202
Closes #199

@Pr3d4dor Pr3d4dor changed the title Add support for polymorphic relations feat: support for polymorphic relations May 15, 2020
@jasonmccreary
Copy link
Collaborator

Thanks. This makes much more sense now.

In seeing the draft, I propose the following syntax:

models:
  Post:
    name: string:400
    relationships:
      morphMany: Image

  User:
    name: string:400
    relationships:
      morphMany: Image

  Image:
    url: string:400
    relationships:
      morphTo: Imageable

This would overcome the morphTo: null issue, yet give Blueprint the information it needs to properly build the relationship. It also prevents the developer from having to remember the format/type of the morph columns.

@Pr3d4dor
Copy link
Contributor Author

@jasonmccreary Thanks, tonight I will refactor this pull request.

@Pr3d4dor
Copy link
Contributor Author

Pr3d4dor commented May 16, 2020

@jasonmccreary I've updated the pull request.

There is one thing that is missing thought, the generated migration for the Image model does not include the imageable_id and imageable_type columns. Do I need to implement this behavior in the MigrationsGenerator?

@jasonmccreary
Copy link
Collaborator

@Pr3d4dor, yes. I would think for whatever models have the morphTo relationships, to use the provided name to create two columns, one for the id and one for the type. Ideally these are right before the timestamps.

@Pr3d4dor
Copy link
Contributor Author

@jasonmccreary I've updated the pull request adding the support for polymorphic relations in migrations.

There is one adjustment that needs to be made: The migration for the table that contains morphTo needs to be before the other ones.

@jasonmccreary
Copy link
Collaborator

Looks pretty good. I have some small changes I'll make.

Why does it need to come first?

@Pr3d4dor
Copy link
Contributor Author

Sorry 😂, I confused myself looking in the Laravel Documentation. It doesn't need to come first.

@jasonmccreary jasonmccreary merged commit 644917e into laravel-shift:master May 20, 2020
@mahfoudfx
Copy link

Thanks. This makes much more sense now.

In seeing the draft, I propose the following syntax:

models:
  Post:
    name: string:400
    relationships:
      morphMany: Image

  User:
    name: string:400
    relationships:
      morphMany: Image

  Image:
    url: string:400
    relationships:
      morphTo: Imageable

This would overcome the morphTo: null issue, yet give Blueprint the information it needs to properly build the relationship. It also prevents the developer from having to remember the format/type of the morph columns.

Hello, I think that it will be very helpful if you could update the official documentation to include polymorphic relationships syntax.

@jasonmccreary
Copy link
Collaborator

@mahfoudfx, totally agree. A bit busy at the moment. Would you be willing to submit a PR to the docs repo to help get this started?

@mahfoudfx
Copy link

mahfoudfx commented Jun 5, 2021

@jasonmccreary , I've submitted a PR. I hope it's correct or require a little changes. Also, I tried also to write something regarding morphToMany and hasManyThrough, but I found nothing and I'm not sure if blueprint handle them.

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.

Polymorphic Relations are not supported
3 participants