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

Use route function everywhere #1568

Merged
merged 11 commits into from
Jul 22, 2018
Merged

Use route function everywhere #1568

merged 11 commits into from
Jul 22, 2018

Conversation

asbiin
Copy link
Member

@asbiin asbiin commented Jul 14, 2018

No description provided.

@asbiin asbiin requested a review from djaiss July 14, 2018 16:06
@asbiin
Copy link
Member Author

asbiin commented Jul 14, 2018

@djaiss every references to url are now handle by route function (except vor Vuejs or js urls ...). Named route in web.php are now grouped with Route::name function.

Route::post('/people/{contact}/reminders/store', 'Contacts\\RemindersController@store')->name('store');
Route::get('/people/{contact}/reminders/{reminder}/edit', 'Contacts\\RemindersController@edit')->name('edit');
Route::put('/people/{contact}/reminders/{reminder}', 'Contacts\\RemindersController@update')->name('update');
Route::delete('/people/{contact}/reminders/{reminder}', 'Contacts\\RemindersController@destroy')->name('delete');
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@djaiss Is the problem you described always here (Special route to delete reminders) ? I replaced the route, and all is OK.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@asbiin really? Well if it works now, awesome.

@djaiss
Copy link
Member

djaiss commented Jul 16, 2018

@asbiin is there a main benefit for this change? I do remember on Heroku having a problem with named routes for an unknown reason.

@asbiin asbiin temporarily deployed to monica-team-pr-1568 July 16, 2018 18:38 Inactive
@asbiin asbiin temporarily deployed to monica-team-pr-1568 July 16, 2018 18:56 Inactive
@asbiin
Copy link
Member Author

asbiin commented Jul 16, 2018

@djaiss The (major) benefit for this change is ... clean code, beautiful aligned code. :D

@asbiin
Copy link
Member Author

asbiin commented Jul 16, 2018

BTW it's working, even on heroku

@asbiin asbiin temporarily deployed to monica-team-pr-1568 July 21, 2018 06:53 Inactive
@djaiss
Copy link
Member

djaiss commented Jul 22, 2018

@asbiin alright! If it works, let's do this.

@asbiin asbiin merged commit 0ec0e85 into master Jul 22, 2018
@asbiin asbiin deleted the fix/route-function branch July 22, 2018 19:04
@github-actions
Copy link

github-actions bot commented Feb 5, 2021

This pull request has been automatically locked since there
has not been any recent activity after it was closed.
Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 5, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants