[Symfony 6] Service deprecation messages require more fields

Deprecation on a deprecation method? That is very meta :)

Or, we could deprecate the single deprecated key and add separate version and message top level or child keys.

I think we should probably go for this option.

We could try to parse the version out of the message, as we have a standard format? We can't guarantee this will be used but we could fall back to empty string/null if we can't parse it?

This seems like a good bc layer. We could potentially even fall back to the current version of the provider too?

One annoyance here is that there's only version for the since bit of the message, there's no 'will be removed in %version' bit, so it's not compatible with our format (and nor is trigger_deprecation()).

One annoyance here is that there's only version for the since bit of the message, there's no 'will be removed in %version' bit, so it's not compatible with our format (and nor is trigger_deprecation()).

OK thinking about it, this is more of a blocker than an annoyance.

Here's what a service deprecation currently looks like:

  config.storage.staging:
    alias: config.storage.sync
    deprecated: The "%alias_id%" service is deprecated in drupal:8.0.0 and is removed from drupal:10.0.0. Use the "config.storage.sync" service instead. See https://www.drupal.org/node/2574957

vs. the message that trigger_deprecation() generates:
https://github.com/symfony/deprecation-contracts/blob/main/function.php

 @trigger_error(($package || $version ? "Since $package $version: " : '').($args ? vsprintf($message, $args) : $message), \E_USER_DEPRECATED);

So this change means we cannot use the container's deprecation functionality with the deprecation format that's in our coding standards.

This leaves several options:

1. Change the deprecation format to match Symfony's, only for service definitions.
2. Change the deprecation format to match Symfony's everywhere.
3. Re-implement service deprecation in our container subclasses to preserve existing functionality.
4. Try to make trigger_deprecation() more flexible upstream - for example adding a $removed_in version

#1 and #2 we can add the information that trigger_deprecation() is missing to the rest of the message string, but it loses us our current standard format.

It does lose the "removed in" version though, but does that matter? If someone receives a deprecation message they can surely just assume the target version is the next major?

We've moved some deprecations to $next_major_version + 1 in the past, such as #3109480: Properly deprecate theme functions for Drupal 10 and #3113284: Move deprecation of REQUEST_TIME to Drupal 10. There are also issues for drupal check/upgrade status (which I can't find right now) to exclude far-future deprecations like that from reports - so that module authors can focus just on incompatibilities with the upcoming major release.

Having said that, we can put the removed in version in the message string - as long as we can make it fit grammatically, it doesn't have to be done as an arg. And trigger_deprecation() has an arbitrary $args argument for replacements which would allow us to do that elsewhere. Maybe Symfony would be open to adding an $args to setDeprecated() too.

Can we just leave everything as-is and call ->setDeprecation('', '', $message)? $package and $version are allowed to be empty in both the method and the function, does defining them really add any value to us?

Ah! I like this idea very much and think we should at least do that here to unblock Symfony 5/6, we can open up a follow-up for using trigger_deprecation() more generally.

One issue with that is we can't actually make that change until we're on Symfony 5, so it'd need to be a Drupal 10-only patch (although we could still have the patch ready).

I was thinking about reflection maybe, but just checking that method seems fine - it's all internal and just a couple of places.

I've opened a Drupal 10 follow-up, and the 'Symfony 4/Symfony 5' comments are very googleable. Thought about what a longer comment might look like, but it'd just be describing what the code is doing, and we'd need to repeat it three times, what we have seems enough.

I think this is ready to go.

I applied the patch and for me everything is correct, rtbc +1

I discussed the version detection a bit more with @catch - I don't think the method_exists() check is elegant - but I can't come up with anything better so it seems fine.

I wonder though if we should consider changes to support package and version. Upstream YamlFileLoader does this...

                if ('deprecated' === $key) {
                    $deprecation = \is_array($value) ? $value : ['message' => $value];

                    if (!isset($deprecation['package'])) {
                        trigger_deprecation('symfony/dependency-injection', '5.1', 'Not setting the attribute "package" of the "deprecated" option in "%s" is deprecated.', $file);
                    }

                    if (!isset($deprecation['version'])) {
                        trigger_deprecation('symfony/dependency-injection', '5.1', 'Not setting the attribute "version" of the "deprecated" option in "%s" is deprecated.', $file);
                    }

                    $alias->setDeprecated($deprecation['package'] ?? '', $deprecation['version'] ?? '', $deprecation['message']);
                }

Imo we could do:

                    $deprecation = \is_array($value) ? $value : ['message' => $value];
                    $alias->setDeprecated($deprecation['package'] ?? '', $deprecation['version'] ?? '', $deprecation['message']);

To at least support Symfony's expected format for Sf5 (and 6). To me this is helpful because when someone reads Symfony docs about deprecating services then they will work for Drupal too. (Which feels more important than the the actual structure of the message)

+++ b/core/lib/Drupal/Core/Update/UpdateServiceProvider.php
@@ -19,7 +19,14 @@ class UpdateServiceProvider implements ServiceProviderInterface, ServiceModifier
+      $definition->setDeprecated('drupal', '8.8.0', 'The "%service_id%\" service is deprecated. While updating Drupal all caches use \Drupal\Core\Update\UpdateBackend. See https://www.drupal.org/node/3066407');

I think this should be the package - ie. drupal/core

Seems OK to support the Symfony format while not actually using it. So back to RTBC for me.

Committed 47344ca and pushed to 9.2.x. Thanks!