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

Remove loadMigrationsFrom method from ServiceProvider. #61

Merged
merged 1 commit into from
Apr 17, 2021
Merged

Remove loadMigrationsFrom method from ServiceProvider. #61

merged 1 commit into from
Apr 17, 2021

Conversation

omerkoseoglu
Copy link
Contributor

No description provided.

@ash-jc-allen
Copy link
Owner

Hi @omerkoseoglu, thanks for the pull request! I'm just wondering what the purpose of it is?

@omerkoseoglu
Copy link
Contributor Author

Hi @omerkoseoglu, thanks for the pull request! I'm just wondering what the purpose of it is?

Hi @ash-jc-allen,

I use a multi-tenant model in the system. In most libraries, migrations are free because they are migrated by the developer. I can move migration files to a new subfolder in migrations folder. I may not need the tables for the package in the main database. I can only use the package in tenant databases. Sorry for my english

@ash-jc-allen
Copy link
Owner

Hi @omerkoseoglu, unfortunately at the moment, this isn't something that's really in the scope of the package sorry. However, if it might be something that will be added in the future versions of it to handle situations like these.

@jaulz
Copy link

jaulz commented Apr 16, 2021

@ash-jc-allen any chance to reopen this? 😊 I think migrations should be handled by developers, too, and running "artisan migrate" shouldn't be a big deal for anyone?

@ash-jc-allen
Copy link
Owner

@jaulz It's pretty funny you ask this question, I was actually thinking about reopening this over the weekend. After doing a bit of reading up, I agree with @omerkoseoglu that we probably shouldn't be loading the migrations through the service provider, but instead providing them for publishing.

I'll need to have a quick tinker and update the documentation, but I don't think there'll be any problem with getting this updated in the next few days 😄

@jaulz
Copy link

jaulz commented Apr 16, 2021

@ash-jc-allen cool, sounds promising! 😊 And thanks a lot for this great package!

@ash-jc-allen
Copy link
Owner

@jaulz It's no problem at all, I'm glad that you're liking it. If you've got any suggestions for possible improvements or changes, I'd be happy to hear about them for future updates!

@ash-jc-allen ash-jc-allen reopened this Apr 17, 2021
@ash-jc-allen ash-jc-allen merged commit 104ec8c into ash-jc-allen:master Apr 17, 2021
@ash-jc-allen
Copy link
Owner

Just dropping a quick message here to let you know that these changes have now been included in a new v5.0.0 release (https://github.com/ash-jc-allen/short-url/releases/tag/v5.0.0).

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.

None yet

3 participants