Convert Aggregator feeds into entities

Clean and nice! I'll test it soon.

I tried to apply the patch but I got rejects. Should I reroll the patch or do you plan to do?

Great to see this one being tackled. #4: agree, let's wait until #302936: DB TNG: Convert aggregator.module queries is committed.

This patch also depends on #303930: Pluggable architecture for aggregator.module. Once it is in, feeds as nodes is the next big priority for the aggregator revamp.

Rerolled after the DB:TNG and the pluggable patches.

Subscribing. This is neutral for code weight according to diffstat - although most of the new code is standard hook_* node implementations as opposed to maintaining custom storage.

Is the idea to move aggregator categories to taxonomy as a followup patch? Seems like much of the node hook implementations are for keeping track of categories, so I'd be interested to see if we could strip that out too.

If we decide to drop the classic category stuff, i think the next problem is that the feed items are not nodes, so taxonomy system and classic aggregator items need to be connected somehow.
Taxonomy is basically for nodes. Is there any examples at Drupal core, where taxonomy is connected to some non-node stuff?
I'm really glad if we decide on taxonomy way, but maybe it won't be straightforward for the users. Let's hear the arguments pro and con :)

In that case I reckon our best bet would be waiting for fields in core, and taxonomy as fields - then having aggregator items implement the field API (which should allow for attaching taxonomy vocabularies as fields). Either way, that puts it very much out of scope for this issue.

We didn't really discuss taxonomy as a field. We do plan on having terms be 'fieldable objects' which means that one can attach custom fields to a term (e.g. picture). I would not recommend postponing this patch in favor of fields api.

sub

I think taxonomy as a field makes a lot of sense to me. It seems better to wait for fields in core, but then again, I don't like stalling important work.

Reading my post again I think it's being read the opposite way to how it was meant. IMO this patch could go in as is - but when/if taxonomy becomes a field, we should revisit it again and convert 'categories' to use the taxonomy field - i.e. no need for that to be tackled in this patch.

Great to see this moving. There are several problems with this patch, though:

* It defines its own static node type, there is no way to create additional feed content types. This defeats one important purpose of feeds as nodes: configurable feed types. Aggregator should use a nodeapi hook to dynamically piggy back on any number of content types.
* This is related: feed settings need to go on the content type edit page so that we can create a different setting for every feed content type.
* The 'categorize' tab shows up even if there are no categories defined and the UI on node/x/categorize does not offer the possibility to create a new category. Ideally, either the category tab is hidden when there are no categories to choose from or there is a possibility to create categories on node/x/categorize.
* On a feed's node view, there is no possibility to update the feed's items.
* On a feed's node view, there is no link to a list of the feed's items.

I like that the patch doesn't load the feed node on cron. It would be expensive, it's not necessary for aggregator and if add-on modules need the feed node, they can load it with node_load($feed->nid);

I think that the category issue can and should be decoupled from the one at hand. It introduces a discussion that is actually independent from feeds as nodes. Should we create a new issue?

I talked with Aron Novak about this and concluded that indeed feeds as nodes are a good idea. Most things you can do with a node are applicable to a feed: on a news site you might want to promote it (not just the items, the feed itself). You might want to attach an image (like a screenshot of the site you are following). Even an event could make sense if you are following, well, an event.

Working on #15 now

This patch contains all important changes, works, but tests don't pass.

This is what's done:

