Use PHPStan for deprecation checking

So, here once PHPStan will release 1.11.0, they would also be able to make a new release of phpstan/phpstan-deprecation-rules that will include https://github.com/phpstan/phpstan-deprecation-rules/pull/99.

With that, we will then be able to implement a deprecated scope resolver to inform that @group legacy identifies tests running deprecated code.

I plan on implementing that deprecated scope resolver once it is released. The test code for phpstan-deprecation-rules tests the legacy group. Ideally it'd go into phpstan-symfony, but it's faster for us if it goes into phpstan-drupal and doesn't add a new dev dependency.

I have a fix ready: https://github.com/mglaman/phpstan-drupal/pull/595

However, it means making phpstan/phpstan-deprecation-rules a direct dependency of phpstan-drupal, otherwise, the deprecation scope cannot be registered, requiring end-users to do so.

One approach is to add the @group legacy deprecation scope resolver, but don't configure the service and require end users to do so. This avoids enforcing it as a dependency but is bad DX.

Another approach is to merge and release phpstan-drupal 1.2.0. Drupal core can update to 1.2.0 and implicitly inherit phpstan/phpstan-deprecation-rules but disable it from being auto-configured with:

{
  "extra": {
    "phpstan/extension-installer": {
      "ignore": [
        "phpstan/phpstan-deprecation-rules"
      ]
    }
  }
}

I'm not sure the best approach, here.

As discussed in Slack, I am OK with adding a new dependency of phpstan/phpstan-deprecation-rules in a new minor of mglaman/phpstan-drupal, given that it is a dev dependency only, is maintained by the PHPStan team, and we were already considering adding it in this issue.

We can either disable it initially or just accept the baseline increase (as I understand it some of these are currently false positives) when core upgrades to use the new version.

Upstream has been committed. phpstan-drupal (@mglaman) is waiting for confirmation that it's ok to add the dev dependency i guess.

Although we have an OK from longwave, perhaps another ok from someone from core is helpfull?

(I'm all for, but i'm a mere subsystem maintainer)

+1 from me too :)

Great!

Seems an issue to updae the depencency was opened here: 📌 Bump mglaman/phpstan-drupal to latest to make daily "updated deps" QA run pass again Fixed

I have one concern:

https://git.drupalcode.org/project/drupal/-/merge_requests/4641/diffs#65...

This shows up not because the code is actually calling something deprecated, but because the @var needs to be updated to RevisionableStorageInterface. Although... I guess it might also need to check instanceof there too now, so in about five minutes I went from thinking it might be a false positive to thinking it might have found a bug. So... concern withdrawn.

We have mostly stopped adding tests purely to check a deprecation message is triggered, but this should definitely stop us adding any more.

We can either disable it initially or just accept the baseline increase (as I understand it some of these are currently false positives) when core upgrades to use the new version.

IMO let's accept the baseline increase - it'll help us avoid adding redundant tests, and from #38 I'm less confident about spotting false positives now so there's probably benefit to working through them after rather than before enabling the rules.

My spinder sense is tingling, could be wrong though. Didn't diverse into it 🐛 Rearrange interfaces TranslatableRevisionableStorageInterface extends to fix deprecation warnings Fixed

Now that we have added the dependency and baselined the current errors, I think we need to agree how to manage the case of future deprecations where a call to a deprecated method remains in core as a BC layer, temporarily before a new major removes the method for good.

I would suggest to use a @phpstan-ignore-next-line annotation on the line before the call to the deprecated method. Adding to baseline instead would be IMHO not appropriate, additions should not happen in normal issues but only in dependency bumps or PHPStan rule level bumps.

See 📌 Convert select query extenders to backend-overrideable services RTBC for a case where a RTBC issue has just been pushed back because of usage of a deprecated method in BC code.

#41 sounds good.

@catch did say in this issue 📌 Implement utility method for invoking backward compatible code Fixed :

