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

Add support for composite keys #1479

Merged
merged 2 commits into from Feb 8, 2024
Merged

Conversation

calebdw
Copy link
Contributor

@calebdw calebdw commented Nov 6, 2023

Fixes #1474, topclaudy/compoships#77

Summary

Hello!

This PR adds support for relationships with composite keys. There have been a couple of issues over the years about this but this still has not been resolved (I don't want to have to extend the Artisan command in my app):

There's even an open issue on the Compoship side of things (topclaudy/compoships#77) but as the maintainer stated, there really nothing to do on that end.

I understand Laravel does not support composite keys, however, people do use them and this package already provides the ability to define non-Laravel relations so it would be much easier to support this rather than to expect everyone who uses composite keys to have to extend the Artisan command with the same custom logic.

Thanks!

Type of change

  • New feature (non-breaking change which adds functionality)

Checklist

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

@calebdw calebdw force-pushed the composite_keys branch 2 times, most recently from 1ed7388 to 5075731 Compare November 6, 2023 20:09
@calebdw
Copy link
Contributor Author

calebdw commented Jan 1, 2024

@mfn, would it be possible to review this PR please?

Thanks!

@barryvdh
Copy link
Owner

barryvdh commented Feb 7, 2024

Can we have a test for this?

Copy link
Owner

@barryvdh barryvdh left a comment

Choose a reason for hiding this comment

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

Can you add some tests?

@barryvdh
Copy link
Owner

barryvdh commented Feb 8, 2024

Do we need that package to test it? Or can we replicate a simple case here?

@calebdw
Copy link
Contributor Author

calebdw commented Feb 8, 2024

I'm working on a test at the moment, I don't think we need the package

@calebdw
Copy link
Contributor Author

calebdw commented Feb 8, 2024

Test has been added

@barryvdh barryvdh merged commit 6115100 into barryvdh:master Feb 8, 2024
33 checks passed
@barryvdh
Copy link
Owner

barryvdh commented Feb 8, 2024

Thanks!

@calebdw calebdw deleted the composite_keys branch February 8, 2024 06:42
@calebdw
Copy link
Contributor Author

calebdw commented Feb 8, 2024

Thank you!!

d3v2a pushed a commit to d3v2a/laravel-ide-helper that referenced this pull request Feb 16, 2024
* Add support for composite keys

* chore: add composite foreign key test
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.

Incorrect PHPDoc being written to model for BelongsTo relation
2 participants