* Feed records are linked to nodes
* Feed records don't contain neither title nor description - they live in node and node revision now.
* admin/content/aggregator/add/feed disappears
* Feed configuration moves from admin/content/aggregator/settings to the node type form
* node type form has 'feed content type' checkbox to activate the node type as a feed type.
* A feed lives on $node->feed. For back reference there is a $node->feed->nid, for convenience and performance there is a $node->feed->title in many cases.
* OPML creates feed nodes now.
* UI on aggregator/* adjusted
* Addresses all issues in #15

This is outstanding:

* Fix tests
* Improve UI on aggregator/* - we need a link from aggregator/sources/[id] back to node/[id] - don't know an elegant way yet.
* Validate URL on submission
* Test adding new fetchers, parsers, processors.
* More reviews

I'll be working again tomorrow on the list of outstanding items. Reviews in the meantime more than welcome.

* All tests working

Outstanding:

* Improve UI on aggregator/* - we need a link from aggregator/sources/[id] back to node/[id] - don't know an elegant way yet.
* Validate URL on submission
* Test adding new fetchers, parsers, processors.
* More reviews

Rerolled patch, updated to latest changes.

I'd like to summarize why feeds as nodes are the way to go. Webchick and others have asked me to do so. The basic reasoning behind this patch is clearly missing from this thread, my apologies for overlooking this.

a) Feeds as nodes allow more than one type of feed (think ical, think rss/atom) on a single site by piggy-backing feed configuration on content type configurations. Look at http://drupal.org/project/feedapi to see how many types of aggregator plugins there are. Think for example: RSS, iCal, KML.
b) We get content type permissions and many more node related features for for free (see #16)
c) Feeds as nodes opens the door to using taxonomy on feeds and feed items (albeit with the downside of loosing per item categorization, this is why this is not included in this patch)
d) Feeds can be manipulated through nodeapi hooks.

I am curious to know 2 things since I am deciding on either using Aggregator or FeedAPI for my Drupal 6 multisite setup.

1. The ability to discard feed items is valuable since I plan on running sub-sites and I would rather the user go to the sub-site to get their information as opposed to staying on my portal site. I only want the portal site to host the most recent info from the sub-sites.

2. I like how clicks to the feed headline redirects the user to the original article, the same with the "read more" link. I don't really want users to read the content as if it was on the portal site, I want them to get redirected to the sub-site.

The workflow would look like: Add feeds > Show feed teaser > Redirect to original article > delete article after X days/weeks.

Are these features planning on making it into the "Feeds as nodes" update? I hope they don't get discarded.

Thanks

The ability to click on the title of the node teaser and be directed to the original article instead of the node is a good idea. I don't believe this is currently possible with feeds as nodes but should be.

@specmav and SeanBannister. This patch is about having feeds as nodes, not indvidual feed items - so neither of those concerns are affected.

Well, then I am a little confused about building "feeds as nodes" for a core module. Since using FeedAPI and its extended modules, you can achieve the same results.

What we (ehem..the developers) are doing is duplicating a perfectly good contrib module. What about the folks that just need a lightweight method of dealing with feeds? Granted, I am speaking for myself, but maybe there are others that looked at both FeedAPI and aggregator and found aggregator an easier and simpler approach.

I don't really want to get into a heated debate about it, but wouldn't there be a way for the module to "hijack" the headline output and alter the URL path on feed configured content types (if an option allows it)?

The deletion method could just be as simple as making sure aggregator for drupal 7 ships with the proper actions to allow an admin to set up a time limit to keep the nodes.

The nice thing about aggregator now is that it is a lightweight solution and if the need arises users can upgrade to FeedAPI for a more robust solution.

specmav, I think you're misunderstanding what the patch does. This only makes feeds themselves into nodes - i.e. if I add http://drupal.org/rss.xml to my site as a feed, there'll be a node referencing the feed, with the URL, name, a description (and obviously whatever else you might want to stick onto a node type). What it doesn't do is change aggregator feed item storage - all the items which come from the Drupal front page will still be stored the same way as they were before, automatically expired, lightweight etc. etc.

I think I am misunderstanding. I thought the purpose of this "feeds as nodes" patch was to place each feed item into it's own node.

Sorry it's late were I am and I had a brain fart.

Oh sorry, kinda got side tracked by specmav's post :)

Changes:

* aggregator/sources is a page of node teasers now rather than a list of feed titles with the 3 latest feed items.
* Validates URL on submission
* Fixed block handling
* Fixed some minor UI issues.

This patch is ready for review now. Pay attention to what changed on the UI, especially how the aggregator/ pages flow and the way you configure a feed content type on the admin/build/types pages.

Oops, forgot to cvs add a new file (template for info on feed node pages).

This revision brings back the full validation of feeds as we had them previously: check for duplicate URL, check for duplicate title. I will argue with a separate patch that we don't need this validation and that it doesn't scale, but let's go step by step.

An overview of the changes to Drupal core aggregator:

* To create a feed you create a node of a feed enabled content type.
* One feed content type is installed and configured automatically.
* Feed configuration is now per content-type.
* Feed fid is now nid and ties a feed record to a node.
* UI changes
** admin/content/aggregator/settings can now be found on a content type edit form.
** The UI on aggregator/sources had to change because feed node have their own pages (node/x). This page is not a list of feed titles with the three last feed items anymore, but a teaser list of nodes just like the front page of a vanilla Drupal install.
** Feed statistics moved from the top of aggregator/sources/X to node/X
** aggregator/sources/X holds a link to node/X
** Feed add form does not exist anymore, instead, there is a URL field on a node edit form.

Small cleanups:

* On content type form, keep add on modules on their own spots in the form so that they show up in order.
* No module_invoke call to self in aggregator_processor_node_type_form()

Rerolled to reflect recent changes to core. Still needs work: I'd like to add an API test.

Added API tests, updated API documentation. Ready for review now.

For a summary of changes see #32. For "Why feeds as nodes?" see http://drupal.org/node/397748

I tried it out. It's nice that this patch is really solid now :)
I see some minor problems:

• PHP notices are appearing at preview.
• Feed node loses URL when doing a preview
• aggregator/rss throws a PDOException. None of aggregator-related RSS feeds work.
• Feed images seems to be problematic (does not work for me). I got broken feed image for example at that feed URL:http://downloads.bbc.co.uk/podcasts/radio4/yydisab/rss.xml . But this happens with all BBC feeds.

Here is the updated patch.

#40: when aggregator is uninstalled, feed nodes aren't deleted. Given the problems with deleting many nodes as once, I think that behavior is ok...

That's the same as forum, book and any other module-provided content type in core, so not something which needs to be fixed here.

I did a test run with Aggregator Node Processor and saw that the content type settings form swallowed node processor settings. Fixed.

Also removed the setting for how many items to show with every feed on /aggregator - this is obsolete now, as we show a list of feed node teasers on /aggregator now.

* Clarify node tab label: "Update feed" instead of "Update items".

The idea and the patch look very good to me. I've been giving it a try and everything seems to work fine. Also I've imported some OPML and it seems to work.

However, I think getting rid of aggregator categories should be also part of the same patch. Otherwise, once update scripts added, we are going to have first the ones to change the categories table, then the ones to get rid of it.

My question: isn't just easier to replace categories with taxonomy than changing these old categories bits to work with the nodes? Is there any reason why we want to still keep categories or do it on two steps/patches?

Some other tips:
- Why can't we have them under node revisions (vid) ?
- Now we have the full node workflow, we should refresh on cron only the ones that are set to published or maybe handle it some other way, but there should be some workflow for this.
- As these are now nodes, the node permissions should apply for feed listings.
- There could be more validation for the feed URL, i.e. checking that it is really a feed.

So as I've said, the patch looks great, works fine and these tips can be considered mostly new features. Just when we think of the final goal, will it be easier first committing this (with update scripts), then doing the 'get rid of categories' patch (with more update scripts) or doing all of it in the same patch?

Jose, thanks for review.

The category stack doesn't change much with this patch, so while it's very tempting to change them together with this patch, I really think we should keep it as a separate issue. The obvious solution would be to remove categories from aggregator altogether and to replace them by taxonomy. Taxonomy will support categorization on a feed level, but not on a feed item level. I personally don't care about this limitation, but that might be a controversial issue that I'd rather leave out of the feeds as nodes discussion. I updated an existing issue accordingly: #15266: Replace aggregator category system with taxonomy

"Now we have the full node workflow, we should refresh on cron only the ones that are set to published" - I wanted to do this for FeedAPI but there was some serious and reasonable push back on this. I think published/unpublished shouldn't be coupled with refreshing/not refreshing on cron. #19646: Aggregator: Feed Suspend Option

Node revisions... good idea. Let me think about this. URL validation: separate issue.

These are the issues I'm seeing that need to be addressed with this patch:

* Respect node permissions
* Write update hooks for schema changes

#45 - using vid for tying aggregator to node table (enabling revisions on the aggregator table) is an excellent idea. I don't see any problems with it, we should do it.

For the taxonomy change - assuming it goes in in a followup patch, we could rip out the category updates introduced with this patch since HEAD to HEAD updates aren't supported (if that's feasible given the upgrade path).

we could rip out the category updates introduced with this patch since HEAD to HEAD updates aren't supported (if that's feasible given the upgrade path).

catch, not sure whether I understand your point 100%: I'd say we can always patch an update() function that updates from HEAD to HEAD. So I would leave any category changes in this patch (they are minor, mostly changing fid to nid) to keep aggregator fully functional after this patch. Later, if people agree on #15266: Replace aggregator category system with taxonomy we can patch core in a way so that we don't wind up with converting things in one update hook, then removing them in a subsequent update hook.

alex_b - yeah taht's what I meant. Thanks for re-opening #15266: Replace aggregator category system with taxonomy.

Here is a new version.
Revision support (for both aggregator_feed and aggregator_category_feed tables)
Test class for revision support
aggregator/sources/%node path respect node permissions.

I messed up the test module under /tests/ at the last patch.

The test in #53 failed because of aggregator-feed-info.tpl.php and changes to test module missing. This patch should pass.

Reviewing now.

Just went through the code and did a bunch of small fixes and added an update hook for upgrading the schema. This is looking very good. We need more reviews now to get this to RTBC.

Fixes:

• Respect versions when updating aggregator_feed after parsing.
• Show feed items on aggregator/sources/[nid] when node is unpublished but hide link back to node.
• Add update hook for converting DB schema.
• Clean up language in revisioning test.
• Filter URL on display in feed-info.tpl.php
• Remove aggregator_feed_load() - not used anymore.

I haven't followed this closely enough to know whether anything specific should be tested, but the patch works like a charm.
Creating a feed via node/add and using revisions shows no problems at all.

Just looking through the patch, some minor things, mostly related to DBTNG

+  if (@is_object($node->feed) && user_access('administer news feeds')) {

Can't you use isset() && is_object() instead ? @ is ugly and slow...

+function hook_node_delete_revision($node) {
+  db_delete('aggregator_feed')->condition('vid', $node->vid)->execute();
+  db_delete('aggregator_category_feed')->condition('vid', $node->vid)->execute();
+}

These should be converted to multi-line calls.

+  $nids = pager_query(db_rewrite_sql('SELECT n.nid FROM {node} n INNER JOIN {aggregator_feed} f ON f.vid = n.vid WHERE n.status = 1 ORDER BY n.sticky DESC, n.created DESC'), variable_get('default_nodes_main', 10))->fetchCol();

- That query needs to be converted to a dynamic db_select() query with addTag('node_access') and ->extend('PagerDefault'). See http://drupal.org/files/issues/blog_dbtng_conversion3.patch for an example.

#58 berdir: thanks for your review. This patch takes care of all of your points.

I also added creating a content type when updating from previous aggregator versions. I ran into a problem where field_attach_create_bundle() is not loaded when creating a content type in hook_update(). Apply #439236: field_attach_create_bundle() undefined when creating a content type on hook_update() for testing hook_update of this patch.

It works great, very nice work.

The only thing.. I don't really understand why it creates a content type when enabled and it doesn't define it with _node_info() like blog.module. Also there's this variable in install script seems not to be used: variable_set('aggregator_enabled_feed', TRUE);

About the upgrade script, what happens with existing feeds? I mean I see no nodes being created for existing old feeds. Btw, this whole script would be much easier if you rename old table, create new one and run some INSERT/SELECT, which btw would preserve old data just in case (existing content type, script doesn't complete, etc...)

I've tried the coder module on the code, it just throws these two:
- aggregator.install: severity: normal Line -1: @file block missing (Drupal Docs)
- aggregator.test: Line 567: String concatenation should be formatted with a space separating the operators (dot .) and the surrounding terms... $this->drupalPost('node/'. $feed->nid .'/revisions/'. $vids[0] .'/revert', array(), 'Revert');
(plus there's some bad indenting on aggregator.install line 342

#60: Thanks for review, Jose.

- Fixed code style issue in test. Left out @file block issue in .install file - not related to this patch.
- Rerolled to reflect renaming of drupal_execute() to drupal_form_submit()

Are content migrations required in Drupal major version updates? Eek, looking at other modules I'm afraid so :P

The variable from variable_set('aggregator_enabled_feed', TRUE) is used e. g. in aggregator_form_alter() with variable_get('aggregator_enabled_' . $form['type']['#value'], FALSE) - that's why you couldn't grep it...

The reasoning behind not providing a hard coded content type is that you may want to be able to remove the content type and replace it by another one (and it is perfectly possible to do so). The patch is not following the example of blog module, but rather node module or book module: even the book content type can be removed now.

Still requires #439236: field_attach_create_bundle() undefined when creating a content type on hook_update() to update successfully - am I doing anything wrong when creating the content type in aggregator_update_7001() ?

- needs content upgrade path
- catch situation when a content type 'feed' already exists

This needs to be rerolled. Here's the chunk that was rejected. I'm willing to reroll it but know how to do that exactly.

Looks like the part that wouldn't patch is just ripping out aggregator_form_feed(), along with its submit and validate functions, as well as one little node_load() line.

Is this right?

#64 akahn: you're missing aggregator-feed-info.tpl.php which is a new file. Check out tools like cvsdo to add new files to a repository that you don't have write permissions to.

Rerolled to reflect recent changes to aggregator. I'm going to work on the changes outlined in #60 and #61 now.

Changes:

- On installation/update make sure a feed content type is created by iterating until a type name is found that is not taken.
- Implemented hook_requirements() and warn user if there is no feed content type available.
- Implemented conversion for existing feeds to feed nodes. This is not tested.
- Changed back functionality of aggregator_save_feed() to include a call to aggregator_processor_save_feed_category(). This makes more sense as aggregator_save_feed() should be the single entry point for storing a $node->feed object. This also reflects closer the existing functionality of aggregator_save_feed().

Todo:

- Make feeds to feed nodes a batch operation. This conversion can take a long time.
- Test conversion.

Setting to needs review to get the test bot working on this. I'd also really love some more eyes on this, as this work is getting a lot closer to finalization now.

The update doesn't work: I get the following error message which disrupts the update process:

An error occurred.
Path: http://localhost/drupalupdatetest/update.php?id=2&op=do
Message: PDOException: INSERT INTO {node_type} (type, name, base, has_title, title_label, has_body, body_label, description, help, min_word_count, custom, modified, locked, orig_type) VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1, :db_insert_placeholder_2, :db_insert_placeholder_3, :db_insert_placeholder_4, :db_insert_placeholder_5, :db_insert_placeholder_6, :db_insert_placeholder_7, :db_insert_placeholder_8, :db_insert_placeholder_9, :db_insert_placeholder_10, :db_insert_placeholder_11, :db_insert_placeholder_12, :db_insert_placeholder_13) -
Array
(
[:db_insert_placeholder_0] => feed
[:db_insert_placeholder_1] => Feed
[:db_insert_placeholder_2] => node_content
[:db_insert_placeholder_3] => 1
[:db_insert_placeholder_4] => Title
[:db_insert_placeholder_5] => 1
[:db_insert_placeholder_6] => Body
[:db_insert_placeholder_7] => A feed can be used to download syndicated content like RSS or Atom feeds to your web site.
[:db_insert_placeholder_8] =>
[:db_insert_placeholder_9] => 0
[:db_insert_placeholder_10] => 1
[:db_insert_placeholder_11] => 1
[:db_insert_placeholder_12] => 0
[:db_insert_placeholder_13] => feed
)
SQLSTATE[42S22]: Column not found: 1054 Unknown column 'base' in 'field list' in node_type_save() (line 593 of /srv/www/htdocs/drupalupdatetest/modules/node/node.module).

The error page I am then led to says:

The update process was aborted prematurely while running update #7001 in aggregator.module. All errors have been logged. You may need to check the watchdog database table manually.

- Reflect recent changes to aggregator
- Fixed #67
- Tested conversion in aggregator_update_7001()

Apply [##439236] when testing update.

Outstanding:

- Conversion to nodes needs to be batched.

Resolve recent conflicts. Working on batching now.

- Update hook uses batch API now.

All issues brought up above are now addressed. Further reviews and feedback more than welcome.

Apply #439236: field_attach_create_bundle() undefined when creating a content type on hook_update() when testing update.

I was about to RTBC but + // $this->assertEqual(count(array_diff($parsed, $processed)), 0, t('Found stored results from processing stage.'));
this looks strange. Why is this commented out? Also, if you want to see something being 0 use assertFalse simply: $this->assertFalse(array_diff($parsed, $processed), t('Found stored results from processing stage.')) You can even try $this->assertTrue($parsed == $processed, t('Found stored results from processing stage.')) if that's appropriate.

#73: Thank you for review. This is fixed.

The assertion tests whether the processing stage yields the feed items that the test pseudo feed contains. It's using an assertEqual rather than an assertFalse because it more accurately reflects what we are looking for: a difference of 0 between the results of parsing and processing stage.

I think this is ready -- let's see what others say :)

Rerolled to reflect recent changes by #220233: Put node_get_types() out of its misery and #480102: Update aggregator module to use drupal_static().

Test bot did its work, setting back to RTBC now.

While I support feeds as nodes, this can only be an interim step. It is way too rough around the edges especially around usability and easy of use. We need to take another pass at this with end-user usability in mind. Needs quite a bit more work ...

1. For non-technical end-users, I think the node form is a regression for setting up feeds. A couple of problems:
   • The node form does too much when all you want to do is add a simple feed. The vertical tabs are completely unnecessary in this case (Mark and Leisa's design would solve this by moving it under 'Meta').
   • It made more sense to have the add feed form on admin/content/aggregator/add/feed than to have it under "create content". Sorry, but it is the truth.
2. The default node view for feed items is also a regression:
   • It shows the user that created the feed as the author.
   • Showing the date as we do now doesn't make sense either.
   • The node page is pretty empty therefore confusing. After creating a feed, people probably expect to see the feed's news items, not an empty node page. It would be better to show the feed items.
   • The "Update feed" and "Remove items" tab are odd. It might be better that we remove those and let people do these kind of operations on the admin overview page.
3. The feed page that shows the news items shows: " Feed: feed name" which is redundant. The feed name is already shown above as the title of the page.
4. $node->feed->nid, $feed_node->feed->url, $node->feed->title seems like inproper use of the name space. If feeds are nodes, we should use $feed->nid, $feed->title, $feed->url, etc. The current nesting is messy and not developer friendly.

Hm. Yes, the user interface changes do represent a net loss in simplicity...

Would an alternative to this approach be to make aggregator feeds first-class fieldable entities of their own right, so that you could attach things to them such as parsers and validators?

Just a question (without testing) - to address dries concerns, do the feeds themselves really need to be nodes? Why not use nodes for feed items?

Whats the status of this issue? I would be happy to reroll a patch for this if there is still a chance to have it go into D7?

#79: 293318-78_aggregator_feeds_as_nodes.patch queued for re-testing.

sorry i never meant to do that :(

Tagging for Drupal 8 multilingual initiative.

In Drupal 6, it was possible to set up feeds that saved into their own data tables using Feeds + Data modules. Additionally, since feeds were nodes, they could have taxonomy terms, etc. attached.

In Drupal 7, the Data module looks like it may disappear, and the core Aggregator module has been improved. The "categories" included in the core Aggregator module are useful, but I would love the ability to attach fields to Aggregator feeds.

Rather than making feeds nodes, wouldn't it be possible (via hook_entity_info(), etc.) to make Aggregator module feeds "fieldable"? Each feed category could be a bundle, and then users could attach their own fields (in my case, taxonomy_term_reference fields) to feeds, which would make Aggregator much more useful and flexible.

I'm still getting familiar with the Field Attach API - but from what I know now, this doesn't sound like it would be very hard to implement.

Yep, these should almost definitely be entities, re-titling.

Because of my needs (posted above), and since I assume no new features will make it into the core Aggregator module at least until Drupal 8.x, I decided to create a module (for Drupal 7) that expands the core Aggregator module in a number of ways to make it more scalable. It makes feeds fieldable, uses pagination on the admin/config/services/aggregator page, allows bulk deletion of feeds, provides a log of "bad" HTTP status codes and parse errors encountered (and a new parser & fetcher to help fill this table), a utility to check all current feed URLs on the site (via Batch API), and extra Views integration, along with a utility to let current users of the Feeds module migrate their data to use Aggregator instead.

I realize that not all these features are relevant to this thread, but, if nothing else, the module contains the code needed to make Aggregator feeds fieldable and could lighten the load of someone looking to patch the Aggregator module. I also think the other changes (mentioned above) are necessary to provide scalability and flexibility and should be considered as core features in future versions of Aggregator.

Also, in the above post, I'd suggested that categories could be bundles - but this doesn't make sense the way they're currently treated (a feed can belong to multiple categories, and categories can be changed via checkboxes on the feed edit page at any time). So for now, I exposed one bundle, just called "aggregator_feed," to which all Aggregator feeds belong. Perhaps in the future, categories should become fixed bundles and the type of information categories currently represent could be taken over by user-defined Fields.

This is my first contribution - so it's a sandbox project right now, but the URL is http://drupal.org/sandbox/nkschaefer/1216400 and my full project application is here: http://drupal.org/node/1216496.

Thanks - and I hope this helps get the ball rolling.

Tagging for the content handling leg of D8MI.

Ha, it helps if I do it.

Nevermind.

Ok, here's a start at this.

This converts aggregator feed *items* to EntityNG and uses a render controller. Haven't touched the feed/category objects yet as we weren't sure if they're supposed to be a content or a config entity. My suggestion would be to do each of those in a separate issue, as each requires quite a bit of changes to properly use all the different controllers that we now have.

Surely not finished yet, the view_mode (default and summary) and templates handling is a bit hacky and the view modes aren't yet defined but the tests seem to pass. Not sure what exactly how much they cover but I think quite a bit.

This is in fact quite a bit more code speaking in lines of codes, but it also gives us a ton, like all kinds of hooks that allow to customize aggregator items and we could also use a generic views/entity integration once that is implemented instead of aggregator specific views data/plugins.

Looks nice.

+++ b/core/modules/aggregator/aggregator.pages.inc
@@ -172,8 +163,9 @@ function _aggregator_page_list($items, $op, $feed_source = '') {
+    if ($items) {
+      $rendered_items = entity_view_multiple($items, 'default');
+      $output .= drupal_render($rendered_items);

Calling drupal_render() is a code smell. In this case, we ought to rework this so that the page callback returns a render array. We might want to make use of #theme_wrappers if we are not prepending and appending stuff. I have not looked into this in detail.

+++ b/core/modules/aggregator/aggregator.pages.inc
@@ -527,7 +517,11 @@ function theme_aggregator_page_opml($variables) {
-  $variables['summary_list'] = theme('item_list', array('items' => $variables['summary_items']));
+  $rendered_items = array();
+  foreach (element_children($variables['summary_items']) as $key) {
+    $rendered_items[] = drupal_render($variables['summary_items'][$key]);
+  }

ewww. I haven't looked into this, but we generally try to call render($foo) from the template, and not render stuff in preprocess.

To the second point, the problem is that theme_item_list() doesn't allow render arrays, see #891112: Replace theme_item_list()'s 'data' items with render elements

Can't reproduce that test failure locally.

Converted those strings to render arrays* and removed the aggregator_wrapper template. Replicated the existing div structure, I'm not sure if we really need that, I'd argue that we could get rid of that additional div but maybe some frontent people can give advice on that?

With the exception of the theme_item_list case, that's blocked on #891112: Replace theme_item_list()'s 'data' items with render elements but could be nicely improved after that goes in.

Let's try again, testbot.

Ah, figured it out. The assertion only checked the http response code and while I did have a fatal error locally as well, PHP for some reason did not return a 500 error code, as it does on the testbot.

This should pass. Added some additional assertions to make this less dependendant on the environment.

+++ b/core/modules/aggregator/aggregator.module
@@ -94,6 +89,25 @@ function aggregator_theme() {
+      'id' => 'iid',
+      'label' => 'title',

We want to get away from "iid" id properties. This can be a follow up though.

+++ b/core/modules/aggregator/lib/Drupal/aggregator/ItemStorageController.php
@@ -0,0 +1,118 @@
+      'label' => t('Link'),
+      'description' => t('The link of the feed item.'),
+      'type' => 'string_field',

Is the link a proper URI? In that case I think we should add the 'uri_field' which has an URI as primitive value.

+++ b/core/modules/aggregator/lib/Drupal/aggregator/ItemStorageController.php
@@ -0,0 +1,118 @@
+    $fields['timestamp'] = array(
+      'label' => t('Posted timestamp'),
+      'description' => t('Posted date of the feed item, as a Unix timestamp.'),
+      'type' => 'integer_field',

This should be a date_field.

Haven't touched the feed/category objects yet as we weren't sure if they're supposed to be a content or a config entity.

I think this is one of the situations where config vs content depends on the concrete use-case. Thinking of a aggregator feed, I might to want to:
* configure my own fields on it
* use regular content-entity tools, e.g. workflows for adding or editing feeds
* configure a feed and deploy it via CMI

Given that I think the situation is very similar to taxonomy terms. So to enable all use-cases and for consistency I'd put feeds into the content bin. Export to files/CMI for deployment for content is something I expect to be added by contribs anyway, ala uuid module.

in aggregator_page_sources(), the variable that gets built up as a render array is called $output. We standardized on $build for this. I think this happens elsewhere as well. Other than this minor note, and fago's feedback, this looks ready.

- Renamed $output to $build
- Added UriItem/uri_item and use that for link.
- Added class properties.

Not using date_field yet, as it requires changes to the database storage controller (see #1778178: Convert comments to the new Entity Field API) and I'm not sure yet about how that exactly should work. So maybe change that in a follow-up?

Untested patch, will probably break completely.

Forgot to finish the __construct() unset() stuff. Ugly, but currently necessary and it's what EntityTest and Comment in the comments issue are doing too.

Also lost the latest changes. Let's see if this passes again.

We'll also need to move the unset() fixed to init()/__wakeup() as well to make it work with serialize(). As the fix for EntityNG is in the comment patch, we'd ideally build upon this one. Comment should be ready to go anway.

@date:
Once EntityNG does the storage, setting by timestamp will work out of the box. When reading you'll get a DateTime object, so you have to call ->getTimestamp() on it to get a timestamp.

#107: aggregator-item-entity-293318-107.patch queued for re-testing.

Re-roll and also updated the entity definition and some coding style stuff.

Given that the comment issue is not using date fields anymore either for performance reasons (they will be added later), we don't really need support for serialize/unseralize here and the comment issue still isn't in, I'd suggest to get this in asap and create a follow-up issue to adapt those two things later.

Thoughts?

And with the patch too.

#112: aggregator-item-entity-293318-111.patch queued for re-testing.

Failed because of an unrelated deadlock.

Rerolled patch..tested it a bit:
Visit aggregator/categories and reload a couple of times..(with some test categories and feeds):

User error: "0" is an invalid render array key in element_children() (line 6009 of core/includes/common.inc).
User error: "1" is an invalid render array key in element_children() (line 6009 of core/includes/common.inc).
User error: "datetime" is an invalid render array key in element_children() (line 6009 of core/includes/common.inc).

Dont have time to check this now, will do later if someone does not beat me to it

User error: "0" is an invalid render array key in element_children() (line 6009 of core/includes/common.inc).
User error: "1" is an invalid render array key in element_children() (line 6009 of core/includes/common.inc).
User error: "datetime" is an invalid render array key in element_children() (line 6009 of core/includes/common.inc).

Seems to be a twig bug..no errors when i use theme_datetime directly

Also tests pass locally

#115: aggregator-item-entity-293318-115.patch queued for re-testing.

Re-roll after the blocks issue went in. Haven't seen the render keys error here so far...

Can you provide more detailed steps on how to reproduce it?

hmm, sorry went to retry the render problem and when i added a new feed and tried to update items
Fatal error: Call to a member function getStatusCode() on a non-object in /var/www/d8/core/modules/aggregator/lib/Drupal/aggregator/Plugin/aggregator/fetcher/DefaultFetcher.php on line 54
something is wrong with my guzzle?

Edit
Also when i visit configure page of a feed (aggregator/categories/1/configure) i get
Fatal error: Cannot use object of type stdClass as array in /var/www/d8/core/modules/aggregator/aggregator.admin.inc on line 606

Found more

for the getStatusCode(), I think that's what I described in #1875792-4: Standardize Guzzle exception handling. It's a problem and needs to be fixed, but it is not introduced by this issue.

The category one does sound like a bug, though.

Seems aggregator needs some more love..why there are two places to edit a category or a feed?
eg
admin/config/services/aggregator/edit/category/1
and
aggregator/categories/1/configure
I cant see the point, but anyway thats a different issue..
I fixed aggregator/categories/*/configure and aggregator/categories/*/categorize pages..also fixed some indentation issues

#Guzzle: Thanks Berdir for the link, seems that i only get the fatal error when i try to manually update a feed..not via cron...anyway..seperate issue
#render errors..they are still there...though i cant figure this out, i have no clue what happens in Twig layer..anyway..to reproduce:
-Add a category
-Add a feed that belongs to previously created category( what i used as url is a random fb feed eg http://www.facebook.com/feeds/page.php?id=116391465049133&format=rss20
-Update feed to receive the feed items from net
-Visit aggregator/categories
-Reload aggregator/categories so that error messages appear

Haven't touched the feed/category objects yet as we weren't sure if they're supposed to be a content or a config entity.

I think aggregator categories should be taxonomy..see also #15266: Replace aggregator category system with taxonomy

Using taxonomy as categories makes sense, but it requires that feeds are first converted to a content entity as well. And we will run into the usual problems with taxonomy term references and code listings (We can't provide /aggregator/categories/1 etc. listings in core if the taxonomy term field is not hardcoded and required but do we really want to make it required?) So we would have to replace it with views anyway, which probably makes sense, but there's the deadline problem ;)

working on feeds then.converting em to fieldable content entities with no bundle types, kinda like user module..

my work so far..most probably will fail hard (or even not apply at all, just noticed, catch did quite a few commits)
but i will continue later, if anyone wants to review it, feel free

+++ b/core/lib/Drupal/Core/Entity/Field/Type/UriItem.phpundefined
@@ -0,0 +1,39 @@
+ * @file
+ * Definition of Drupal\Core\Entity\Field\Type\UriItem.

Should be Contains now.

+++ b/core/modules/aggregator/aggregator.moduleundefined
@@ -603,19 +515,17 @@ function aggregator_refresh($feed) {
- * @param $fid
+ * @param int $fid
  *   The feed id.
+ * @param bool $reset
+ *  TRUE to reset the internal cache and load from the database;
+ *  FALSE (default) to load from the internal cache, if set.

I would suggest to not add the reset argument, I want to get rid of that from load functions anyway...

+++ b/core/modules/aggregator/lib/Drupal/aggregator/FeedStorageController.phpundefined
@@ -0,0 +1,183 @@
+   * Overrides Drupal\Core\Entity\DataBaseStorageController::__construct().

Should be \Drupal\Core\...

+++ b/core/modules/aggregator/lib/Drupal/aggregator/ItemRenderController.phpundefined
@@ -0,0 +1,31 @@
+ * Cotains Drupal\aggregator\ItemRenderController.

Typo.

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Plugin/Core/Entity/Feed.phpundefined
@@ -0,0 +1,182 @@
+  public function __construct(array $values, $entity_type) {
+    parent::__construct($values, $entity_type);
+
+    // We unset all defined properties, so magic getters apply.
+    unset($this->fid);
+    unset($this->title);
+    unset($this->url);
+    unset($this->refresh);
+    unset($this->checked);
+    unset($this->queued);
+    unset($this->link);
+    unset($this->description);
+    unset($this->image);
+    unset($this->hash);
+    unset($this->etag);
+    unset($this->modified);

This should move to the init function now.

this got big pretty fast:/
but i think its ready now..tests should pass

done for today

Looks great!

Some comments below

+++ b/core/lib/Drupal/Core/Entity/DatabaseStorageControllerNG.phpundefined
@@ -290,7 +290,9 @@ protected function invokeHook($hook, EntityInterface $entity) {
     foreach ($this->entityInfo['schema_fields_sql']['base_table'] as $name) {
-      $record->$name = $entity->$name->value;
+      if (isset($entity->$name->value)) {
+        $record->$name = $entity->$name->value;
+      }

Might be worth checking if this is still necessary. Should not actually be will probably conflict with #1833334: EntityNG: integrate a dynamic property data table handling

+++ b/core/lib/Drupal/Core/Entity/Field/Type/UriItem.phpundefined
@@ -0,0 +1,39 @@
+ * Contains Drupal\Core\Entity\Field\Type\UriItem.

When a namespace is used then it should start with a \

+++ b/core/modules/aggregator/aggregator.moduleundefined
@@ -399,100 +395,18 @@ function aggregator_save_category($edit) {
- * @param $feed
+ * @param Drupal\aggregator\Plugin\Core\Entity\Feed $feed

Same here, @param \Drupal...

+++ b/core/modules/aggregator/lib/Drupal/aggregator/FeedFormController.phpundefined
@@ -0,0 +1,145 @@
+ * @file
+ * Definition of Drupal\aggregator\FeedFormController.

Another one that should be Contains and have a \ :)

Might be able to search from them with " Drupal\" in the new files....

+++ b/core/modules/aggregator/lib/Drupal/aggregator/FeedStorageController.phpundefined
@@ -0,0 +1,193 @@
+    $fields['block'] = array(
+      'label' => t('Block'),
+      'description' => t('Number of items to display in the feed’s block.'),
+      'type' => 'integer_field',

Wondered for a moment if this was re-introduced here due to a merge conflict after the blocks as plugins issue went in, but it seems to be duplicated in HEAD as well (the block instance has the same setting).

I have no clue which one is actually used but this should probably be removed here (in another issue, this one is big enough!)

Thanks for the review Berdir

I have no clue which one is actually used but this should probably be removed here (in another issue, this one is big enough!)

Agreed:)

I think i fixed all namespaces

Ok, passed without that check and looks ready to me. Can't really RTBC it as I've written half of it but should get in ASAP I think so that we can continue improve this to actually have a useful aggregator system in core that contrib can build upon instead of replacing it completely :)

Otherwise we could probably just as well remove it.

Maybe @moshe can RTBC it? :)

Just minor stuff I noticed. Only the first one is a prerequisite for RTBC, IMO.

+++ b/core/modules/aggregator/aggregator.pages.inc
@@ -556,9 +564,13 @@ function theme_aggregator_page_opml($variables) {
 function template_preprocess_aggregator_summary_items(&$variables) {
-  $variables['title'] = check_plain($variables['source']->title);
-  $variables['summary_list'] = theme('item_list', array('items' => $variables['summary_items']));
-  $variables['source_url'] = $variables['source']->url;
+  $variables['title'] = check_plain($variables['source']->label());
+  $rendered_items = array();
+  foreach (element_children($variables['summary_items']) as $key) {
+    $rendered_items[] = drupal_render($variables['summary_items'][$key]);
+  }
+  $variables['summary_list'] = theme('item_list', array('items' => $rendered_items));

theme_item_list() now take render arrays as items so we don't need to call drupal_render() here.

+++ b/core/modules/aggregator/aggregator.admin.inc
@@ -92,172 +93,47 @@ function aggregator_view() {
+function aggregator_feed_edit(Feed $feed) {
+  drupal_set_title(t('Edit %label feed', array('%label' => $feed->label())), PASS_THROUGH);
+  return entity_get_form($feed);
 }

Thats pretty old school. Can we not use a title_callback in aggregator_menu()?

+++ b/core/modules/aggregator/aggregator.pages.inc
@@ -32,9 +34,9 @@ function aggregator_page_last() {
+function aggregator_page_source(Feed $feed) {
+  drupal_set_title($feed->label());
+  $feed_source = entity_view($feed, 'default');

Again, can title be set in a title_callback in hook_menu.

+++ b/core/modules/aggregator/aggregator.admin.inc
@@ -92,172 +93,47 @@ function aggregator_view() {
+function aggregator_feed_edit(Feed $feed) {
+  drupal_set_title(t('Edit %label feed', array('%label' => $feed->label())), PASS_THROUGH);
+  return entity_get_form($feed);
 }

Again, can title be set in a title_callback in hook_menu?

Thanks for the review:)
About title_callback, i am not sure, lets keep it consistent for now. rest of core does the same, i couldnt find title_callback being used anywhere like that..
fixed the render array thingie though

Edit Nevermind my grep was wrong..patch coming

+++ b/core/modules/aggregator/aggregator.pages.incundefined
@@ -565,11 +565,14 @@ function theme_aggregator_page_opml($variables) {
-  $rendered_items = array();
+  $summary_items = array();
   foreach (element_children($variables['summary_items']) as $key) {
-    $rendered_items[] = drupal_render($variables['summary_items'][$key]);
+    $summary_items[] = $variables['summary_items'][$key];
   }
-  $variables['summary_list'] = theme('item_list', array('items' => $rendered_items));
+  $variables['summary_list'] = array(
+    '#theme' => 'item_list',
+    '#items' => $summary_items,

I think it should be possible to drop the foreach here completely, I just addded that because I had to use drupal_render().

So just #items => $variables['summary_items'].

I have to use element_children() cause $variables['summary_items'] has $variables['summary_items']['#sorted'] => TRUE which on render prints an <li>1</li>

patch coming with a title callback

Well, i removed aggregator_feed_edit and used directly entity_get_form in hook_menu..Also added a title callback for view page.not sure if its better now, but i guess #135 could be commited instead:)

That title_callback is perfect. If you don't mind, there were a couple other instances that need it. See #134.

Yeah, i tried to use callbacks there as well, but then tabs where messed up in aggregator feed view page..the tab instead of displaying "Configure" had the full title from the title callback, eg Edit feed Test

Yes, that is exactly the difference between menu link title and page title, which is not always the same.

#138: aggregator-entity-293318-138.patch queued for re-testing.

+++ b/core/modules/aggregator/aggregator.moduleundefined
@@ -275,6 +277,19 @@ function _aggregator_category_title($category) {
 /**
+ * Title callback for aggregator feeds.
+ *
+ * @param Drupal\aggregator\Plugin\Core\Entity\Feed $feed
+ *   An aggregator feed entity.
+ *
+ * @return string
+ *   The feed label to be used as the page title.
+ */
+function aggregator_feed_title(Feed $feed) {
+  return $feed->label();

You can use entity_page_label() for this, instead of having to add a separate helper function.

patch no longer applied, here is a rerolled one that incorporates #143 as well
entity_page_label() rocks, thanks Berdir:)

had to add a langcode field for tests to pass...no idea what happens there tbh, but now tests pass

+++ b/core/modules/aggregator/lib/Drupal/aggregator/ItemStorageController.phpundefined
@@ -77,6 +77,11 @@ public function baseFieldDefinitions() {
+      'description' => t('The language code of the aggregator feed.'),

yes..should be aggregator feed item..fixed in the following

Ok, I think the langcode should not be be a required property.

The problem is that EntityNG::language() unconditionally access the language property but that throws an exception if it's not defined. This also triggerd #1894436: EntityNG doesn't declare Language and therefore fixes and adds test coverage for that. Yay! :)

Ouch. langcode, not language...

Also removed the langcode field from feeds.

This doesn't introduce any new features, it just converts feeds and feed items which were using custom storage functions to entities. And cleans up rendering/theming a bit. Changing from a feature to a task.

+++ b/core/lib/Drupal/Core/Entity/EntityNG.php
@@ -263,8 +264,11 @@ public function isEmpty() {
   public function language() {
-    $language = $this->get('langcode')->language;
-    if (!$language) {
+    // Get the language code if the property exists.
+    if ($this->getPropertyDefinition('langcode')) {
+      $language = $this->get('langcode')->language;
+    }
+    if (!isset($language)) {
       // Make sure we return a proper language object.
       $language = new Language(array('langcode' => LANGUAGE_NOT_SPECIFIED));

@Berdir asked me to have a look to this hunk: the change looks totally sensible to me.

No longer applies, one more reroll

Awesome work. All recent objections are addressed, AFAICT.

#154: aggregator-entity-293318-154.patch queued for re-testing.

Trying to avoid reroll hell for a key Entity adoption patch.

today is my birthday..i would love this as present :P
also opened #1905470: Convert Aggregator categories to config entities

This looks good to me, however I'd like to understand all of the follow-up patches (e.g. moving the categories to taxonomy terms, changing the aggregator pages to views, etc). Please add this in the comments here, as well as in the issue summary of this issue or in that of an aggregator meta issue.

@Dries if converting aggregator categories to taxonomy fields would fall under tasks and not features, i would be more than happy to close #1905470: Convert Aggregator categories to config entities and go on with #15266: Replace aggregator category system with taxonomy..else i just see #1905470: Convert Aggregator categories to config entities as actionable right now.
I am not sure about views conversions

IMO core should use taxonomy terms for categories. Handily, entity reference Field was just committed. It starts getting used at #1847596: Remove Taxonomy term reference field in favor of Entity reference. Thats the model we would use here (with a different upgrade path of course).

Sites with a massive number of feed items (and categories) might want to use a custom reference field if taxonomy doesn't scale to their needs.

One immediate follow-up would be at #1821844: Aggregator views integration

I agree with Moshe in #161. Let's create an overview of all the follow-up issues in the issue summary, and than we can commit this.

A proper issue summary

Updated issue summary with issues above

remove paste failure

Alright! Committed to 8.x. Now let's work on the follow-up issues.

Updated issue summary.

I am gonna write a change notice for it later. also added multilingual feeds/feed items to follow up issues

Add multilingual

Change notification here http://drupal.org/node/1905910

Made some small adjustments, looks good otherwise.

Can someone familiar with this arch change follow up on #1830068: Change teaser_length to default_summary_length in aggregator? to clarify what the plan is?

add multilingual issue link

Would a back port to 7.x be feasible?

Sorry to post to a closed issue but it relates directly to this.
Couldn't think of a better place.