New major release - wishlist

JustSteveKing commented 2 years ago

This issue is to keep a log on things the users of this package want. As maintainers we have a few ideas we would like to investigate, however we want to get ideas from people who use this package also so we can test ideas and get ideas to make this package better for all who use it.

JustSteveKing commented 2 years ago

On thing we have weighed up is moving this package to its own organisation, taking the responsibility off of Nuno alone as a personal repo - but also to aid us in future efforts.

JustSteveKing commented 2 years ago

Another option we are currently debating is a plugin ecosystem, where the base phpinights package will do basic custom tests and anything that requires a third party package is a plugin - allowing us to control dependency conflicts internally.

This would also allow users to create their own plugins to use either personally or to open source. The plugin ecosystem would be well documented and would encourage more options and creativity.

alexmakii commented 2 years ago

1+ for plugins, would it allow third-party presets ?

My team enabled phpinsight checks in several projects. We're analyzing almost all php files, with a few exclusions. Based on challenges we've faced I'd propose next additions:

1. Make all insights/sniff/suppress be suppressed

Background: Sometimes we need to disable some checkers for a line/block/method. We didn't want to exclude the whole file, just single statement that we know can't be done other way. We had to spend time looking if a particular checker can be disabled and how. Some checkers didn't have this ability - we've extended them to get things done. Current state: Slevomat sniffs can be disabled, insights can't. I've seen some reported issues about that, related docs topic is a little complicated.

2. Make method/property signature checkers read parent class signature to get full info about subject statement

Background: We have next hierarchy of classes

vendor/BaseModel
   ^ app/AModel
   ^ app/BModel

BaseModel have no native type hints for multiple properties/methods. So don't have type hints in child classes We'll get multiple warnings "no type hint" for AModel, BModel

Solution I Add phpdoc type hints in AModel, BModel Result - lot of copy/paste, lot of work
Solution change hierarchy to
```
vendor/BaseModel
   ^ app/BaseModel # added type hints
      ^ app/AModel # added {@inheritdoc}
      ^ app/BModel # added {@inheritdoc}
```
Result - no warnings, but because {@inheritdoc} blindly mutes any related insights, so you don't know whenever you had other types of issues or not. Especially if you added

From my POV, in this case, best solution would be (phpstorm and phpstan way):

Follow 2nd solution
Don't add ={@inheritdoc}
Let insight - merge current method info with same method info in all parent classes.
Insight checks info against collected data

Current state: We should put duplicate type hints for every single property we used, otherwise we can use {@inheritdoc} and mute any checks, regardless of actual type.

3. Remove hardcoded exclusion of tests

Background: My team enabled phpinsight for all php code, but found that tests aren't checked by default. To achieve this - we had to run phpinsights twice - with default folders and just for tests folder. From my POV tests are just another bunch of code that should be qualitative (type hints added, code style and so on..). In case a project don't want to analyse tests - them can be simply excluded. That should simplify internal code, minimize unpredictable internal flows. As I know, laravel projects tend to use pestphp for tests and and those tests can be hard to fix by phpinsights - tests exclusion can be added to laravel preset. Current state: Test classes are hardly excluded from the analyse, except case the test folder is directly analysed. There's a PR that adds option to disable this behavior.

4. Check if exclusion/paths configs can be re-thinked

Background: Working on a PR for phpinsights, found that it has both paths/exclude configs. I didn't found any docs about paths config and don't know whenever it is obsolete or just not documented. I think that a phpinsight-like tool should either include or exclude files for analyse, not both at the same time. Taking in account that other checkers (phpstan, phpcs, psalm, phpmd) requires explicit including of files for analyse - I'd remove exclude and leave just paths. Current state: paths config have issues and can't be used easily. But, I'd fix it in 1 month, if needed.

5. Replace "common_path" config with "root_path" or similar

Background: I found that common_path is used in two ways: calculate the longest common path to speedup the analyse, lookup for composer.lock. I think that those functionality should use different variables: common path calculable and can't be set by user, root_path can be set and defaults to current working dir. Current state: common_path have dual meaning that can complicate the internal logic of phpinsight.

nunomaduro / phpinsights