Problem/Motivation

Aggregator system lack the flexibility of other systems in drupal core.

Proposed resolution

- Convert Aggregator feeds and feed items to Content Entities leveraging EntityNG

Remaining tasks

- Commit rtbc'ed patch

Follow-up issues

- #1821844: Aggregator views integration
- #1905870: Make feeds/feed items multilingual
- #1847596: Remove Taxonomy term reference field in favor of Entity reference
- #15266: Replace aggregator category system with taxonomy
- Once we figure out #1806334: Replace the node listing at /node with a view, change feed and item listings to Views

Original report by mustafau

I have tried my best to keep this patch as simple as possible. There is no functionality change. Only difference is that feeds became nodes.

Files: 
CommentFileSizeAuthor
#154 aggregator-entity-293318-154.patch106.35 KBParisLiakos
PASSED: [[SimpleTest]]: [MySQL] 48,746 pass(es). View
#151 aggregator-entity-293318-151.patch106.33 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 50,817 pass(es). View
#151 aggregator-entity-293318-151-interdiff.txt778 bytesBerdir
#150 aggregator-entity-293318-150.patch106.51 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 50,694 pass(es). View
#150 aggregator-entity-293318-150-interdiff.txt558 bytesBerdir
#148 aggregator-entity-293318-148.patch106.51 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 50,673 pass(es), 119 fail(s), and 32 exception(s). View
#148 aggregator-entity-293318-148-interdiff.txt990 bytesBerdir
#147 aggregator-entity-293318-147.patch105.73 KBParisLiakos
PASSED: [[SimpleTest]]: [MySQL] 50,774 pass(es). View
#146 aggregator-entity-293318-146.patch105.73 KBParisLiakos
PASSED: [[SimpleTest]]: [MySQL] 50,695 pass(es). View
#146 interdiff.txt795 bytesParisLiakos
#144 aggregator-entity-293318-144.patch105.55 KBParisLiakos
FAILED: [[SimpleTest]]: [MySQL] 50,626 pass(es), 34 fail(s), and 4 exception(s). View
#138 aggregator-entity-293318-138.patch105.91 KBParisLiakos
PASSED: [[SimpleTest]]: [MySQL] 50,804 pass(es). View
#138 interdiff.txt3.14 KBParisLiakos
#135 aggregator-entity-293318-135.patch105.71 KBParisLiakos
PASSED: [[SimpleTest]]: [MySQL] 50,791 pass(es). View
#135 interdiff.txt1.84 KBParisLiakos
#132 aggregator-entity-293318-132.patch104.83 KBParisLiakos
PASSED: [[SimpleTest]]: [MySQL] 49,602 pass(es). View
#132 interdiff.txt12.3 KBParisLiakos
#129 aggregator-entity-293318-130.patch106.24 KBParisLiakos
PASSED: [[SimpleTest]]: [MySQL] 50,680 pass(es). View
#126 aggregator-item-entity-293318-126.patch62.55 KBParisLiakos
FAILED: [[SimpleTest]]: [MySQL] 50,293 pass(es), 93 fail(s), and 223 exception(s). View
#122 aggregator-item-entity-293318-122.patch34.94 KBParisLiakos
PASSED: [[SimpleTest]]: [MySQL] 50,485 pass(es). View
#122 interdiff.txt3.7 KBParisLiakos
#118 aggregator-item-entity-293318-118.patch33.2 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 50,576 pass(es). View
#115 aggregator-item-entity-293318-115.patch33.12 KBParisLiakos
PASSED: [[SimpleTest]]: [MySQL] 49,995 pass(es). View
#112 aggregator-item-entity-293318-111.patch32.85 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 49,246 pass(es). View
#111 aggregator-item-entity-293318-111-interdiff.txt7.45 KBBerdir
#107 aggregator-item-entity-293318-107.patch33.03 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch aggregator-item-entity-293318-107.patch. Unable to apply patch. See the log in the details link for more information. View
#107 aggregator-item-entity-293318-107-interdiff.txt2.63 KBBerdir
#105 aggregator-item-entity-293318-105.patch31.67 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 46,093 pass(es), 62 fail(s), and 584 exception(s). View
#105 aggregator-item-entity-293318-105-interdiff.txt7 KBBerdir
#102 aggregator-item-entity-293318-102.patch29.72 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 42,806 pass(es). View
#102 aggregator-item-entity-293318-102-interdiff.txt1.78 KBBerdir
#100 aggregator-item-entity-293318-100.patch28.71 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 42,796 pass(es), 2 fail(s), and 0 exception(s). View
#100 aggregator-item-entity-293318-100-interdiff.txt5.62 KBBerdir
#96 aggregator-item-entity-293318-96.patch24.95 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 42,805 pass(es), 2 fail(s), and 0 exception(s). View
#79 293318-78_aggregator_feeds_as_nodes.patch105.6 KBmustafau
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 293318-78_aggregator_feeds_as_nodes.patch. View
#76 293318-76_aggregator_feeds_as_nodes.patch108.84 KBalex_b
Passed: 11640 passes, 0 fails, 0 exceptions View
#73 293318-73_aggregator_feeds_as_nodes.patch108.69 KBalex_b
Failed: 11318 passes, 3 fails, 0 exceptions View
#71 293318-71_aggregator_feeds_as_nodes.patch108.78 KBalex_b
Failed: 11316 passes, 3 fails, 0 exceptions View
#70 293318-70_aggregator_feeds_as_nodes.patch107.81 KBalex_b
Failed: Failed to apply patch. View
#68 293318-68_aggregator_feeds_as_nodes.patch108.37 KBalex_b
Failed: 11082 passes, 377 fails, 184 exceptions View
#66 293318-66_aggregator_feeds_as_nodes.patch108.19 KBalex_b
Failed: Failed to apply patch. View
#65 293318-65_aggregator_feeds_as_nodes.patch106.85 KBalex_b
Failed: Failed to apply patch. View
#64 293316-64-aggregator-feeds-as-nodes.patch105.59 KBakahn
Failed: Failed to apply patch. View
#63 aggregator.admin_.inc_.rej_.txt6.77 KBakahn
#61 293318-61_aggregator_feeds_as_nodes.patch107 KBalex_b
Failed: Failed to apply patch. View
#59 293318_aggregator_feed_node_with_revisions.patch106.98 KBalex_b
Failed: Failed to apply patch. View
#56 293318_aggregator_feed_node_with_revisions.patch106.45 KBalex_b
Failed: Failed to apply patch. View
#55 293318_aggregator_feed_node_with_revisions.patch104.21 KBalex_b
Failed: Failed to apply patch. View
#53 293318_aggregator_feed_node_with_revisions.patch100.31 KBAron Novak
Failed: 10744 passes, 0 fails, 92 exceptions View
#51 293318_aggregator_feed_node_with_revisions.patch103.43 KBAron Novak
Failed: 10756 passes, 24 fails, 146 exceptions View
#44 293318_aggregator_feed_node.patch99.01 KBalex_b
Failed: Failed to apply patch. View
#43 293318_aggregator_feed_node.patch99.01 KBalex_b
Failed: Failed to apply patch. View
#39 293318_aggregator_feed_node_9.patch96.08 KBmustafau
Failed: Failed to apply patch. View
#38 293318_aggregator_feed_node_8.patch98.51 KBAron Novak
Failed: Failed to apply patch. View
#36 293318_aggregator_feed_node.patch92.76 KBalex_b
Failed: Failed to apply patch. View
#35 293318_aggregator_feed_node.patch80.89 KBalex_b
Failed: Failed to apply patch. View
#33 293318_aggregator_feed_node.patch80.27 KBalex_b
Failed: Failed to apply patch. View
#32 293318_aggregator_feed_node.patch80.06 KBalex_b
Failed: Failed to install HEAD. View
#31 293318_aggregator_feed_node.patch77.69 KBalex_b
Failed: 9669 passes, 7 fails, 0 exceptions View
#29 293318_aggregator_feed_node.patch76.3 KBalex_b
Failed: 9256 passes, 0 fails, 38 exceptions View
#20 293318_aggregator_feed_node.patch68.27 KBalex_b
Failed: Failed to apply patch. View
#19 293318_aggregator_feed_node.patch67.43 KBalex_b
Failed: Failed to apply patch. View
#18 293318_aggregator_feed_node.patch53.2 KBalex_b
Failed: Failed to apply patch. View
#7 aggregator-feed-node-2.patch59.34 KBAron Novak
Failed: Failed to apply patch. View
#1 aggregator-feed-node-1.patch51.89 KBmustafau
Failed: Failed to apply patch. View
aggregator-feed-node.patch39.43 KBmustafau
Failed: Failed to apply patch. View

Comments

mustafau’s picture

Title: Make Aggregator feeds into first class Drupal nodes » Make Aggregator feeds into 1st class Drupal nodes
FileSize
51.89 KB
Failed: Failed to apply patch. View

I have updated the patch.

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

Aron Novak’s picture

Clean and nice! I'll test it soon.

Aron Novak’s picture

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

mustafau’s picture

Status: Needs review » Needs work

This changes every DB query in the Aggregator module. I think we should wait until DB:TNG patch gets committed.

alex_b’s picture

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

alex_b’s picture

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.

Aron Novak’s picture

Status: Needs work » Needs review
FileSize
59.34 KB
Failed: Failed to apply patch. View

Rerolled after the DB:TNG and the pluggable patches.

catch’s picture

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.

Aron Novak’s picture

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

catch’s picture

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.

moshe weitzman’s picture

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.

SeanBannister’s picture

sub

Dries’s picture

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.

catch’s picture

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.

alex_b’s picture

Status: Needs review » Needs 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?

chx’s picture

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.

alex_b’s picture

Assigned: Unassigned » alex_b

Working on #15 now

alex_b’s picture

FileSize
53.2 KB
Failed: Failed to apply patch. View

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.

alex_b’s picture

FileSize
67.43 KB
Failed: Failed to apply patch. View

* 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

alex_b’s picture

FileSize
68.27 KB
Failed: Failed to apply patch. View

Rerolled patch, updated to latest changes.

alex_b’s picture

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.

bsherwood’s picture

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

SeanBannister’s picture

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.

catch’s picture

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

bsherwood’s picture

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.

catch’s picture

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.

bsherwood’s picture

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.

SeanBannister’s picture

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

alex_b’s picture

Status: Needs work » Needs review
FileSize
76.3 KB
Failed: 9256 passes, 0 fails, 38 exceptions View

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.

Status: Needs review » Needs work

The last submitted patch failed testing.

alex_b’s picture

Status: Needs work » Needs review
FileSize
77.69 KB
Failed: 9669 passes, 7 fails, 0 exceptions View

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

alex_b’s picture

FileSize
80.06 KB
Failed: Failed to install HEAD. View

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.

alex_b’s picture

FileSize
80.27 KB
Failed: Failed to apply patch. View

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

Status: Needs review » Needs work

The last submitted patch failed testing.

alex_b’s picture

FileSize
80.89 KB
Failed: Failed to apply patch. View

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

alex_b’s picture

Assigned: alex_b » Unassigned
Status: Needs work » Needs review
FileSize
92.76 KB
Failed: Failed to apply patch. View

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

Aron Novak’s picture

Status: Needs review » Needs work

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.
Aron Novak’s picture

Status: Needs work » Needs review
FileSize
98.51 KB
Failed: Failed to apply patch. View

Here is the updated patch.

mustafau’s picture

FileSize
96.08 KB
Failed: Failed to apply patch. View

Added following code to aggregator_form_opml_submit:

if (isset($form_state['values']['category'])) {
  $edit['values']['feed']['category'] = $form_state['values']['category'];
}
$edit['values']['promote'] = '0';

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

$build['pager'] = array(
  '#markup' => theme('pager', NULL, variable_get('default_nodes_main', 10)),
  '#weight' => 5,
);

Run coder over aggregator.

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

mustafau’s picture

What will happen to feed nodes when uninstalling Aggregator?

alex_b’s picture

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

catch’s picture

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.

alex_b’s picture

FileSize
99.01 KB
Failed: Failed to apply patch. View

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.

alex_b’s picture

FileSize
99.01 KB
Failed: Failed to apply patch. View

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

Jose Reyero’s picture

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?

alex_b’s picture

Status: Needs review » Needs work

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

alex_b’s picture

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

catch’s picture

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

alex_b’s picture

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.

catch’s picture

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

Aron Novak’s picture

Status: Needs work » Needs review
FileSize
103.43 KB
Failed: 10756 passes, 24 fails, 146 exceptions View

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.

Status: Needs review » Needs work

The last submitted patch failed testing.

Aron Novak’s picture

Status: Needs work » Needs review
FileSize
100.31 KB
Failed: 10744 passes, 0 fails, 92 exceptions View

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

Status: Needs review » Needs work

The last submitted patch failed testing.

alex_b’s picture

Status: Needs work » Needs review
FileSize
104.21 KB
Failed: Failed to apply patch. View

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

Reviewing now.

alex_b’s picture

FileSize
106.45 KB
Failed: Failed to apply patch. View

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.
tstoeckler’s picture

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.

Berdir’s picture

Status: Needs review » Needs work

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.

alex_b’s picture

Status: Needs work » Needs review
FileSize
106.98 KB
Failed: Failed to apply patch. View

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

Jose Reyero’s picture

Status: Needs review » Needs work

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

alex_b’s picture

Status: Needs work » Needs review
FileSize
107 KB
Failed: Failed to apply patch. View

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

alex_b’s picture

Status: Needs review » Needs work

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

akahn’s picture

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.

akahn’s picture

FileSize
105.59 KB
Failed: Failed to apply patch. View

Is this right?

alex_b’s picture

Assigned: Unassigned » alex_b
FileSize
106.85 KB
Failed: Failed to apply patch. View

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

alex_b’s picture

Assigned: alex_b » Unassigned
Status: Needs work » Needs review
FileSize
108.19 KB
Failed: Failed to apply patch. View

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.

tstoeckler’s picture

Status: Needs review » Needs work

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.

alex_b’s picture

Status: Needs work » Needs review
FileSize
108.37 KB
Failed: 11082 passes, 377 fails, 184 exceptions View

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

Status: Needs review » Needs work

The last submitted patch failed testing.

alex_b’s picture

Assigned: Unassigned » alex_b
Status: Needs work » Needs review
FileSize
107.81 KB
Failed: Failed to apply patch. View

Resolve recent conflicts. Working on batching now.

alex_b’s picture

Assigned: alex_b » Unassigned
FileSize
108.78 KB
Failed: 11316 passes, 3 fails, 0 exceptions View

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

chx’s picture

Status: Needs review » Needs work

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.

alex_b’s picture

Status: Needs work » Needs review
FileSize
108.69 KB
Failed: 11318 passes, 3 fails, 0 exceptions View

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

chx’s picture

Status: Needs review » Reviewed & tested by the community

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

Status: Reviewed & tested by the community » Needs work

The last submitted patch failed testing.

alex_b’s picture

Status: Needs work » Needs review
FileSize
108.84 KB
Passed: 11640 passes, 0 fails, 0 exceptions View
alex_b’s picture

Status: Needs review » Reviewed & tested by the community

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

mustafau’s picture

FileSize
105.6 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 293318-78_aggregator_feeds_as_nodes.patch. View

Coder generated this warning:

aggregator.module
  * Line 270: Use ANSI standard <> instead of !=
Dries’s picture

Status: Reviewed & tested by the community » Needs work

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.
webchick’s picture

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?

nbz’s picture

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?

mustafau’s picture

Version: 7.x-dev » 8.x-dev
dixon_’s picture

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?

mustafau’s picture

This won't go into D7.

kompressaur’s picture

Status: Needs work » Needs review

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

