localgovdrupal / localgov_project

Project template for Drupal 10 sites built with the LocalGov Drupal distribution.
https://localgovdrupal.org
GNU General Public License v2.0
10 stars 8 forks source link

chore: add phpstan strict type checking #153

Closed millnut closed 3 weeks ago

millnut commented 5 months ago

Additional context here: https://github.com/localgovdrupal/localgov/issues/685

  1. Imported bleedingEdge rules to match Drupal core (https://git.drupalcode.org/project/drupal/-/blob/10.3.x/core/phpstan.neon.dist?ref_type=heads#L5)
  2. Set level to 1 to match Drupal core (https://git.drupalcode.org/project/drupal/-/blob/10.3.x/core/phpstan.neon.dist?ref_type=heads#L9)
  3. Added phpstan/phpstan-strict-rules
  4. Updated phpstan.neon to include rule checkFunctionArgumentTypes as per https://www.drupal.org/project/drupal/issues/3404246
  5. Turned off all strict rules from phpstan/phpstan-strict-rules except strictCalls
  6. Set to mirror the results from drupal/upgrade_status
  7. Rename Deprecated check on workflow to PHPStan
rupertj commented 2 months ago

Looks good to me. The errors saying

Class                                                                                     
         Drupal\localgov_step_by_step\Plugin\PreviewLinkAutopopulate\StepBySteps                   
         extends unknown class                                                                     
         Drupal\preview_link\PreviewLinkAutopopulatePluginBase.     

looked like they could be a failure to find all the classes in contrib, but that's the only instance of that kind of thing, and that class is relatively new, so I think it might be a problem with step_by_step rather than the CI setup.

stephen-cox commented 2 months ago
Class                                                                                     
         Drupal\localgov_step_by_step\Plugin\PreviewLinkAutopopulate\StepBySteps                   
         extends unknown class                                                                     
         Drupal\preview_link\PreviewLinkAutopopulatePluginBase.     

looked like they could be a failure to find all the classes in contrib, but that's the only instance of that kind of thing, and that class is relatively new, so I think it might be a problem with step_by_step rather than the CI setup.

This is because we have included some plugins that are defined in a patch on the Preview Link module, but we haven't release the code pulling in the patch because of test failures: https://github.com/localgovdrupal/localgov/pull/730#issuecomment-2176831091

Just looking at potential fixes for this.

millnut commented 1 month ago

Think this is ready to go now.

I've created a PHPStan baseline to ignore the remaining errors for now so we can fix them in future. The reason for this is we are getting more people contributing and this feels more important to get in now so it runs on the workflows and for future code contributions.

millnut commented 3 weeks ago

Yep that makes sense @stephen-cox I've updated the PR to include ignoring Drupal 12 deprecations.

Also correct on the second point, once merged I plan to create issue tickets for everything in the baseline and tag with "good first issue" or similar so over time we can chip away at that file and then remove it.

EDIT: Looks like I need to regenerate the baseline https://github.com/localgovdrupal/localgov_project/actions/runs/10217264291/job/28270808622, leave this PR with me until completed

millnut commented 3 weeks ago

Hi @stephen-cox phpstan baseline updated, test failures unrelated

finnlewis commented 3 weeks ago

It looks like the pull request from @MattOz-CDS here inludes some of the same lines for Drupal 12 deprecations: https://github.com/localgovdrupal/localgov_project/pull/171/files

I note that in that pull request, it mentions renderPlain twice, and specifies the interface and class, where as here we just mention it once and don't specifiy the interface / class.

    - '#Call to deprecated method renderPlain\(\) of interface Drupal\\Core\\Render\\RendererInterface#'
    - '#Call to deprecated method renderPlain\(\) of class Drupal\\Core\\Render\\Renderer#'

Is it just less specific to avoid mention of interface or class?

millnut commented 3 weeks ago

Is it just less specific to avoid mention of interface or class?

@finnlewis yes when I tested recently the message had changed slightly so it wasn't ignoring, this makes it less specific but still allows for it to be ignored