-
Notifications
You must be signed in to change notification settings - Fork 11.6k
DI via constructor for Migration classes #4259
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
Conversation
Shouldn't it just typehint to |
Yeah, and line 60 obviously. There no need to change the service provider part since that is only used on Laravel. |
Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's going on with the spacing here? Doc blocks should have 1 tab, then one space, then a *
, then a space, then text. You are missing that space after the tab. Note when we start with a /**
, we don't need that space,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll fix the missing spaces.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was testing Atom editor but it didn't handle doc blocks well. I overlooked the awkwardly wrong doc blocks. Fixed it! Thanks!
👍 Could you squash this please? :P |
Personally I wonder if you should be using a command or something. I don't particularly understand the need to dependency inject into a migration. You could just use App::make to resolve something in the migration. |
I was surprised that it didn't do auto DI via constructor. |
https://github.com/laravel/framework/blob/4.1/src/Illuminate/Database/Migrations/Migrator.php#L291-L297
It is perfectly doable to apply constructor injection.