Skip to content

Conversation

X-Coder264
Copy link
Contributor

@X-Coder264 X-Coder264 commented Jul 31, 2018

Q A
Branch? 5.7
Bug fix? no
New feature? yes
BC break(s)? yes
Test(s) added? yes
Tests pass? yes

Currently we can type-hint our services in for example the boot method of a service provider, the rules and authorize methods of a FormRequest, in the handle method of jobs, Artisan commands etc. but the same doesn't apply to the migration up and down methods. This is not only inconsistent, but it also means that we have no other choice but to use facades or the global app() helper to get our services from the container. There is no reason why we couldn't get our services in the same way we get them in the methods mentioned above so this PR aims to correct that and in doing so it makes the usage of the Laravel migration system even better / more developer-friendly.

The only breaking change is that the Migrator now has to take a new parameter in its constructor (the container instance). I also added a test to cover this functionality.

@X-Coder264 X-Coder264 force-pushed the inject-services-from-DIC-to-migration-files branch from 954051a to 640a904 Compare July 31, 2018 18:57
@X-Coder264 X-Coder264 force-pushed the inject-services-from-DIC-to-migration-files branch from 640a904 to 04e66e1 Compare July 31, 2018 19:02
@taylorotwell
Copy link
Member

Just use facades or the resolve() helper.

@cwhite92
Copy link

cwhite92 commented Aug 1, 2018

@X-Coder264 what's your use case? Reason I ask is that using a service inside a migration comes off as a code smell, and something you'd probably not want to do. Migrations are meant to be fixed to a point in time and stay immutable, but services will evolve over time as your application changes. If you use a service inside a migration, it could affect the result of your migration, which may break the migrations when you first do a php artisan migrate or php artisan migrate:reset.

@X-Coder264
Copy link
Contributor Author

@taylorotwell The whole point of this MR was that it gives another option to developers so that we aren't forced to use facades or the app/resolve helper.

@cwhite92 I did not plan to inject any app specific services in my migrations, but having the option to inject the Schema builder for example (just like in the test in this MR) instead of having the framework limit and force me to use the Schema facade or the app() helper would be really great. Personally I'm indifferent to the whole "use facades or not", but there are many people who are not fan of facades at all, which includes some people in my company which is also the reason why facades are completely banned on the project I'm currently working on. The migration system is actually the only place where we must make an exception to that rule (and that's basically the reason I made this MR). If we can use facades there to get stuff from the container I don't see any valid reason why method injection which achieves the same thing couldn't be also possible (especially since I already mentioned stuff like that is used on a lot of places throughout the framework).

@X-Coder264 X-Coder264 deleted the inject-services-from-DIC-to-migration-files branch August 5, 2018 22:19
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.

3 participants