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 of type hinted scope parameters #415

Merged
merged 10 commits into from
Feb 11, 2020

Conversation

50bhan
Copy link
Contributor

@50bhan 50bhan commented Jan 17, 2020

Add ability of resolving type hinted scope parameters (issue #146)

@50bhan 50bhan requested review from freekmurze and removed request for freekmurze January 17, 2020 15:50
@50bhan 50bhan requested a review from freekmurze February 5, 2020 19:21
@michapietsch
Copy link

@50bhan Are you going to follow up on this? Else I'd like to offer to help.

@50bhan
Copy link
Contributor Author

50bhan commented Feb 10, 2020

@michapietsch Thanks for the offer. Actually I think this feature is done and ready to merge. But doing what is up to @freekmurze and @AlexVanderbist. Maybe there is a policy for adding new features! I would appreciate if they enlighten us.

@AlexVanderbist
Copy link
Member

Hey, my bad fir not following this up sooner. I'll look into this PR when im at the office later today. Thanks!

@michapietsch
Copy link

@50bhan Thanks for your work and sorry for rushing forward

Copy link
Member

@AlexVanderbist AlexVanderbist left a comment

Choose a reason for hiding this comment

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

Great start! Couple of thoughts and nitpicks I've added in the comments. Also, can we add another test that has a scope with both regular parameters and scoped parameters?

Thanks for the PR! 👍

@50bhan 50bhan removed the request for review from freekmurze February 10, 2020 20:46
@50bhan
Copy link
Contributor Author

50bhan commented Feb 11, 2020

@AlexVanderbist All changes have been done. Please check it out and let me know if something else is needed.

@AlexVanderbist
Copy link
Member

@50bhan thanks for the changes, everything looks great.

One final thought regarding the (non-)numeric keys: would it make more sense to use $model->resolveRouteBinding()? This way the parameter resolving works the same way as their model resolving for routes. Any thoughts? (@dominikb feel free to chip in as well :) )

@50bhan
Copy link
Contributor Author

50bhan commented Feb 11, 2020

@AlexVanderbist Thanks. It's way better now.

Copy link
Member

@AlexVanderbist AlexVanderbist left a comment

Choose a reason for hiding this comment

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

Awesome, thanks! Will merge this in a couple of minutes 💯

@AlexVanderbist AlexVanderbist merged commit a5694bb into spatie:master Feb 11, 2020
@50bhan 50bhan deleted the feat/type-hinted-scope-support branch February 11, 2020 10:11
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.

4 participants