For core there are no core-version-specific bc paths, so it can't use this pattern, but that also means there's no inherent performance implication to adding it, it'd just be about whether a contrib module is using it in the critical path.

Seems that is not entirely true. PHPStan will wil be detecting the pattern in that issue sometime in the near future hopefully. But that's not now.

Personally I vote to use an ignore comment on temporary code paths for now, although we could also consider using the linked issues utility method, but that would only be when phpstan-drupal can 'unscope' the BC path there. Then we might need to consider performance a little more.

I don't think we'd be able to use the new helper for 📌 Convert select query extenders to backend-overrideable services RTBC because it's not version specific - it's running different code based on the existence of a service or not (in case contrib database drivers don't have that service yet).

Yeah fair enough, it would end up being a bit of a conveluted way to ignore a line of code then ;)

So basically, you also say add an ignore?

Suppression in PHPStan baseline:

Pro:
- It's (relatively) easy and automated: run the (ever so easy to remember) vendor/bin/phpstan analyze --configuration=core/phpstan.neon.dist --generate-baseline ./core/phpstan-baseline.neon, commit new baseline, profit.
- Because of the above, there's not much manual labour involved for the current BC-layers.

Con:
- Adding new suppressions should not be the standard, but an exception.

@phpstan-ignore-next-line:

Pro:
- This enforces more thought on keeping deprecated code for BC layers.

Con:
- It does _exactly_ what is said on the tin, just one line is ignored, which can be cumbersome.
- We need through comb through the current present (currently suppressed) BC deprecation-triggering code.

Basically I fully agree with #41.

Disclaimer: Personal opinion, n=1, yada yada...

Yeah I also think add an ignore, it should be pretty rare hopefully, although I guess we'll find out..

Adding my 2 cents:

I think that we should clearly distinguish two different use cases for ignoring PHPStan errors:

- Issues that are waiting for the refactoring => should go to the baseline (upon refactoring changing of the baseline is often required anyways)
- BC layer => should go to the inline comments (when BC layer is being removed - comments will go with the code)

Also, I think inline comments must use error identifiers, so a specific error is being ignored:

If you want to ignore only a specific error, you can take advantage of error identifiers and use the new @phpstan-ignore tag that comes with two features:

- It figures out automatically if you want to ignore an error on the current line or the next line. If there’s no code on the line with the comment, it ignores the next line.
- It requires an error identifier to only ignore a specific error instead of all errors.

Thanks @Taran2L, I wasn't aware of the new @phpstan-ignore which will be available in the not-yet-released PHPStan 1.11 (See https://phpstan.org/user-guide/ignoring-errors#ignoring-in-code-using-ph...).

I like it and your proposal (like it a lot actually), should we wait until phpstan/phpstan1.11.0 is released and use in Drupal core?

In the perfect case we can catch deprecations even at PHP CI https://github.com/php/php-src/pull/10050

That's more about detection of deprecations which coming yearly with new PHP releases but also will help to detect regressions in DrupalCI which still using PHP 8.2.0 but current secure release is 8.2.9

FYI at the moment the language deprecations are caught by PHPCS already, in related it require to fix each usage with
// phpcs:ignore Generic.PHP.DeprecatedFunctions.Deprecated

the assert_options() was just removed in commited 📌 Fix deprecated assert_options() function usage for PHP 8.3 Needs review

phpstan-drupal-deprecations became a dependency of phpstan-drupal with PR https://github.com/mglaman/phpstan-drupal/pull/595 and released in 1.2.0.

Since we're using the PHPStan extension installer, we already have deprecation rules being applied. phpstan-drupal was updated in 📌 Bump mglaman/phpstan-drupal to latest to make daily "updated deps" QA run pass again Fixed .

I think this can be closed in favor of 📌 [Follow-up] In deprecation policy, document adding the deprecation to the phpstan baseline Active

Yes, this is overcome by phpstan-drupal incorporating depercation rules. Let’s close.