Deprecate Laminas\Feed reader and writer services

I think it's sufficient to point to that CR. Great work @Erik Frèrejean! I'm not setting to RTBC yet as I'd like to get feedback from others that have touched this area. I recently saw a discussion between @catch and @alexpott about this bridge, so I feel that they have more knowledge about this.

+++ b/core/core.services.yml
@@ -1383,67 +1383,88 @@ services:
+    deprecated: The "%service_id%" service is deprecated. You should use the 'feed.bridge.reader' to create a reader instead. See https://www.drupal.org/node/2979042

We should be more specific here and say something like use \Drupal::service('feed.bridge.reader')->get('DublinCore\Entry') instead.

I've updated the change record with a table showing all the deprecated services. This can go in the current published CR because the extension managers are there today and people can update their code already.

We now need to decide in which version of Drupal we are going to do the deprecation and how long we're going to maintain them... see #3088246: [policy, no patch] How to handle Drupal 8.9.x deprecations

Here's a patch that updates the messages to be more specific.

@alexpott pointed out somewhere (I thought it was here but apparently not) that the contrib feeds module uses these services. So given that and the interim decision on #3088246: [policy, no patch] How to handle Drupal 8.9.x deprecations I'm going to move this to 9.1.x

Rerolling for 9.2.x

Adding info about contrib usage to the issue summary.

Patch looks fine but do we need a test? On one hand we usually test everything, but here these aren't used in core, the deprecated service definition key is a Symfony feature, and testing them all individually seems a waste of time.

What I'm a bit perplexed about is why we're not triggering any deprecations. Like why is \Drupal\aggregator\Plugin\aggregator\parser\DefaultParser::parse() not resulting in a deprecation?

Because DefaultParser uses feed.bridge.reader which we aren't deprecating?

Better title. We aren't deprecating the bridge, we are deprecating the individual readers/writers. And it's no longer Zend!

However I see what you mean now, ZfExtensionManagerSfContainer seems like it should be instantiating the services at some point, so why doesn't that happen here?

When you get the service via a kernel test, the deprecation is correctly triggered, but this doesn't happen at runtime in a functional test.

I *think* this is because during a kernel test, $container is a Drupal\Core\DependencyInjection\ContainerBuilder and ContainerBuilders will trigger deprecations when the service is instantiated. But at runtime, $container is a compiled Drupal\Core\DependencyInjection\Container, and this seemingly has no way of triggering deprecations.

@longwave ohhh that's very very interesting! We need to create an issue for that - I think we should issue a deprecation from \Drupal\Component\DependencyInjection\Container::get() or maybe \Drupal\Component\DependencyInjection\Container::createService() if the service is deprecated.

btw does it mean that this services are not instantiated or unused?

@andypost nope it means the calling \Drupal::service() on a deprecated service does not issue a deprecation notice. Only adding a deprecated service as an argument does... ie. deprecations are issued by ContainerBuilder and not Container. /me is jealous of Symfony and it's private by default service wonderland.

So #17 seems fine for testing (with a follow-up for runtime service deprecations somehow) - do we want to add all the other services to the test? Or just not bother...

@catch I think the problem here is that in core we're doing return $this->container->get($this->prefix . $this->canonicalizeName($extension)); in \Drupal\Component\Bridge\ZfExtensionManagerSfContainer but that is not triggering a deprecation because container deprecations only happen on container build. We need to fix that.

Opened #3215611: Service deprecations are only triggered on container build,not ::get().

#3215611: Service deprecations are only triggered on container build,not ::get() landed.

Fixed up coding standard issues in #17, let's see where this explodes.

Swapped around the order in ZfExtensionManagerSfContainer so we check the standalone services first and fall back to the container, I think this is the right (and easiest) solution.

Do we need to deprecate retrieving services from the container entirely?

Not sure I see how to do that, bumping to NW for now.

As MR used

Here's my understanding of the problem:

• We need to keep the services in the container just in case someone is accessing one via \Drupal::service().
• We want them deprecated so we issue a deprecation notice when then happens.
• We can't change the logic in \Drupal\Component\Bridge\ZfExtensionManagerSfContainer::get() because if we do then anything that has decorated one of this services will break.

In order to satisfy the requirements we need to not call $this->container->get() when doing so would result in a deprecation. I think the only way to do this is to add a compiler pass that checks the feed.reader.* and feed.writer.* services, and if the class matches the class defined in core.services.yml, add the service name or extension name to a list stored on \Drupal\Component\Bridge\ZfExtensionManagerSfContainer. Then this list is used to ensure we don't call $this->container->get() for these extension but to calling $this->standalone->get($extension) instead.

I don't think we need to support decorating these services. If we're worried that someone actually is decorating them, we could make this change in Drupal 10 for Drupal 11, but that also seems extremely unlikely.

feed.reader.*: http://grep.xnddx.ru/search?text=feed.reader&filename=

feed.writer.*: http://grep.xnddx.ru/search?text=feed.writer&filename=

Almost no references at all and nothing decorating.

Rebased the branch onto 9.4.x and improved the deprecation message.

I wanted to say that the final fix looks great. Same with the minor clean up in core/lib/Drupal/Component/Bridge/ZfExtensionManagerSfContainer.php

The MR looks good to me. Just 1 remark.

@daffie I think this unnecessary to be honest. The message is not really tested by the test - ie it's not tested for correctness - it's tested for a match. So it's just massive duplication. And if someone changes the message then we'll have to change it in both places. Even the test added here is superfluous because the ability for a service to be deprecated by adding this key is well-tested upstream. What's more important is testing the we don't get deprecations when accessing readers and writers via the bridge and that's tested here.

Extended the test to cover all deprecated feed services.

Crosspost with @alexpott, oh well - it doesn't add a huge amount of extra code, I think this is OK.

@longwave thanks - I guess :) there's no harm in the test but it is not actually testing much more than that we got the deprecation key in the container yaml correct. I guess it is also testing that message format is consistent.

I believe that @daffies last remaining remark in #39 is now taken care of.

+1 for RTBC.

Committed fc90d0c and pushed to 10.0.x. Thanks!

Cherry-picked to 9.4.x too.