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

Laravel Linter #1

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Laravel Linter #1

wants to merge 1 commit into from

Conversation

elishaukpong
Copy link
Owner

This pull request includes changes and recommendations for crafting your application "The Laravel Way". Feel free to commit any additional changes to the shift-50428 branch.

Before merging, you need to:

  • Checkout the shift-50428 branch
  • Review all pull request comments for additional changes
  • Thoroughly test your application

If you need help with your upgrade, check out the Human Shifts. You may also join the Shifty Coders Slack workspace to level-up your Laravel skills.

@elishaukpong
Copy link
Owner Author

elishaukpong commented Oct 5, 2021

⚠️ The following controllers contain actions outside of the 7 resource actions (index, create, store, show, edit, update, destroy). For more details, review the docs or watch Cruddy by Design to see if you can rework these into resource controllers.

  • app/Http/Controllers/BaseController.php
  • app/Http/Controllers/HomeController.php

@elishaukpong
Copy link
Owner Author

⚠️ Shift found inline validation in the following controllers. Unless you require this level of control, you should use a Form Request to encapsulate this validation logic and keep your controller clean. You may automate this conversion with the Laravel Fixer.

  • app/Http/Controllers/Auth/ConfirmablePasswordController.php
  • app/Http/Controllers/Auth/NewPasswordController.php
  • app/Http/Controllers/Auth/PasswordResetLinkController.php
  • app/Http/Controllers/Auth/RegisteredUserController.php

@elishaukpong
Copy link
Owner Author

⚠️ Previous versions of Laravel converted the second argument passed to the query builder orderBy method to desc when not set to asc. Starting with Laravel 5.8, an InvalidArgumentException is throw.

If you were attempting order by multiple columns, you should instead chain multiple calls to orderBy.

Shift detected the following uses of the orderBy() passing a second argument:

  • app/Repository/BaseRepository.php

@elishaukpong
Copy link
Owner Author

ℹ️ Shift detected controller namespaces being set in your RouteServiceProvider. Laravel 8 began registering controllers using static class references instead of namespace prefixes and controller names.

You may automate this conversion using the Laravel Fixer or tasks within the Shift Workbench.

@elishaukpong
Copy link
Owner Author

⚠️ Shift found the following config files differ from the defaults. While you are welcome to customize your configuration, you should leverage ENV variables rather than hardcode values. If you find you're adding a lot of configuration options, consider creating a domain specific config file, such as core.php. Both will make app upgrades and deployments easier.

  • config/app.php

@elishaukpong
Copy link
Owner Author

ℹ️ Your Laravel application contains 82 classes and 3,400 lines of code.

Most of your classes are controllers. Your controllers contain the most lines of code (474 loc) and your middleware have the highest lines of code per method (5 loc).

@elishaukpong
Copy link
Owner Author

ℹ️ As noted, much of the lint detected above can be automatically fixed using the Laravel Fixer. Save yourself time and clean up your codebase quickly with this new Shift.

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