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

Upgrade to Laravel 5 #2

Closed
tobyzerner opened this issue Dec 20, 2014 · 15 comments
Closed

Upgrade to Laravel 5 #2

tobyzerner opened this issue Dec 20, 2014 · 15 comments

Comments

@tobyzerner
Copy link
Contributor

Currently Flarum is running on Laravel 4.2. We will want to begin the transition to Laravel 5 sooner rather than later.

I don't know how stable Laravel 5 is at the moment, but I can't imagine that the changes will really affect Flarum's code that much. Can anyone who is more familiar with the changes in Laravel 5 comment?

@mikedugan
Copy link

I've worked on 2 applications built on L5, as with anything it's a rather tedious process of fixing everything when it breaks from a composer update. As of right now, Laravel 5 isn't even alpha, and package loading is currently being restructured, so I would strongly recommend you stick it out with L4 through January (there will be an accessible upgrade path afaik)

@cryptiklemur
Copy link

Honestly, the whole upgrade path of laravel makes me surprised you guys chose it as the base.

@tobyzerner
Copy link
Contributor Author

@mikedugan Alright, thanks, we'll postpone the upgrade till it's stable.

@aequasi From what I gather, the upgrade from L4 to L5 won't actually be that painful at all. It's just at this point, L5 is changing so much that upgrading early causes a lot of pain.

@cryptiklemur
Copy link

@tobscure i meant the upgrade path in general. Laravel. BC breaks, undocumented changes, lack of community additions..... All kind of scary for a project like this

@lancepioch
Copy link

No, don't upgrade, it's not ready yet.

@mikedugan
Copy link

Suggest closing this thread to prevent it from turning into L5 thread :)

@yulianovdey
Copy link

Since L5 is finally out I'll start working on this.

@tobyzerner
Copy link
Contributor Author

Thanks @yulianovdey. I have a few thoughts:

  • We can ditch the laracasts/commander command bus and domain events for the native L5 one (which you've already mentioned :)
  • I wonder if we should refactor the whole Api/Actions thing to be more "native"? Make use of controllers and middlewares and stuff? Haven't thought about this in detail, and I do like the current setup (it's a nice layer of decoupling from Laravel), but I'm open to suggestions.
  • Will also need to work on the flarum/flarum repo and installation instructions. Specifically, workbench is gone.

Let me know if you have any questions.

@yulianovdey
Copy link

@tobscure I like the current setup, it helps enforce simple classes. I don't think there is any advantage to using controllers if you like the way it is now, unless making things more 'native' is worth the extra bit of familiarity new contributors will have.

As far as middleware, it may be a nice way to move your attemptLogin filter to its own class. I gotta look around more to see what else would make sense for it. Either way I'm assuming you want to keep as little as possible in flarum/flarum so we'll need a clean way to register any middleware out of the core. At least the default stuff doesn't even look like it's going to be needed since you're not using (cookie) sessions at all :).

@thangngoc89
Copy link

@yulianovdey It looks like we have to remove everything inside app/Kernel.php

@tobyzerner
Copy link
Contributor Author

@yulianovdey OK, let's keep the current Api/Actions stuff :)

Don't worry too much about all of the auth stuff at the moment. I'm going to implement OAuth2 soon so it's gonna get a makeover. Will probably also make use of cookies somewhat (maybe).

@tobyzerner
Copy link
Contributor Author

Hey @yulianovdey turns out I have been working quite heavily on the Laravel app — mainly auth stuff, and I'm refactoring API Actions so that it's easier to call them from within other routes. Will be pushing soon. Let me know how far along you are (if you've made a start) and I'm happy to help merge.

@yulianovdey
Copy link

@tobscure I haven't started yet, still working on something else. Looking forward to seeing your changes :).

@tobyzerner
Copy link
Contributor Author

@yulianovdey No worries. The refactoring has become a bit of a mess and the next logical step is really to do the L5 upgrade. So I'll be making a start tomorrow!

@solhuebner
Copy link

Great work with the upgrade!

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

No branches or pull requests

7 participants