sorry i never meant to do that :(

Status: Needs review » Needs work

The last submitted patch, 293318-78_aggregator_feeds_as_nodes.patch, failed testing.

Gábor Hojtsy’s picture

Issue tags: +D8MI

Tagging for Drupal 8 multilingual initiative.

nkschaefer’s picture

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.

catch’s picture

Title: Make Aggregator feeds into 1st class Drupal nodes » Make Aggregator feeds into entities

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

nkschaefer’s picture

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.

Gábor Hojtsy’s picture

Tagging for the content handling leg of D8MI.

Gábor Hojtsy’s picture

Issue tags: +language-content

Ha, it helps if I do it.

tim.plunkett’s picture

Title: Make Aggregator feeds into entities » Convert Aggregator feeds into ConfigEntity
Issue tags: +API change, +Configuration system
tim.plunkett’s picture

Title: Convert Aggregator feeds into ConfigEntity » Convert Aggregator feeds into entities
Issue tags: -Configuration system +Entity system

Nevermind.

Berdir’s picture

Status: Needs work » Needs review
FileSize
24.95 KB
FAILED: [[SimpleTest]]: [MySQL] 42,805 pass(es), 2 fail(s), and 0 exception(s). View

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.

moshe weitzman’s picture

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.

tim.plunkett’s picture

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

Status: Needs review » Needs work

The last submitted patch, aggregator-item-entity-293318-96.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
5.62 KB
28.71 KB
FAILED: [[SimpleTest]]: [MySQL] 42,796 pass(es), 2 fail(s), and 0 exception(s). View

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.

Status: Needs review » Needs work

The last submitted patch, aggregator-item-entity-293318-100.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
1.78 KB
29.72 KB
PASSED: [[SimpleTest]]: [MySQL] 42,806 pass(es). View

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.

fago’s picture

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

moshe weitzman’s picture

Status: Needs review » Needs work

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.

Berdir’s picture

Status: Needs work » Needs review
FileSize
7 KB
31.67 KB
FAILED: [[SimpleTest]]: [MySQL] 46,093 pass(es), 62 fail(s), and 584 exception(s). View

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

Status: Needs review » Needs work

The last submitted patch, aggregator-item-entity-293318-105.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
2.63 KB
33.03 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch aggregator-item-entity-293318-107.patch. Unable to apply patch. See the log in the details link for more information. View

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.

fago’s picture

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.

Berdir’s picture

Status: Needs review » Needs work
Issue tags: +Entity system, +API change, +D8MI, +language-content

The last submitted patch, aggregator-item-entity-293318-107.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
7.45 KB

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?

Berdir’s picture

FileSize
32.85 KB
PASSED: [[SimpleTest]]: [MySQL] 49,246 pass(es). View

And with the patch too.

Status: Needs review » Needs work
Issue tags: -Entity system, -API change, -D8MI, -language-content

The last submitted patch, aggregator-item-entity-293318-111.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
Issue tags: +Entity system, +API change, +D8MI, +language-content

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

Failed because of an unrelated deadlock.

ParisLiakos’s picture

FileSize
33.12 KB
PASSED: [[SimpleTest]]: [MySQL] 49,995 pass(es). View

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

Status: Needs review » Needs work
Issue tags: -Entity system, -API change, -D8MI, -language-content

The last submitted patch, aggregator-item-entity-293318-115.patch, failed testing.

ParisLiakos’s picture

Status: Needs work » Needs review
Issue tags: +Entity system, +API change, +D8MI, +language-content

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.

Berdir’s picture

FileSize
33.2 KB
PASSED: [[SimpleTest]]: [MySQL] 50,576 pass(es). View

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?

ParisLiakos’s picture

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

ParisLiakos’s picture

Assigned: Unassigned » ParisLiakos
Status: Needs review » Needs work

Found more

Berdir’s picture

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.

ParisLiakos’s picture

Assigned: ParisLiakos » Unassigned
Status: Needs work » Needs review
FileSize
3.7 KB
34.94 KB
PASSED: [[SimpleTest]]: [MySQL] 50,485 pass(es). View

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

ParisLiakos’s picture

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

Berdir’s picture

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

ParisLiakos’s picture

Assigned: Unassigned » ParisLiakos

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

ParisLiakos’s picture

FileSize
62.55 KB
FAILED: [[SimpleTest]]: [MySQL] 50,293 pass(es), 93 fail(s), and 223 exception(s). View

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

Status: Needs review » Needs work

The last submitted patch, aggregator-item-entity-293318-126.patch, failed testing.

Berdir’s picture

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

ParisLiakos’s picture

Status: Needs work » Needs review
FileSize
106.24 KB
PASSED: [[SimpleTest]]: [MySQL] 50,680 pass(es). View

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

ParisLiakos’s picture

Assigned: ParisLiakos » Unassigned

done for today

Berdir’s picture

Status: Needs review » Needs work

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

ParisLiakos’s picture

Status: Needs work » Needs review
FileSize
12.3 KB
104.83 KB
PASSED: [[SimpleTest]]: [MySQL] 49,602 pass(es). View

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

Berdir’s picture

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

moshe weitzman’s picture

Status: Needs review » Needs work

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?

ParisLiakos’s picture

FileSize
1.84 KB
105.71 KB
PASSED: [[SimpleTest]]: [MySQL] 50,791 pass(es). View

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

Berdir’s picture

Status: Needs work » Needs review
+++ 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'].

ParisLiakos’s picture

Assigned: Unassigned » ParisLiakos

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

ParisLiakos’s picture

FileSize
3.14 KB
105.91 KB
PASSED: [[SimpleTest]]: [MySQL] 50,804 pass(es). View

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

moshe weitzman’s picture

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

ParisLiakos’s picture

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

Berdir’s picture

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

ParisLiakos’s picture

Berdir’s picture

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

ParisLiakos’s picture

FileSize
105.55 KB
FAILED: [[SimpleTest]]: [MySQL] 50,626 pass(es), 34 fail(s), and 4 exception(s). View

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

Status: Needs review » Needs work

The last submitted patch, aggregator-entity-293318-144.patch, failed testing.

ParisLiakos’s picture

Status: Needs work » Needs review
FileSize
795 bytes
105.73 KB
PASSED: [[SimpleTest]]: [MySQL] 50,695 pass(es). View

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

ParisLiakos’s picture

FileSize
105.73 KB
PASSED: [[SimpleTest]]: [MySQL] 50,774 pass(es). View
+++ 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

Berdir’s picture

FileSize
990 bytes
106.51 KB
FAILED: [[SimpleTest]]: [MySQL] 50,673 pass(es), 119 fail(s), and 32 exception(s). View

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

Status: Needs review » Needs work

The last submitted patch, aggregator-entity-293318-148.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
558 bytes
106.51 KB
PASSED: [[SimpleTest]]: [MySQL] 50,694 pass(es). View

Ouch. langcode, not language...

Berdir’s picture

FileSize
778 bytes
106.33 KB
PASSED: [[SimpleTest]]: [MySQL] 50,817 pass(es). View

Also removed the langcode field from feeds.

Berdir’s picture

Category: feature » task

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.

plach’s picture

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

ParisLiakos’s picture

FileSize
106.35 KB
PASSED: [[SimpleTest]]: [MySQL] 48,746 pass(es). View

No longer applies, one more reroll

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

Awesome work. All recent objections are addressed, AFAICT.

moshe weitzman’s picture

moshe weitzman’s picture

Priority: Normal » Major

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

ParisLiakos’s picture

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

Dries’s picture

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.

ParisLiakos’s picture

@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

moshe weitzman’s picture

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

Dries’s picture

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.

ParisLiakos’s picture

Issue summary: View changes

A proper issue summary

ParisLiakos’s picture

Updated issue summary with issues above

ParisLiakos’s picture

Issue summary: View changes

remove paste failure

Dries’s picture

Status: Reviewed & tested by the community » Fixed

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

Dries’s picture

Issue summary: View changes

Updated issue summary.

ParisLiakos’s picture

Title: Convert Aggregator feeds into entities » Change notice: Convert Aggregator feeds into entities
Priority: Major » Critical
Issue tags: +Needs change record

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

ParisLiakos’s picture

Issue summary: View changes

Add multilingual

ParisLiakos’s picture

Status: Fixed » Needs review
Issue tags: -Needs change record

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

Berdir’s picture

Title: Change notice: Convert Aggregator feeds into entities » Convert Aggregator feeds into entities
Priority: Critical » Major
Status: Needs review » Fixed

Made some small adjustments, looks good otherwise.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

sun’s picture

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?

sun’s picture

Issue summary: View changes

add multilingual issue link

izmeez’s picture

Issue summary: View changes

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.