[Followup] Replace aggregator rss parsing with Zend Feed

In the Symfony world, everyone uses the Zend_Feed library from Zend Framework.

http://framework.zend.com/manual/2.0/en/modules/zend.feed.introduction.html

I've not used but did review it for a project earlier this year and largely liked it. It does RSS and Atom, and supports Pubsubhubbub.

IMO that would be the most robust way to handle RSS and Atom handling. No need to write our own serializer at all.

(For Atom, we may want to use a serializer for the payload itself, since Atom doesn't specify that. But the Atom parts in particular we should use Zend_Feed for.)

Talked some with the Views folks, too. Based on that, and Lin's comment, I'm going to suggest we refocus this issue to ripping out all existing RSS support in core (we have 2 systems now, including Views), and replacing it with Zend Feed and a single Views display plugin. Revised issue summary coming momentarily.

I have no strong preference regarding using the serializers as a thin wrapper around Zend Feed. Whatever gets the job done easiest. We still need someone to roll the Composer patch for it, though.

I'm not sure about the composer.json codestyle, but this patch pulls in the full zend framework at the moment, see https://github.com/composer/composer/issues/1063 and maybe https://github.com/zendframework/zf2/pull/1934

At least the full zend framework is too big for drupal.org at the moment.

This does not pull the whole framework but zend feed requires a few components:

    "require": {
        "php": ">=5.3.3",
        "zendframework/zend-escaper": "self.version",
        "zendframework/zend-stdlib": "self.version",
        "zendframework/zend-servicemanager": "self.version",
        "zendframework/zend-uri": "self.version",
        "zendframework/zend-version": "self.version"
    },

Patch is pretty big, so maybe pull Zend Feed first and then cleanup views and aggregator?

IMO it's way too late in the release cycle for this and it should be moved to D9.

We chatted about this a bit in IRC. Let's see what catch thinks since he filed the issue. :) D9?

Well, it's not a matter of just adding it--we'd also need to make Views support it, etc. And then the swath of conversion issues.

i just went through this, and i dont think the conversion issues are that many...after we convert taxonomy_term_feed to a view then we can get rid of node_feed, which leaves us with two places to convert:

1. Rss style plugin in views
2. Aggregator

I already started converting aggregator in my sandbox..you know just getting to know zend feed, so even if this doesnt make it, i dont have any problem:)
Here is how awesome aggregator's parser looks after conversion
http://drupalcode.org/sandbox/rootatwc/1969260.git/blob/7f4ffe6:/core/mo...

thats 320 less LOC for me to maintain:)

here is the full list of dependencies:

1. zendframework/zend-version (2.1.5)
2. zendframework/zend-stdlib (2.1.5)
3. zendframework/zend-servicemanager (2.1.5)
4. zendframework/zend-i18n (2.1.5)
5. zendframework/zend-validator (2.1.5)
6. zendframework/zend-escaper (2.1.5)
7. zendframework/zend-uri (2.1.5)
8. zendframework/zend-feed (2.1.5)

thanks to the awesome guys at Zend and Crell for contacting them, the dependency list, just got way smaller:) yay!

The patch in #15 is down to only two dependencies: stdlib and escaper. IMO that's small enough to be acceptable. Huge thanks to MWOP and Padriac from Zend for making that happen!

Awesome, MWOP, Padriac, and Crell ++

I would love to help with this.

Aside:
I notice that this uses ctype_*. I remember that being discouraged before, but it looks like lots of things that we've pulled in now depend on it. Is this documented somewhere?

I am gonna post a patch with the conversion of aggregator parser, if i find time in weekend i ll convert views too

sure, i am not familiar at all yet with serialization module, but i will study it tonight, and see how i can combine them

edit: no i wont be on DrupalCon unfortuantely, but i am on irc most of the time

For my part, I'm happy with whatever gets us using this library fastest. :-)

so, what i fail to understand after going through serialization and rest modules is why someone would want to output an entity alone in rss or atom format? or does it support outputting lists of entities and i couldn't find this possibility?

here is the aggregator conversion
i got stuck in views, so i let someone more familiar to do the job there

@ParisLiakos great work.
This made me smile

 use Drupal\Core\Database\Database;
 use Drupal\Core\SystemListingInfo;
 use Drupal\Core\Template\Attribute;
+use Zend\Feed\Writer\Writer;
+use Zend\Feed\Reader\Reader;

Great patches need interdiffs :D

#23: drupal-zend_feed-1839468-23.patch queued for re-testing.

seems Reader is caching some stuff:)
this should fix tests

thanks jibran for the interdiff!

about views:

i have the writer plugins registered already in views.services.yml
The conversion should probably happen in \Drupal\views\Plugin\views\style\Rss::render() right?
anywhere thats where i got stuck, just leaving notes, might be helpful for someone

Tagging.

