Use Zend-Feed's standalone extension managers to prevent sites breaking

This is cause by the new reader/writers in the zend extension that aren't added to the Drupal service definition yet.

Added a patch, but this is more a base system issue then an aggregator module since the issue arises from the zend bridge.

Encountered the same issue and the patch in #3, resolves the issue.

Bumping this to critical because it's a fatal error during normal site operation. However, it's very unfortunate that these need to be registered as services just in case they might get used. #2025643: Add proxy services to Zend\Reader and Zend\Writer for better DX is an old issue but IMO a better long term approach.

Then we need to bump the version of Zend to actually have these classes.

From composer.lock:

            "name": "zendframework/zend-feed",
            "version": "2.7.0",

I'm not entirely sure this is a bug. It's tricky. Somehow @HenrikBak must have upgraded zend-feed to have these classes. If you upgrade a dependency are you responsible for adding the services?

@alexpott, the composer file itself has the version constraint
"zendframework/zend-feed": "^2.4",
So if you manage a project with composer for example using drupal-composer/drupal-project, as suggested in the documentation, you'll get (at the moment) version 2.10.1 of the zendframework/zend-feed library and not 2.7.0 as defined in the Drupal core composer.lock file.

[vagrant@kennismeester]$ composer create-project drupal-composer/drupal-project:8.x-dev some-dir --stability dev --no-interaction
........
  - Installing zendframework/zend-stdlib (3.2.0): Loading from cache
  - Installing zendframework/zend-escaper (2.6.0): Loading from cache
  - Installing zendframework/zend-feed (2.10.1): Loading from cache
  - Installing zendframework/zend-diactoros (1.7.2): Loading from cache

@Erik Frèrejean yep I get that but that still means in order to fix this we either need to declare we are incompatible with 2.10 (not a good option for 8.6.x which we have to fix first) - or bump the version in core/composer.json and composer.lock

I'm in favour of making this a duplicate of #2025643: Add proxy services to Zend\Reader and Zend\Writer for better DX - @Erik Frèrejean / @HenrikBak it'd be awesome if you could try the patch on that issue and see if it fixes your problem. If it does can you close this one as a duplicate or perhaps change this into a task to update Zend-Feed before 8.6.0? Thanks.

Changed my mind after talking to @catch - this issue can proceed as a bug fix. The scope of #2025643: Add proxy services to Zend\Reader and Zend\Writer for better DX is subtly different - it is aiming to improve the API. After doing this issue things get a lot simpler for that one.

No interdiff because new approach and changing to 8.6.x as we have to fix there first.

Added CR https://www.drupal.org/node/2979042

Updated patch to have CR in deprecation messages.

+++ b/core/core.services.yml
@@ -1415,80 +1415,95 @@ 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 See https://www.drupal.org/node/2979042

duplicated "See http"

The standalone extension managers have been around since Zend-Feed 2.4.0 - https://github.com/zendframework/zend-feed/tree/release-2.4.0/src/Reader so we don't need to bump the version in the composer.json file here either.

Thanks @andypost - fixed.

"zendframework/zend-feed": "^2.4" we need to bump it to 2.9.1 at least
see #2566163-10: zend-feed causing installation to halt

@andypost but thats not related to this issue. Yes sure for that issue we need to bump the version but we don't need to fix this specific issue.

Cr filed & good to go

It'd be great to have confirmation from @HenrikBak or @Erik Frèrejean that the current fix works for them. I think it should but obviously we have no explicit test for GooglePlayPodcast

I've been running the patch in #15, on one of our production sites for the last couple of hours and haven't seen any issues so far.

Not quite committing yet but just to say #15 looks great.

I applied the patch from #15 but I still see this issue.

Could not load extension: GeoRSS using Plugin Loader. Check prefix paths are configured and extension exists.

@ghalenir did you rebuild the container after applying the patch?

@ghalenir actually where is the GeoRSS coming from? It doesn't appear to be part of https://github.com/zendframework/zend-feed

