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

Enforce PSR-12 and organise imports on Winter CMS core files #1188

Draft
wants to merge 32 commits into
base: develop
Choose a base branch
from

Conversation

bennothommo
Copy link
Member

@bennothommo bennothommo commented Aug 23, 2024

This commit changes the PHP code sniffer to use PSR-12 rules, as that ruleset will pick up a lot of the code style issues being flagged in PRs recently.

In order to set the baseline, all current core files have been reformatted to fit the ruleset.

I have also implemented rules around imports in files - mainly, taking out the aliases and using qualified facades and class names, and stripping out unused imports. This will improve IDE support for PHP intelligence and code completion.

Arrays are also now enforced to have a trailing comma to improve diffs.

Sorry in advance for the code review... 😬

This commit changes the PHP code sniffer to use PSR-12 rules, as that ruleset will pick up a lot of the code style issues being flagged in PRs recently.

In order to set the baseline, all current core files have been reformatted to fit the ruleset.
- Convert all aliases to qualified class names (mainly facades)
- Reorder imports alphabetically
- Ensure last item in array has a trailing comma
- Strip out unused imports
@bennothommo bennothommo changed the title Enforce PSR-12 on Winter CMS core files Enforce PSR-12 and organise imports on Winter CMS core files Aug 23, 2024
@LukeTowers

This comment was marked as outdated.

This will allow us to use the ruleset in plugins as well
With new ruleset, it now passes
This command will allow developers to check code style against the Winter CMS code style rules in both the core and installed plugins.
@bennothommo bennothommo marked this pull request as draft October 11, 2024 00:25
@bennothommo bennothommo added enhancement PRs that implement a new feature or substantial change and removed Status: In Progress labels Oct 11, 2024
phpcs.base.xml Outdated Show resolved Hide resolved
@LukeTowers
Copy link
Member

LukeTowers commented Dec 7, 2024

Initial thoughts from testing on a client project:

Problems:

  • Running on existing project with old phpcs.xml probably won't work because of the reference to winter.demo
  • Running on no file doesn't work because of the dependency on phpcs.base.xml being in the root (solve by moving to system module)
  • Old require-dev is insufficient to run as we require SlevomatCodingStandard rules
  • The scaffolded root phpcs.xml references demo plugin, doesn't exist on most existing projects, also wouldn't cover their plugins, so what is the purpose of a project root level file?
  • bootstrap/app.php, autoload.php fail on existing projects
  • Ton of warnings on core files from default configuration
  • No ability to add --fix flag to switch to phpcbf

Coding style suggestions:

  • Disallow or warn on loose type comparison (== vs ===)
  • Use of overridden Illuminate facades when a winter one exists
    • Config, DB, Event, File, Http (Do not block this one, Laravel one should be what we use going forwards), Mail?, Url?, Validator?
  • Block "use Model;" class alias instead of Winter\Storm\Database\Model (block or warn against all global use imports if possible, or make it easier for IDEs to play nice with them)
  • Not using anonymous classes in migrations
  • Empty methods in Plugin.php (or other clases, perhaps just issue a warning?)
  • Use of routes in Plugin.php instead of in the routes.php file
  • Semicolon on line by itself (usually unintentional, can result from a fix that applies ;;)
  • Warning for .htm files in the backend view directories (example: controllers/*/.htm)
  • Use of dot syntax for implement values (when not using soft implementation @). I.e. Backend.Behaviors.FormController should always be \Backend\Behaviors\FormController::class.
  • Alphabetical order for all second+ level keys in translation files? (i.e. *.*, but root level keys can remain unalphabetized)
  • Warning for $formConfig = 'config_form.yaml' etc (overridden property matches default value).

Enhancement Ideas

  • Switch WinterTest to extend Winter's console command and alias test to it
  • Alias sniff to winter:sniff
  • Add support for --fix option in sniff
  • Evaluate Laravel coding standards for alignment with Winter: https://docs.styleci.io/presets#laravel
  • Add support for --baseline option in sniff to generate a baseline to ignore errors from
  • Add support for module options to match winter:test
  • Make winter VSCode extension auto load or recommend the PHPCS extension
  • Add github action support for auto-fixing files on PRs
  • Provide coding standard as an external repository so we can import it on our packages & plugins
  • Add composer scripts test, sniff, etc

To test

  • Test running with the same package identifiers as test
  • Test running on a new plugin
  • Test phpcsfixer
  • Test output of all scaffold commands for adherance to standards
  • Run sniff & fix on all first party Winter plugins and review changes / errors / warnings from those

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement PRs that implement a new feature or substantial change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants