Skip to content

Conversation

@yovanoc
Copy link
Owner

@yovanoc yovanoc commented Jan 5, 2019

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

Before merging, you should:

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

If you want help with your upgrade, check out the Shift Human Services or join the Shifty Coders Slack room to get answers to all your Laravel questions.

The Laravel framework adopts the PSR-2 coding style in version 5.1.
Laravel apps *should* adopt this coding style as well. Read the
[PSR-2 coding style guide][1] for more details and check out [PHPCS][2]
to use as a code formatting tool.

[1]: https://github.com/php-fig/fig-standards/blob/master/accepted/PSR-2-coding-style-guide.md
[2]: https://github.com/squizlabs/PHP_CodeSniffer
@yovanoc
Copy link
Owner Author

yovanoc commented Jan 5, 2019

⚠ The following files reference Laravel facades through the global namespace. For example, you're referencing \DB or importing use DB. Instead import Illuminate\Support\Facades\DB and reference DB.

While global references are allowed through aliases, you should import the facade explicitly. This can improve clarity not only for developers, but static analysis used by your IDE.

  • app/Jobs/UploadImage.php
  • app/Jobs/UploadVideo.php

@yovanoc
Copy link
Owner Author

yovanoc commented Jan 5, 2019

⚠ 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/ChannelSubscriptionController.php
  • app/Http/Controllers/EncodingWebhookController.php
  • app/Http/Controllers/VideoCommentController.php
  • app/Http/Controllers/VideoVoteController.php

@yovanoc
Copy link
Owner Author

yovanoc commented Jan 5, 2019

⚠ 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.

  • app/Http/Controllers/Auth/RegisterController.php

@yovanoc
Copy link
Owner Author

yovanoc commented Jan 5, 2019

⚠ Shift found opportunities to use the built-in Blade directives like @auth, @csrf in the following views:

  • resources/views/auth/login.blade.php
  • resources/views/auth/passwords/email.blade.php
  • resources/views/auth/passwords/reset.blade.php
  • resources/views/auth/register.blade.php
  • resources/views/channels/edit.blade.php
  • resources/views/layouts/app.blade.php
  • resources/views/layouts/partials/_navigation.blade.php
  • resources/views/videos/edit.blade.php
  • resources/views/videos/index.blade.php
  • resources/views/videos/show.blade.php
  • resources/views/welcome.blade.php

@yovanoc
Copy link
Owner Author

yovanoc commented Jan 5, 2019

❌ Shift could not run composer install for your application. As the analysis for lines of code are unavailable.

@yovanoc
Copy link
Owner Author

yovanoc commented Jan 5, 2019

⚠ 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
  • config/auth.php
  • config/filesystems.php

@yovanoc
Copy link
Owner Author

yovanoc commented Jan 5, 2019

ℹ If you have manually upgraded your app from previous versions of Laravel, you should run the Laravel Linter to ensure your app is fully upgraded to the latest version of Laravel.

@yovanoc yovanoc self-assigned this Dec 26, 2021
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