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

Slim down default Laravel app #3

Merged
merged 1 commit into from
Apr 30, 2021
Merged

Conversation

MuhammadFarag
Copy link
Contributor

@MuhammadFarag MuhammadFarag commented Apr 30, 2021

  • Remove User model and migrations
  • Remove the default view

Copy link
Collaborator

@paulomarg paulomarg left a comment

Choose a reason for hiding this comment

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

Looking good, made a couple of suggestions!

@@ -6,16 +6,5 @@

Copy link
Collaborator

Choose a reason for hiding this comment

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

Not visible here, but we may not want to extend from Illuminate\Auth\Middleware\Authenticate as I'm sure it will involve other authentication means. We may even want to get fully rid of this class, what do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed it from the middleware, so it shouldn't be called. I kept it as a place holder. I can delete it though 👍

Copy link
Collaborator

Choose a reason for hiding this comment

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

Cool, as long as it doesn't run it probably doesn't matter if it's there!

Comment on lines 41 to 45
Route::prefix('api')
->middleware('api')
->namespace($this->namespace)
->group(base_path('routes/api.php'));

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure we want to remove the api routes - we'll need API endpoints to fetch data once we go through OAuth.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe the reason api is a separate thing here because it has its own security rules etc. I thought we delete it and we add it when we need it.
Now, thinking about it. Laravel users will probably expect that it will be there 🤔

Copy link
Collaborator

Choose a reason for hiding this comment

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

That's true, but Laravel is usually extremely configurable since it has dependency injection for just about anything (and that usually involves a config object 😄 ), so we can probably turn off any features we don't want.

Base automatically changed from new-laravel-app to main April 30, 2021 13:29
@MuhammadFarag MuhammadFarag requested a review from a team as a code owner April 30, 2021 13:29
@paulomarg
Copy link
Collaborator

I think we may need a rebase on this PR so we can move ahead :)

Remove User model and migrations

Remove the default view
Copy link
Collaborator

@paulomarg paulomarg left a comment

Choose a reason for hiding this comment

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

🚀

@MuhammadFarag MuhammadFarag merged commit 0ac9152 into main Apr 30, 2021
@MuhammadFarag MuhammadFarag deleted the remove-unused-components branch April 30, 2021 17:28
paulomarg added a commit that referenced this pull request Jun 6, 2022
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

Successfully merging this pull request may close these issues.

2 participants