Fully deprecate WorkflowDeleteAccessCheck

I believe since this is a tagged service, so the class is instantiated and triggers deprecation issues, even if no code is using the API. We actually already trigger a deprecation in the body of ::access:

/**
   * {@inheritdoc}
   */
  public function access(RouteMatchInterface $route_match, AccountInterface $account) {
    @trigger_error('Using the _workflow_state_delete_access check is deprecated in Drupal 8.6.0 and will be removed before Drupal 9.0.0, use _workflow_access instead. As an internal API _workflow_state_delete_access may also be removed in a minor release.', E_USER_DEPRECATED);
    return parent::access($route_match, $account);
  }

I don't believe you are suppose to use these classes as injected services, rather as references from route definitions. So any actual usage of this class would in fact trigger a deprecation warning, and this will be safe to remove even if it's autoloaded by the access check collector.

Have I missed anything important?

I have just hit this problem in #3094454-6: Fix remaining @deprecated manually and enable the coding standard and have 12 core tests failing for this problem.

We actually already trigger a deprecation in the body of ::access

Yes, that is correct. However, the deprecation messages were not previously being thrown, due to a combination of the way the @deprecated tag was written and how Symfony\Component\DependencyInjection\ContainerBuilder::createService() (in vendor/symfony/dependency-injection/ContainerBuilder.php) detects the deprecation.

Currently WorkflowDeleteAccessCheck has the following:

/**
 * Provides a access checker for deleting a workflow state.
 *
 * @internal
 *   Marked as internal for use by the workflows module only.
 *
 * @deprecated
 *   Using the _workflow_state_delete_access check is deprecated in Drupal 8.6.0

Note the single @deprecated on its own line. In CreateService() there is the code

            if (!$definition->isDeprecated() && 0 < strpos($r->getDocComment(), "\n * @deprecated ") && (!isset($deprecationWhitelist[$id]) || $deprecationWhitelist[$id] !== $class)) {
                @trigger_error(sprintf('The "%s" service relies on the deprecated "%s" class. It should either be deprecated or its implementation upgraded.', $id, $r->name), E_USER_DEPRECATED);
            }

The key bit is strpos($r->getDocComment(), "\n * @deprecated ") which will only match when there is a space immediately after the @deprecated tag. So currently the core tests pass with no deprecation message because this part of the logic returns false. When I corrected the @deprecated tag to follow our standards, i.e. have the version etc following in the same line, that strpos logic check returned true and the deprecation warning was produced, and those tests failed - see https://www.drupal.org/pift-ci-job/1474532 This is the only case where this has happened in the 653 +21 = 674 @deprecated standards fixes which have been done. So it looks like an oddity in how the @deprecated tag got added on its own line.

Not sure how best this can be fixed, but it needs to be done for #3094454: Fix remaining @deprecated manually and enable the coding standard to be completed, which was a follow-up from the major issue #3048498: [≈Nov. 11] Fix Drupal.Commenting.Deprecated coding standard

Cannot add a second parent, but this issue is now listed on #2959269: [meta] Core should not trigger deprecated code except in tests and during updates

To illustrate what happens when the @deprecated tag in WorkflowDeleteAccessCheck is written as per standards, i.e. with text following the tag. This should cause 12 or so test failures.

Here is the coding standard fix in #7 plus addition to the Tests/Listeners/DeprecationListenerTrait

Yes, that is correct. However, the deprecation messages were not previously being thrown, due to a combination of the way the @deprecated tag was written and how Symfony\Component\DependencyInjection\ContainerBuilder::createService() (in vendor/symfony/dependency-injection/ContainerBuilder.php) detects the deprecation.

Wait, we manually call trigger_error in the body of the function, so aren't deprecation errors correctly triggered when the service is actually used?

Yes, that's the problem with event subscriber/tagged services, you can't prevent that they are called, we can only ensure that they are not actually used. Not sure what's the correct thing here.

I'm not a fan of using getSkippedDeprecations() for our own things, it *should* be reserved for third-party components that we can't fix/update yet. I tried deprecating the service too, but all access checks are instantiated in \Drupal\Core\Access\CheckProvider::loadDynamicRequirementMap().

So, I think this really is the best we can do in this case, we do have the @trigger_error() in case someone actually uses it and properly mark it as deprecated. Just one nitpick:

+++ b/core/tests/Drupal/Tests/Listeners/DeprecationListenerTrait.php
@@ -147,6 +147,9 @@ public static function getSkippedDeprecations() {
       'The "locale/drupal.locale.datepicker" asset library is deprecated in drupal:8.8.0 and is removed from drupal:9.0.0. See https://www.drupal.org/node/3081864',
+      // Fully deprecate WorkflowDeleteAccessCheck
+      // @see https://www.drupal.org/project/drupal/issues/3009848
+      'The "workflows.access_check.delete_state" service relies on the deprecated "Drupal\workflows\WorkflowDeleteAccessCheck" class. It should either be deprecated or its implementation upgraded.',
     ];
   }

the current comment isn't very useful, lets just try to describe what I said in my comment? Something like:

"All access check services are instantiated during route rebuilds, an additional deprecation message ensures that it is not used for access checks in routes."

Thanks Berdir. I felt that this was ultimately the way to solve it, but did not have the technical vocabulary or background to explain it like you have.

Attached is a patch with the comment as per your request above. Didn't do an interdiff as it is just that one line altered in DeprecationListenerTrait.php

What's the motivation for this, is it just for consistency or static analysis perhaps? I don't get why we'd trigger two deprecations for this class and silence one of them. Apologies if I'm missing some key bit of info here.

One argument to keep the @deprecated is to officially mark the class as deprecated, although it is arguably very unlikely that someone used this service or class directly.

But yes, the alternative would be to just remove the @deprecated altogether and maybe just keep it as a regular comment in the class docblock (and the service?).

I'd be fine with removing the notice triggering @deprecated comment on the class and replace it with a regular comment given that it's also marked @internal.

OK. This patch replaces the @deprecated tag with a regular comment and does not make any changes to DeprecationListenerTrait.php at all.

I can live with both options, as it's already internal anyway this is probably fine.

Thanks @Berdir. Either #11 or #15 is fine with me. This simple correction will allow me to get back and finish #3094454: Fix remaining @deprecated manually and enable the coding standard

+++ b/core/modules/workflows/src/WorkflowDeleteAccessCheck.php
@@ -11,10 +11,8 @@
- * @deprecated
- *   Using the _workflow_state_delete_access check is deprecated in Drupal 8.6.0
- *   and will be removed before Drupal 9.0.0, you can use _workflow_access in
- *   route definitions instead.
+ *   This is deprecated in drupal:8.6.0 and is removed from drupal:9.0.0.

Hm, why commit this with a format that does not conform to coding standards?

Hm, misread the patch. Moving back to RTBC to undo my misunderstanding. Now read all the comments and it all makes sense.

Did not apply to Drupal 9 anymore as WorkflowDeleteAccessCheck.php does not exist in Drupal 9.0.x anymore. So we don't need to ensure to actually remove this even though it was only marked with a comment. Committed to 8.9.x though :)

I agree undoing the deprecation in our listener or not adding it has the same runtime effect and we have the trigger_error() anyway when its actually used.