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

✨ Support laravel-mongodb v4.x namespace #1144

Merged
merged 2 commits into from
Aug 23, 2024

Conversation

jsrodas-pdpaola
Copy link
Contributor

Summary

Packages:

"mongodb/laravel-mongodb": "^4.7",
"rebing/graphql-laravel": "^9.2",

This pull request addresses an issue encountered when using $query->select($fields->getSelect()). Without this fix, the result of the query is incorrect, as it fails to properly handle the selection of fields when working with the latest version of laravel-mongodb.

The issue arises due to the change in the namespace from Jenssegers\Mongodb\ to MongoDB\Laravel\ in the latest version 4 of laravel-mongodb. This change affects the way fields are selected and retrieved in queries, leading to discrepancies in the output.

Example Output Before the Fix

When executing the following code:

$query->select($fields->getSelect());

//  The output of dd($fields->getSelect()) is as follows:

array:15 [
  0 => "users._id"
  1 => "users.active"
  2 => "users.email"
  3 => "users.name"
  4 => "users.password"
  5 => "users.created_at"
  6 => "users.updated_at"
]

Example Output After the Fix

With the fix applied (also support MongoDB\Laravel\Eloquent\Model), the output is corrected to:

array:16 [ 
  0 => "_id"
  1 => "active"
  2 => "email"
  3 => "name"
  4 => "password"
  5 => "created_at"
  6 => "updated_at"
]

Type of change:

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update
  • Misc. change (internal, infrastructure, maintenance, etc.)

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.

After digging a bit around, my understanding is that:

Therefore I think: it doesn't make sense to have a check for both namespaces and it should just be changed.

WDYT?

@jsrodas-pdpaola jsrodas-pdpaola force-pushed the fix-namespace-mongodb-packages branch 2 times, most recently from 86c74c9 to 8b87a50 Compare August 23, 2024 06:53
@jsrodas-pdpaola jsrodas-pdpaola force-pushed the fix-namespace-mongodb-packages branch from 8b87a50 to e0615ed Compare August 23, 2024 06:54
@jsrodas-pdpaola
Copy link
Contributor Author

Hi @mfn , I totally agree with what you said, I made the change only leaving MongoDB\Laravel\, thanks for your comments.

@jsrodas-pdpaola jsrodas-pdpaola requested a review from mfn August 23, 2024 07:03
@mfn mfn self-assigned this Aug 23, 2024
@mfn mfn merged commit 561a0c7 into rebing:master Aug 23, 2024
17 checks passed
@mfn
Copy link
Collaborator

mfn commented Aug 23, 2024

Thanks for the PR!

@jsrodas-pdpaola
Copy link
Contributor Author

@mfn Thank you very much for your promptness! 👍

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.

2 participants