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

[Bug fix] Fix BelongToMany when using ObjectId in relation #3014

Open
wants to merge 10 commits into
base: 5.x
Choose a base branch
from

Conversation

florianJacques
Copy link
Contributor

@florianJacques florianJacques commented Jun 28, 2024

When we use mongodb ObjectIDs for relationships
In the case of a BelongToMany relation, the foreignKeys was incorrectly cast

{
    "_id" : ObjectId("667e831289f74871500d80a7"),
    "name" : "Jean-Luc Picard",
    "updated_at" : ISODate("2024-06-28T09:32:02.401+0000"),
    "created_at" : ISODate("2024-06-28T09:32:02.401+0000"),
    "planetIds" : [
        {
            "oid" : "667e831289f74871500d80a2"
        },
        {
            "oid" : "667e831289f74871500d80a3"
        },
        {
            "oid" : "667e831289f74871500d80a4"
        }
    ]
}

With the type check, the cast is adapted to avoid converting an object into an associative array.

{
    "_id" : ObjectId("667e91e6bfeefc593308f745"),
    "name" : "Tanya Kirbuk",
    "updated_at" : ISODate("2024-06-28T10:35:18.459+0000"),
    "created_at" : ISODate("2024-06-28T10:35:18.459+0000"),
    "planetIds" : [
        ObjectId("667e91e6bfeefc593308f742"),
        ObjectId("667e91e6bfeefc593308f743"),
        ObjectId("667e91e6bfeefc593308f744")
    ]
}

#3015

Checklist

  • Add tests and ensure they pass
  • Add an entry to the CHANGELOG.md file
  • Update documentation for new features

@florianJacques florianJacques requested a review from a team as a code owner June 28, 2024 10:35
@alcaeus
Copy link
Member

alcaeus commented Jun 28, 2024

Can you please add tests for this change? Thank you!

@florianJacques
Copy link
Contributor Author

florianJacques commented Jun 28, 2024

Can you please add tests for this change? Thank you!

Yes of course when I have a little time.
I missed the sync/toggle functions.
I still have work to do.

@florianJacques
Copy link
Contributor Author

Add Test OK.

@alcaeus alcaeus requested review from GromNaN and removed request for alcaeus July 2, 2024 08:10
@GromNaN
Copy link
Member

GromNaN commented Jul 4, 2024

Sorry for the time it takes to review your PR. This part of the code is complicated and we need to ensure this is feature we can support and maintain for the long run. The more we copy and modify code from the parent class, the more difficult it is to maintain and keep compatibility with multiple versions of Laravel version (even minor version can break). Also, I guess belongsToMany is not the only type of relationship that would need such change.

@florianJacques
Copy link
Contributor Author

Sorry for the time it takes to review your PR. This part of the code is complicated and we need to ensure this is feature we can support and maintain for the long run. The more we copy and modify code from the parent class, the more difficult it is to maintain and keep compatibility with multiple versions of Laravel version (even minor version can break). Also, I guess belongsToMany is not the only type of relationship that would need such change.

No problem, the big change is in the "formatRecordsList" method, which currently assumes the existence of a pivot table.

@GromNaN GromNaN added the fixed label Jul 5, 2024
@florianJacques
Copy link
Contributor Author

Changes in version 4.6 make this modification incompatible. should I make the changes?

@alcaeus alcaeus changed the base branch from 4.8 to 5.x September 12, 2024 08:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants