Problem/Motivation
We are getting the following error from the aggregator module, when trying to import new items from a XML feed.
Zend\Feed\Reader\Exception\RuntimeException: Could not load extension: GooglePlayPodcast using Plugin Loader. Check prefix paths are configured and extension exists. i Zend\Feed\Reader\Reader::registerExtension() (line 590 af /.../vendor/zendframework/zend-feed/src/Reader/Reader.php).
As far as I can see it's related to the zend-feed component from the Zend Framework. The zend-feed component was updated on the 24th of May and the error could be related to that update as it concerns the Google Play Podcasts DTD.
https://github.com/zendframework/zend-feed/releases/tag/release-2.10.0
Does anyone have a clue of what to do about this? Is this an issue in the aggregator module or in the zend-feed component?
Is there a way to downgrade the zend-feed component to a previous version temporarily?
Proposed resolution
The problem is caused by the fact we maintain a list of all the Zend extensions in core.services.yml. This list can change as Zend updates - which with a composer driven workflow we are not 100% in control of which version different sites are on. Fortunately in 24 Feb 2015 Zend-Feed added some standalone extension managers that make this much easier to solve.
Remaining tasks
User interface changes
None
API changes
None
Data model changes
None
Comment | File | Size | Author |
---|---|---|---|
#46 | zend-2976335-46-8_5.patch | 6.56 KB | xjm |
#35 | 2976335-35.patch | 6.56 KB | alexpott |
#35 | 28-35-interdiff.txt | 1.54 KB | alexpott |
#28 | 2976335-27.patch | 5.82 KB | alexpott |
#28 | 26-27-interdiff.txt | 8.57 KB | alexpott |
Comments
Comment #2
Erik FrèrejeanThis is cause by the new reader/writers in the zend extension that aren't added to the Drupal service definition yet.
Comment #3
Erik FrèrejeanAdded a patch, but this is more a base system issue then an aggregator module since the issue arises from the zend bridge.
Comment #4
HenrikBak CreditAttribution: HenrikBak commentedI have applied the patch to our website (Core 8.5.3) and our aggregator import is working as expected again. Thanks!
Comment #5
liesm CreditAttribution: liesm commentedEncountered the same issue and the patch in #3, resolves the issue.
Comment #6
catchBumping 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.
Comment #7
alexpottThen we need to bump the version of Zend to actually have these classes.
From composer.lock:
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?
Comment #8
Erik Frèrejean@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 thezendframework/zend-feed
library and not 2.7.0 as defined in the Drupal core composer.lock file.Comment #9
alexpott@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.
Comment #10
alexpottChanged 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.
Comment #11
alexpottAdded CR https://www.drupal.org/node/2979042
Comment #12
alexpottUpdated patch to have CR in deprecation messages.
Comment #13
andypostduplicated "See http"
Comment #14
alexpottThe 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.
Comment #15
alexpottThanks @andypost - fixed.
Comment #16
andypost"zendframework/zend-feed": "^2.4"
we need to bump it to 2.9.1 at leastsee #2566163-10: zend-feed causing installation to halt
Comment #17
alexpott@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.
Comment #18
andypostCr filed & good to go
Comment #19
alexpottIt'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
Comment #20
Erik FrèrejeanI'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.
Comment #21
catchNot quite committing yet but just to say #15 looks great.
Comment #22
ghalenir CreditAttribution: ghalenir commentedI applied the patch from #15 but I still see this issue.
Comment #23
alexpott@ghalenir did you rebuild the container after applying the patch?
Comment #24
alexpott@ghalenir actually where is the GeoRSS coming from? It doesn't appear to be part of https://github.com/zendframework/zend-feed
Comment #25
alexpottTalked to @ghalenir in Slack. Discovered that this approach breaks the feeds module :( Because the feeds module registers it's own extensions by doing
We can't break this.
Comment #26
ghalenir CreditAttribution: ghalenir commentedI removed the #15 patch from the composer and added this
composer require --dev zendframework/zend-feed:2.7.0
It worked now.Comment #27
alexpottHere'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.
Comment #28
alexpottSo... the
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.
Comment #29
Erik FrèrejeanI'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.
Comment #30
alexpott@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.
Comment #31
Erik FrèrejeanOur 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.
Comment #32
Erik FrèrejeanCreated a followup issue to mark the services as deprecated. #2979588: Deprecate Laminas\Feed reader and writer services
Comment #33
larowlanwould 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 exceptionComment #34
alexpottThrowing the exception maintains the current behaviour. I think that it is the right thing to do. Here's the current get method:
Comment #35
alexpottLet's add a test for the behaviour.
Comment #36
catchJust to say #35 looks fine to me, and thanks Erik Frèrejean for opening the follow-up.
Tests still running..
Comment #37
mwop CreditAttribution: mwop commentedJust 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.
Comment #38
catch@mwop thanks for the heads-up, that release is now here: https://github.com/zendframework/zend-feed/releases/tag/release-2.10.2
Comment #41
catchLooks like that was a test bot hiccup, retested and restoring previous status.
Comment #42
catchCommitted/pushed to 8.7.x and cherry-picked to 8.6.x. Thanks!
Comment #45
xjmComment #46
xjmTesting an 8.5.x backport since this is a critical.
Comment #48
mandclu CreditAttribution: mandclu commentedI 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.
Comment #49
xjmThe 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.
Comment #50
catchThis 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.