Skip to content

Allow has method with numeric indexes #2187

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

Closed
wants to merge 6 commits into from

Conversation

stephandesouza
Copy link
Contributor

Fix #2182

@divine
Copy link
Contributor

divine commented May 7, 2021

@stephandesouza taking look at this PR it can create some regression like this one #2203

No problem, we're here to find the best solution for everyone!

What solution we could create? Any thoughts?

Thanks!

@stephandesouza
Copy link
Contributor Author

This workaround does not modify the query used by whereHas(), only how the IDs are returned by getConstrainedRelatedIds.

And, for now, we can't run away from array_count_values and array_keys. But we can avoid using array_map when no numeric ID is used, with a "hasNumericIndex" variable.

Copy link
Contributor Author

@stephandesouza stephandesouza left a comment

Choose a reason for hiding this comment

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

Done the review.

Copy link
Contributor

@divine divine left a comment

Choose a reason for hiding this comment

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

Hello,

I still think that this fix doesn't make sense at all.

_id: "12345" is a type of string, why this library should take care of logical problems in your project?

Is defining $keyType = 'int'; not enough?

I'm just trying to understand and this type of code that we're still having issues with.

Thanks!

@codecov-commenter
Copy link

codecov-commenter commented May 12, 2021

Codecov Report

Merging #2187 (428eacd) into master (4056a73) will increase coverage by 0.09%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #2187      +/-   ##
============================================
+ Coverage     86.91%   87.00%   +0.09%     
- Complexity      665      669       +4     
============================================
  Files            33       33              
  Lines          1559     1570      +11     
============================================
+ Hits           1355     1366      +11     
  Misses          204      204              
Impacted Files Coverage Δ Complexity Δ
src/Helpers/QueriesRelationships.php 94.28% <100.00%> (+1.06%) 32.00 <0.00> (+4.00)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4056a73...428eacd. Read the comment docs.

@stephandesouza
Copy link
Contributor Author

Sorry, the test was wrong after suggested review.. This is a problem with the "reverse" foreign key used on this method, and also happens with primaryKey as int

Situation:
Author "id:1" has two books Book "id:A", and book "id:B".
Author "id:'2'" has two books Book "id:C"

Today:
getConstrainedRelatedId() returns

["1", "2"]

Fixes:
getConstrainedRelatedId() returns

[1, 2]

And yes, even a "string integer" needs to be converted to int,

@stephandesouza stephandesouza requested a review from divine May 14, 2021 17:44
@divine
Copy link
Contributor

divine commented May 14, 2021

Hello,

I'll take a look tomorrow with a blank project to understand better this issue.

Thanks!

@mongodb mongodb deleted a comment from codecov-io Nov 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

getConstrainedRelatedId method not returning ids as string if key is a numeric string
4 participants