Adding Scans for Deprecated Function in Core

[aweingarten]

This PR will scan custom code in the standard locations for deprecated functions and fail the build if they are found. This will help the progression to D9!

It should the same patterns as PHPCS or Symfony Security Scans

[mikemadison13]

@aweingarten might make sense to add this to the validate:all (similar to how we did the security one) so it runs as part of the standard validation process, then we can disable parts of it.

[mikemadison13]

also, IMHO, this should be disabled by default.

[aweingarten]

1: Added this to the validate all call.
2: Dumb question how do I disable this off by default? Is that in the build.yml file?
like by adding disable-targets.tests:phpstan

[mikemadison13]

@aweingarten i'm ok with it running, as long as it's not scanning files. so maybe just comment out the file sets by default?

[mikemadison13]

FYI looks like phpcs validation failures on the build failure

[aweingarten]

PHPCS addressed

[aweingarten]

@mikemadison13 okay I think all the feedback has been addressed. Just nailing down one build failure. How do you want this to be socialized/documented?

[mikemadison13]

honestly, we don't have a section on validation in the docs (that i see). we probably need to refactor the testing docs to better differentiate between automated testing and validation that occurs in blt. let me think on this a bit and get back to you.

[aweingarten]

Sounds good. I am thinking that we should also probably think about adding it to the the release notes so that people start enabling it.

Also once we get this turned on for deprecation, increasing the sensitivity of PHPstan to Level 2-7 scans is also freaking awesome. It's really worth considering nudging people as we head towards D9. Checkout the results of the scan for Acquia connector.

acquia-connector-scan.txt

[aweingarten]

So @mikemadison13 I'm an idiot. There is an obvious answer here. What if we make this work like PHPCS where we only scan the files that have changed rather than all the things? That way we gently make the world a better place without immediate whiplash?

[mikemadison13]

@aweingarten i really like that idea, although i would be interested in benchmarking it. i do think that creates some disparity between "why are my changed files getting deprecation scans" and my others ones aren't.

i do think we need this to be on globally or off globally, so maybe the commit scan needs to respect the same rules as the normal scan (just within the changed fileset)

[aweingarten]

So anecdotally the time for our custom files is under 30 seconds. As for the disparity it already exists for PHPCS so I think we are okay.

[mikemadison13]

it does, but re: #3366 i'm trying to change that since commit hooks find different things than blt validate and that is driving me batty.

[danepowell]

The disconnect between pre-commit validation and blt validate is jarring enough already, I don't want to add this in a way that tests might pass blt validate but fail during pre-commit.

Rather, I'd like to see this run alongside all of the other validations during blt validate.

In 10.0.x, it can be enabled by default, if we can get this PR in before 10.0.0 drops. But it should have a message clearly indicating how to disable it when/if it fails.

In 9.2.x, it should be disabled by default (if we decide to backport it to 9.2.x at all).

[aweingarten]

@danepowell
Thanks for the feedback! I added some info to the error message for how to turn off the scans!

The disconnect between pre-commit validation and blt validate is jarring enough already, I don't want to add this in a way that tests might pass blt validate but fail during pre-commit. Rather, I'd like to see this run alongside all of the other validations during blt validate.

Can you elaborate on this? Let me know what you are looking for here. I thought that the phpstan scans were built into blt validate. So the PR is intended to replicate the PHPCS behavior. Are you requesting that we also refactor PHPCS functionality in this PR?

[danepowell]

Think this trailing comma isn't valid JSON

[danepowell]

The problem with PHPCS is that if you pass it a list of files to check (i.e. during git pre-commit hook), it will always scan those files even if they wouldn't otherwise be scanned according to the defined filesets in BLT and/or phpcs.xml.dist.

As long as this filters the file list (i.e. scans an intersection between git changed files and the phpstan fileset) it should be fine.

[danepowell]

Looks like tests are still failing, and there are still some comments from Matt to address.

[aweingarten]

@danepowell I should have time this weekend to devote to this PR!

[aweingarten]

Given the dismissive nature of the maintainer here: #3438 and the lack of support on getting local testing working I'm going to close this PR. It's not worth the effort struggling to make this work.