Can you post a -for-review-do-not-test patch as well that doesn't include the Zend libs? That will make it easier to review.

sure!

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Plugin/aggregator/parser/DefaultParser.php
@@ -26,353 +29,48 @@
+    foreach ($channel as $item) {
+      // Move the values to an array as expected by processors.
+      $parsed_item['title'] = $item->getTitle();
+      $parsed_item['guid'] = $item->getId();
+      $parsed_item['link'] = $item->getLink();
+      $parsed_item['description'] = $item->getDescription();
+      $parsed_item['author'] = '';
+      if ($author = $item->getAuthor()) {
+        $parsed_item['author'] = $author['name'];
+      }
+      $parsed_item['timestamp'] = '';
+      if ($date = $item->getDateModified()) {
+        $parsed_item['timestamp'] = $date->getTimestamp();

Since some of these keys are conditionally set, you need to explicitly reset $parsed_item at the start of the loop.

That said, why the translation? Why not just pass it up to the caller directly?

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Plugin/aggregator/parser/DefaultParser.php
@@ -26,353 +29,48 @@
+    // Clear the page and block caches.
+    Cache::invalidateTags(array('content' => TRUE));

Uh, why? We're moving the cache to be short-lived anyway. We need to blast the cache less, not more. :-)

We should also register the Zend feed services in core.services.yml, since core is providing them. Views and Aggregator don't "own" them, and I should be able to use it directly without enabling those modules.

That said, why the translation? Why not just pass it up to the caller directly?

Well, to do that all potential processors/parser plugins should standarize on Zend Feed, dunno if we want that? this way anyone can use whatever implementation sees fit to parse items.

Uh, why? We're moving the cache to be short-lived anyway. We need to blast the cache less, not more. :-)

Well, that was in the old code, as cache_invalidate_tags(), i just used the class directly, so, yay! now the parser is unit-testable!
i would remove it, but dunno if it will bring problems, given that aggregator test coverage is not the best:P

Fixed the other two points!:)

#33: drupal-zend_feed-1839468-33.patch queued for re-testing.

sandbox: http://drupal.org/node/1969260/git-instructions/zend-feed

Just in general this looks really great. It removes like a ton of custom code, even on the other hand the full framework is pretty big.

+++ b/core/lib/Drupal/Component/Bridge/ZfExtensionManagerSfContainer.phpundefined
@@ -0,0 +1,86 @@
+   * @param  string $name

Sorry about the nitpick: there are two spaces after @param

+++ b/core/lib/Drupal/Component/Bridge/ZfExtensionManagerSfContainer.phpundefined
@@ -0,0 +1,86 @@
+    // this is just for performance instead of using str_replace

"This" ... "."

+++ b/core/lib/Drupal/Component/Bridge/ZfExtensionManagerSfContainer.phpundefined
@@ -0,0 +1,86 @@
+  /**
+   * Injects the service container used by this object.
+   *
+   * @param \Symfony\Component\DependencyInjection\ContainerInterface $container
+   *   The service container this object should use.

We could just use @inheritdoc

thanks for the review..fixed above, also merged the fix for the plugins, so that the bridge can manage both reader and writer ones

#39: drupal-zend_feed-1839468-39.patch queued for re-testing.

forgot to update aggregator accordingly

#43: drupal-zend_feed-1839468-43.patch queued for re-testing.

Adapt title.

here is a just for review patch, that i forgot to post in #43

here is the patch with added legal info on some copy pasted code from Zend's ServiceManager

#49: drupal-zend_feed-1839468-49.patch queued for re-testing.

I think the only thing blocking this now is Dries' approval to proceed. Dries, please proceed. :-)

here is a reroll since patch above does not apply anymore

no longer applies, reroll

Just a quick note: at the time the patch was originally created, we were still in RC phase for ZF 2.2.0; at this time, we've actually released 2.2.1 already, and the composer.json should more correctly use the version ">=2.2.0".

Thanks! Let's update that dependency.

thanks mwop!
i guess we can remove the
"minimum-stability": "dev"
bit from our composer.json now too.

This is gonna conflict with #1989230: Update to Symfony 2.3 though, so maybe wait?

Yeah, sounds good to me. Here is a patch just containing the composer.json changes, just to make it easier later on..

symfony update committed, here is a reroll of #54 with the stable zend version

+++ b/composer.jsonundefined
@@ -19,7 +19,8 @@
-    "phpunit/phpunit": "3.7.*"
+    "phpunit/phpunit": "3.7.*",
+    "zendframework/zend-feed": "2.2.*"

I *thought* that we always specify exact dependencies so that the result is stable.

For example, let's say I want to update guzzle, then doing a composer update will not only update guzzle but also zend-feed because there was a new minor version.

*thought* because I thought that we were already doing this but the examples above show that we don't?

yes, this was changed in #1977570: Update third-party vendors and fix composer.json

#60: drupal-zend_feed-1839468-60.patch queued for re-testing.

back to rtbc

I'm sorry, you just can't rtbc your own patches :)

Plus this does update a few deps.

i rerolled a rtbc'ed patch to resolve conflicts, usually thats fine
but anyway

OK, I will then.

Super, thanks. sorry Paris.

Restoring tag.

Can someone update the issue summary explaining what is already done by the RTBC patch, and what follow ups will be needed if this is committed?

.

Before I commit this, I'd love to know what follow-ups are required. Thanks!

Added issue summary with implementation plan.

there is just one direct..the other two where already there
#2003108: Switch Views RSS to use Serializer/Zend
I updated the issue summary

Update isssue summary with newest plan of action

to better explain catch's comment meaning:

Moving on with this patch means:

- Worst case: we ship with 2 serializers and a parser each using its own API.. Which is the status exactly right now... But with this patch we get a much more solid parser for aggregator.
- Best case: this patch makes #2003108: Switch Views RSS to use Serializer/Zend possible, which in combination with views conversions in the issue summary, will unify our RSS handling to Zend Feed.

So nothing to lose here, whatever the outcome of other issues is.

Committed to 8.x.

Let's add change notification.

Change notice added: https://drupal.org/node/2023905

Although it's clearly too late for this issue, I'll retroactively voice my opinion that THIS IS CRAZY.

So we removed 500 lines of custom code in this patch. Yay!

Then we added an external library that is 25,000 lines of new code!? Did we benchmark how much inevitably slower and more resource intensive this is? It seems, like everything else we've done in D8, that this inevitably will lead to slower, more expensive code. Even after we replace the Views code, is there any chance we'd be able to recoup the cost of a bloated library like this?

@catch

I discussed converting the ZfExtensionManagerSfContainer into a real service that proxies the calls to Read::*() with ParisLaikos. It would also hide some of calls that aren't supported since we didn't pull in ZendHttp.

Also, #2025643: Add proxy services to Zend\Reader and Zend\Writer for better DX.

with the calls in _drupal_bootstrap_code:

Overall Summary	
Total Incl. Wall Time (microsec):	270,042 microsecs
Total Incl. CPU (microsecs):	264,000 microsecs
Total Incl. MemUse (bytes):	16,078,680 bytes
Total Incl. PeakMemUse (bytes):	16,176,320 bytes
Number of Function Calls:	31,464

Without them

Overall Summary	
Total Incl. Wall Time (microsec):	257,130 microsecs
Total Incl. CPU (microsecs):	252,000 microsecs
Total Incl. MemUse (bytes):	15,995,624 bytes
Total Incl. PeakMemUse (bytes):	16,094,304 bytes
Number of Function Calls:	31,264

I am not sure where else to set the serviceManager though..the proxy solution, we discussed with twistor is maybe the best one

Assuming Feeds module survives to Drupal 8, I'm sure it would want to leverage these services without enabling Aggregator and its UI. Having a service in the DIC has virtually no overhead at runtime unless it's used.

lets just do that..it might not be DX frendly, but it restores performance till me or twistor finds time to work on the proxy

Here's a really quick sketch.

The issue isn't where the service is defined as much as when we set the extension manager. That said, if we define the service in aggregator, then any module that wants to use it will have to depend on aggregator, or redefine it :/

This definitely needs work, I ignored the writer for now. ZfExtensionManagerSfContainer should be broken up into just a bridge class that is extended/proxied in separate classes for the Reader and Writer.

This also includes part of #2025643: Add proxy services to Zend\Reader and Zend\Writer for better DX so that we can avoid the reset calls.

Bah, cross post.

It just dawned on me that we could expose Zend\Feed\Reader\Reader directly as a service, and call setExtensionManager() from there. But, that seems even uglier. Either way we're faking a static class as an object. The proxy approach gives us a little more control and doesn't hide the facts as much as just using Reader as an object in the container. Either way *should* work though. It also allows us a place to document the methods we actually support as I don't think there are any plans to implement bridges for Zend\Http and Zend\Cache

Ok, this is pretty fleshed out.

As an API consumer, this is how I would like things to look. It allows me to easily register new extensions (which I already have some planned) and easily discover the supported methods. It took quite a bit of time trawling through Zend\Feed for me to get this far.

As to where the service should be defined. It's not just Aggregator and Feeds. It's any module that consumes or creates feeds. I'm not sure how many there are in contrib ATM, but I know there's more than a few. Each module would have to copy/paste 65 lines of Yaml. This also duplicates objects in memory, if for example, you are running Aggregator and Feeds on the same site. During cron both modules would pull in duplicate dependencies. Not a huge deal, granted.

@Crell, Feeds is already ported.

ok, we should get proxy to #2025643: Add proxy services to Zend\Reader and Zend\Writer for better DX, this issue is start getting huge. lets go forward with the simple solution here

just rerolling