Make Aggregator feeds into entities

I have updated the patch.

Most of the tests are passing now.
OPML import is working.

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.

Rerolled after the DB:TNG and the pluggable patches.

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 :)

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.

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.

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?

Working on #15 now

* 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

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.

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.

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.

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.

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).

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.

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.

Added following code to aggregator_form_opml_submit:

Page "aggregator/sources" was showing two pagers. Got rid of one of them by deleting following from aggregator_page_sources:

Run coder over aggregator.

Imported near 1000 real feeds and run cron. No aggregator related errors happened.

#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...

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.

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?

#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.

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.

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.

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.

#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.

#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() ?

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.

#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.

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.

The last submitted patch failed testing.

- 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.

#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.

The last submitted patch failed testing.

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.

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.

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

Tagging for the content handling leg of D8MI.