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

Implement new config to specify return type of custom relations #1300

Merged
merged 6 commits into from
May 22, 2022
Merged

Implement new config to specify return type of custom relations #1300

merged 6 commits into from
May 22, 2022

Conversation

Jefemy
Copy link
Contributor

@Jefemy Jefemy commented Jan 17, 2022

Summary

Custom Relation types can be added using the config additional_relation_types already, but in the backend the return types for those relations is determined by using a simple check determining if the class name has Many in it to decide the return type of the relation.

This works for the built-in Laravel relations where they are named properly but packages like https://github.com/staudenmeir/eloquent-json-relations use a name such as belongsToJson to return a many relationship. This change allows a config option to specify the actual return type of the relation such as many or morphTo

Type of change

  • New feature (non-breaking change which adds functionality)
  • This change requires a documentation update

Checklist

  • Existing tests have been adapted and/or new tests have been added
  • Add a CHANGELOG.md entry
  • Update the README.md
  • Code style has been fixed via composer fix-style

Copy link
Collaborator

@mfn mfn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, left a minor feedback and can you please also cover not just the 'many' test but also 'morphTo'? (or did I miss it?)

@mfn
Copy link
Collaborator

mfn commented May 3, 2022

Oh, and please also add it to the README; otherwise it's hard to discover for others.

@Jefemy
Copy link
Contributor Author

Jefemy commented May 21, 2022

Oh, and please also add it to the README; otherwise it's hard to discover for others.

I added a test for the morphTo case. I'm not sure where to put documentation on this feature. The config option additional_relation_types which is already required for this to work isn't documented anywhere other than the config file. Should I write up something for that option too?

@mfn
Copy link
Collaborator

mfn commented May 21, 2022

Should I write up something for that option too?

If you can spare the time, this would be terrific! I believe that mentioning things in the README (aka our primary docs) definitely makes things easier to figure out for other developers.

PS: you can ignore the failed tests, I've a separate PR for fixing this in #1351

Copy link
Collaborator

@mfn mfn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yissir, LGTM, thanks for your work!

ping @barryvdh

@barryvdh barryvdh merged commit ee9188a into barryvdh:master May 22, 2022
menthol added a commit to menthol/laravel-ide-helper that referenced this pull request Nov 18, 2022
menthol added a commit to menthol/laravel-ide-helper that referenced this pull request Nov 18, 2022
menthol added a commit to menthol/laravel-ide-helper that referenced this pull request Feb 10, 2023
d3v2a pushed a commit to d3v2a/laravel-ide-helper that referenced this pull request Feb 16, 2024
…yvdh#1300)

* Implement new config to specify return type of custom relations

* Apply suggestions from code review

Co-authored-by: Markus Podar <[email protected]>

* add test for morphed

* fix test output

* add info about how to add custom relationships

* fix wording

Co-authored-by: Markus Podar <[email protected]>
d3v2a pushed a commit to d3v2a/laravel-ide-helper that referenced this pull request Feb 16, 2024
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