-
-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
Fixed routes for optimize #11595
Fixed routes for optimize #11595
Conversation
Signed-off-by: snipe <[email protected]>
Signed-off-by: snipe <[email protected]>
Signed-off-by: snipe <[email protected]>
Signed-off-by: snipe <[email protected]>
Signed-off-by: snipe <[email protected]>
Signed-off-by: snipe <[email protected]>
Signed-off-by: snipe <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great! There are some inconsistencies between when we use singular or plurals - I don’t care which we use, or if there are different contexts (e.g. “component.update” versus “components.index” or whatever), I just think it ought to be consistent throughout. If we can clean up those last subtle differences, then our routes list will be clean, and work great!
Signed-off-by: snipe <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One small nit about the logout route naming, but I approve regardless.
This is a pared down version of #11422, that ONLY handles the routes that conflict with
php artisan optimize
(basically, those with conflicting route names).In newer versions of Laravel, since they reworked all of how routes worked, they also started disallowing duplicate route names, even if they were GET vs POST. Our convention had been to keep the route names the same for consistency, but that stopped working when we upgraded Laravel. That is to say, they still worked in the app, but it would return an error if you tried to run
php artisan optimize
- which hey, not everyone does, but it's been impossible to do so since Snipe-IT v6 was released.I closed PR #11422 simply because it was biting off a little more than it could chew. The route names are all over the place, for sure, and we should go back and refactor them for consistency, but that PR will be bigger and messier.
Note: This PR ONLY touches web routes, not API routes, which in general are named less stupidly anyway.