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

Refactor migrations code to use Laravel as a base #1365

Closed
tobyzerner opened this issue Feb 9, 2018 · 6 comments
Closed

Refactor migrations code to use Laravel as a base #1365

tobyzerner opened this issue Feb 9, 2018 · 6 comments

Comments

@tobyzerner
Copy link
Contributor

In an ideal world, we should really be extending Laravel's migration stuff rather than rolling our own like we are now.

Investigate whether it's possible to use some of the Laravel classes as a base and override methods as necessary.

@franzliedke
Copy link
Contributor

Can you clarify why?

Overriding a moving target like Laravel smells like pain to me. ;)

@tobyzerner
Copy link
Contributor Author

Migrations are something that we don't particularly want to be reinventing the wheel with. We're building forum software, not a PHP framework. So if there's existing code we can make use of, we should strongly consider it. Laravel migrations are fully-featured and at the moment our implementation is very lacking (see #1363). While it may be somewhat of a moving target, I'd say the benefits gained in the stuff we essentially get for free outweighs the potential upgrade pain. I suppose it all depends on how easy it is to "extend" Laravel's code to fit our own purposes in the first place.

@luceos luceos added the Backend label Feb 12, 2018
@luceos
Copy link
Member

luceos commented Feb 12, 2018

Adding your additional logic to the Laravel migrate commands is pretty straightforward, I've been doing so on my tenancy package ever since it was created. Nevertheless I'm unsure we prefer to have the ugliness of extending these classes inside our pretty codebase just to make it compatible with our way of working. If we're not going to adopt the Laravel commands, we at least should spend some time on improving our migration logic.

If we do adopt the Laravel migrate commands, take into consideration that Laravel assumes one single migrations path and the option for a realpath has been added since Laravel 5.6. So take note that we will need to modify the code to be aware about the actual location of migration files (in addition to their package that requires them).

@franzliedke
Copy link
Contributor

If we do adopt the Laravel migrate commands, take into consideration that Laravel assumes one single migrations path and the option for a realpath has been added since Laravel 5.6. So take note that we will need to modify the code to be aware about the actual location of migration files (in addition to their package that requires them).

Isn't that a fairly strong argument that trying to make Laravel's interface work for our use-case is more pain than just doing it ourselves? 😀

@tobyzerner
Copy link
Contributor Author

Potentially!

I suppose it all depends on how easy it is to "extend" Laravel's code to fit our own purposes in the first place.

I'd say one of us needs to investigate this and report back with some hard facts :D

@franzliedke
Copy link
Contributor

IMO, there is nothing else to do here. Rather, we should focus on further separating core and extension migrations, as those have completely different lifecycles. (And frankly, we might not even need down migrations for core, while we do need them for extensions.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants