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

fix: dynamic wheres can have a mixed parameter #482

Merged
merged 1 commit into from Mar 3, 2020

Conversation

mr-feek
Copy link
Contributor

@mr-feek mr-feek commented Feb 28, 2020

fixes #475

I'm not sure if I've implemented this properly, but the test passes.

Basically the parameter should be a mixed type, instead of just array when dynamicWhere is invoked via the ForwardsCalls trait on the model

@mr-feek mr-feek force-pushed the dynamic-where-param branch 2 times, most recently from 7c5b213 to 51b6afa Compare February 28, 2020 23:30
@mr-feek mr-feek changed the title fix: dynamic wheres can have a string parameter fix: dynamic wheres can have a mixed parameter Feb 28, 2020
@mr-feek mr-feek force-pushed the dynamic-where-param branch 2 times, most recently from 10b6df0 to 724059d Compare February 28, 2020 23:36
@canvural canvural self-requested a review March 1, 2020 19:04
Copy link
Collaborator

@canvural canvural left a comment

Choose a reason for hiding this comment

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

Hi,

Thank you for the PR! I knew this issue was existing but didn't come around to fix it.

src/Methods/BuilderHelper.php Outdated Show resolved Hide resolved
@mr-feek
Copy link
Contributor Author

mr-feek commented Mar 1, 2020

@canvural are you able to pass an object/array to a dynamic where? Not sure if we can get more specific than mixed

@canvural
Copy link
Collaborator

canvural commented Mar 1, 2020

mixed is okay. We just need to mark it as variadic by passing true as last argument.

@mr-feek
Copy link
Contributor Author

mr-feek commented Mar 1, 2020

done 👍

@canvural
Copy link
Collaborator

canvural commented Mar 1, 2020

Can you also add a test case for multiple arguments?

@mr-feek
Copy link
Contributor Author

mr-feek commented Mar 1, 2020

@canvural I believe I did, lemme know if you had something else in mind

Sent with GitHawk

@canvural
Copy link
Collaborator

canvural commented Mar 1, 2020

Oh, sorry. Now I see it. I guess Github showed me the old changes.

Now I also see the tests are at Models/Relation Oh, sorry. Now I see it. I guess Github showed my the old changes.

Now I also see the tests are at Models/Relations I don't think this is the right place. Maybe we can put it in the Methods/Builder cause its related to query builder and dynamic wheres.

@mr-feek
Copy link
Contributor Author

mr-feek commented Mar 2, 2020

@canvural done

@canvural canvural merged commit ad33c44 into larastan:master Mar 3, 2020
@mr-feek mr-feek deleted the dynamic-where-param branch March 3, 2020 19:36
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.

[False Positive] Dynamic Where's Do Not Require A Parameter Of Type Array
2 participants