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

Route serviceprovider #485

Open
wants to merge 6 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@rjvandoesburg

rjvandoesburg commented Feb 17, 2018

Hello~

In Laravel 5.3 there was a change in how routes were loaded (see: https://laravel.com/docs/5.3/upgrade#upgrade-5.3.0)

However when using this package I'd always find myself re-writing the routes in the modules to reflect the same changes as in the framework.

Thus I decided to create a PR reflecting the changes from the framework.
Old modules will still work as they still load start.php from the module.json.

When running php artisan module:make {name} these are the new changes:

  • A Providers/RouteServiceProvider.php is now created and referenced in Providers/{name}ServiceProvider.php
  • Http/routes.php has been moved to Routes/web.php and is now loaded from Providers/RouteServiceProvider.php
  • start.php is no longer created and referenced in module.json
  • Route cache check in Routes/web.php has been removed as this is handled by the RouteServiceProvider

Updated the tests to reflect the new changes. Now all tests success on my and but because I don't know the ins and outs of the module I might have forgotten something. If so, please let me know so I can fix this.

Now this is all on the basis this is a change you are willing to add to the package of course :)

rjvandoesburg added some commits Feb 17, 2018

Create RouteServiceProvider on module create
- Remove start.php for new modules
- Move Http/routes.php to Module/Routes/web.php folder
- Update RouteServiceProvider to create a new group
- Call module:route-provider on new module creation
Update tests to reflect new changes
- Removed it_generates_start_php_file test as the file is no longer
  generated
@nWidart

This comment has been minimized.

Show comment
Hide comment
@nWidart

nWidart Feb 17, 2018

Owner

Hello,

Thanks for the contribution! Those route changes are optional, however. Where routes are defined is not imposed by the framework.

On top of this, laravel 5.3 is not supported anymore.

In the future try create an issue first in order to discuss changes, and not wasting time on your end.

Owner

nWidart commented Feb 17, 2018

Hello,

Thanks for the contribution! Those route changes are optional, however. Where routes are defined is not imposed by the framework.

On top of this, laravel 5.3 is not supported anymore.

In the future try create an issue first in order to discuss changes, and not wasting time on your end.

@rjvandoesburg

This comment has been minimized.

Show comment
Hide comment
@rjvandoesburg

rjvandoesburg Feb 17, 2018

No it is not imposed by the framework, however I merely meant that in 5.3 there was a change to the routes and this PR applies those same changes to the modules.

rjvandoesburg commented Feb 17, 2018

No it is not imposed by the framework, however I merely meant that in 5.3 there was a change to the routes and this PR applies those same changes to the modules.

@shirshak55

This comment has been minimized.

Show comment
Hide comment
@shirshak55

shirshak55 Mar 25, 2018

actually @nWidart seems correct routes can be defined anywhere as it is referenced in routeserviceprovider. You can read the content of RouteServiceProvider.php

shirshak55 commented Mar 25, 2018

actually @nWidart seems correct routes can be defined anywhere as it is referenced in routeserviceprovider. You can read the content of RouteServiceProvider.php

@rjvandoesburg

This comment has been minimized.

Show comment
Hide comment
@rjvandoesburg

rjvandoesburg Mar 25, 2018

@bloggervista I never stated anything was wrong with the code as it is now. I just felt an update on how routes are defined could be updated. Laravel didn't make the change themselves for nothing.

My opinion was if someone starts a new Laravel project from scratch and then use this module, the way routes are defined would be the same as Laravel allowing them to follow the Laravel docs to continue onwards.

Seeing @nWidart you haven't rejected the PR yet, are you considering this change or should I create and Issue asking people if this is a change they'd like to see?

If you don't plan on accepting the PR, just decline it and don't leave me hanging here.

ps. It would seem referring back to L5.3 only caused more confusion than support for this change :/

rjvandoesburg commented Mar 25, 2018

@bloggervista I never stated anything was wrong with the code as it is now. I just felt an update on how routes are defined could be updated. Laravel didn't make the change themselves for nothing.

My opinion was if someone starts a new Laravel project from scratch and then use this module, the way routes are defined would be the same as Laravel allowing them to follow the Laravel docs to continue onwards.

Seeing @nWidart you haven't rejected the PR yet, are you considering this change or should I create and Issue asking people if this is a change they'd like to see?

If you don't plan on accepting the PR, just decline it and don't leave me hanging here.

ps. It would seem referring back to L5.3 only caused more confusion than support for this change :/

@shirshak55

This comment has been minimized.

Show comment
Hide comment
@shirshak55

shirshak55 Mar 25, 2018

yes i know what you mean.

I also think better to place routes inside routes folder instead of http because i think routes is the first place everybody goes . I guess that was the reason laravel moved routes from app/http/routes to routes/web .

I guess this pr is good :)

shirshak55 commented Mar 25, 2018

yes i know what you mean.

I also think better to place routes inside routes folder instead of http because i think routes is the first place everybody goes . I guess that was the reason laravel moved routes from app/http/routes to routes/web .

I guess this pr is good :)

@nWidart

This comment has been minimized.

Show comment
Hide comment
@nWidart

nWidart Aug 29, 2018

Owner

Hi,

Sorry, it took so long to reply, I wasn't sure if I wanted this change or not.
After reflection, it does look nice.

Could you please fix the conflicts, I think it's easy to do, someone removed the start file before.
I not I'll take care of it.

Thanks.

Owner

nWidart commented Aug 29, 2018

Hi,

Sorry, it took so long to reply, I wasn't sure if I wanted this change or not.
After reflection, it does look nice.

Could you please fix the conflicts, I think it's easy to do, someone removed the start file before.
I not I'll take care of it.

Thanks.

@rjvandoesburg

This comment has been minimized.

Show comment
Hide comment
@rjvandoesburg

rjvandoesburg Aug 31, 2018

Cheers! I'll have a look at them next week :)

rjvandoesburg commented Aug 31, 2018

Cheers! I'll have a look at them next week :)

@rjvandoesburg

This comment has been minimized.

Show comment
Hide comment
@rjvandoesburg

rjvandoesburg Sep 14, 2018

Sorry it took a bit longer but I've fixed the merge conflicts!

rjvandoesburg commented Sep 14, 2018

Sorry it took a bit longer but I've fixed the merge conflicts!

@nWidart

This comment has been minimized.

Show comment
Hide comment
@nWidart

nWidart Sep 14, 2018

Owner

Thanks! Since this is a breaking change, I've created the 3.0, and this will be on master, which will become 4.0 with laravel 5.7 support.

Owner

nWidart commented Sep 14, 2018

Thanks! Since this is a breaking change, I've created the 3.0, and this will be on master, which will become 4.0 with laravel 5.7 support.

@nWidart

This comment has been minimized.

Show comment
Hide comment
@nWidart

nWidart Sep 14, 2018

Owner

Master branch on laravel 5.7 is failing 😢

Owner

nWidart commented Sep 14, 2018

Master branch on laravel 5.7 is failing 😢

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