Talked to @ghalenir in Slack. Discovered that this approach breaks the feeds module :( Because the feeds module registers it's own extensions by doing

   $result = new ParserResult();
   Reader::setExtensionManager(\Drupal::service(‘feed.bridge.reader’));
   Reader::registerExtension(‘GeoRSS’);

We can't break this.

I removed the #15 patch from the composer and added this
composer require --dev zendframework/zend-feed:2.7.0 It worked now.

Here's an approach that should work with feeds. I've installed the Feeds module and done the exact same code as in #25 and it no longer errors. The patch still gets us to a place where core doesn't need to have all the zend services in the container and we use the standalone extension managers from Zend but we still have our bridge in place. The change is that the bridge sort of decorates a standalone extension manager and uses that first then resorts to the container if it can't find it.

It might be worth considering changing this around so we check the container first. That way if someone has replaced feed.reader.* service in the container we'll still use their replacement.

So... the

+++ b/core/core.services.yml
@@ -1418,77 +1418,100 @@ services:
   feed.reader.atomfeed:
     class: Zend\Feed\Reader\Extension\Atom\Feed
     shared: false
+    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

deprecated: does very little because deprecations are not triggered on Container::get(). This only works when a service is used by ContainerBuilder. For example try \Drupal::service('router.matcher');. The service is deprecated but this won't trigger a message.

Given that deprecating these things here is pretty pointless. I'm also thinking that minimising any BC breaks for a critical fix is a good thing so here's a patch with no deprecations and no BC impact unless you've replaced the feed.bridge.reader or feed.bridge.writer services. I'll update the CR to reflect all of this later.

I've just applied the patch in #28 without an issue, but the feed we're watching won't receive new data before noon tomorrow (UTC), so I'll check back after it has ran for any issues.

@Erik Frèrejean thanks for testing this - looking forward to the results :)

I've updated https://www.drupal.org/node/2979042 to reflect the new patch.

Our feed has just ran and works as expected, new content has been created as usual and no errors in the logs, the CR seems okay as well.

On a side note, I'd feel that deprecating the services at this point (even from just a documentation point of view), makes a lot of sense. But I can follow the logic to not include that in this patch. With that updated the issue description to reflect the fact that there are no deprecations anymore.

Created a followup issue to mark the services as deprecated. #2979588: Deprecate Laminas\Feed reader and writer services

+++ b/core/lib/Drupal/Component/Bridge/ZfExtensionManagerSfContainer.php
@@ -62,14 +68,25 @@ public function __construct($prefix = '') {
+    try {
+      return $this->container->get($this->prefix . $this->canonicalizeName($extension));
+    }
+    catch (ServiceNotFoundException $e) {

would it be more efficient to use $this->container->has() instead of using Exceptions for logic flow?

We even have our own has method we could check first instead of relying on the exception

Throwing the exception maintains the current behaviour. I think that it is the right thing to do. Here's the current get method:

  /**
   * {@inheritdoc}
   */
  public function get($extension) {
    return $this->container->get($this->prefix . $this->canonicalizeName($extension));
  }

Let's add a test for the behaviour.

Just to say #35 looks fine to me, and thanks Erik Frèrejean for opening the follow-up.

Tests still running..

Just FYI: we will be rolling out version 2.10.2 later today, which makes the GooglePlayPodcast extension registration conditional on its presence in the extension manager.

@mwop thanks for the heads-up, that release is now here: https://github.com/zendframework/zend-feed/releases/tag/release-2.10.2

Looks like that was a test bot hiccup, retested and restoring previous status.

Committed/pushed to 8.7.x and cherry-picked to 8.6.x. Thanks!

Testing an 8.5.x backport since this is a critical.

I just ran into an issue that seems related to this, and reported it in #2996277: Aggregator throws Zend Feed errors and won't import. If anyone here has ideas on what could be done to resolve it, I would greatly appreciate it.

The CR https://www.drupal.org/node/2979042 could probably use a little more detail explaining to site owners what the impact of fixing this bug is for them.

This has been out in a release for a while now, and I don't think we can commit this to 8.5.0 any more, so moving back to fixed.