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

[5.4] Fix route bindings #17973

Merged
merged 1 commit into from
Feb 22, 2017
Merged

[5.4] Fix route bindings #17973

merged 1 commit into from
Feb 22, 2017

Conversation

themsaid
Copy link
Member

This is another approach to fix #16926

@taylorotwell taylorotwell changed the title [5.4] Fix route bidnings [5.4] Fix route bindings Feb 19, 2017
@cgoosey1
Copy link

This seems a lot more elegant than my solution in #16926, can't see any issues address in mine that haven't been addressed in this one (though haven't thought through too much the $values[$key - $instancesCount] maintaining the order of non object dependencies.)

* @return mixed
*/
protected function transformDependency(ReflectionParameter $parameter, $parameters, $originalParameters)
protected function transformDependency(ReflectionParameter $parameter, $parameters)

Choose a reason for hiding this comment

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

same feedback as appeared in mine, this can be type hinted :P

Copy link
Member Author

Choose a reason for hiding this comment

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

¯_(ツ)_/¯ :D

Choose a reason for hiding this comment

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

protected function transformDependency(ReflectionParameter $parameter, array $parameters)

@taylorotwell taylorotwell merged commit 544faac into laravel:5.4 Feb 22, 2017
@vlakoff
Copy link
Contributor

vlakoff commented Feb 22, 2017

How about applying this bug fix to the previous, supposedly still supported versions?

@GrahamCampbell
Copy link
Member

It's probably too controversial for earlier versions IMO.

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.

5 participants