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.
Comment | File | Size | Author |
---|---|---|---|
#154 | aggregator-entity-293318-154.patch | 106.35 KB | ParisLiakos |
#151 | aggregator-entity-293318-151.patch | 106.33 KB | Berdir |
#151 | aggregator-entity-293318-151-interdiff.txt | 778 bytes | Berdir |
#150 | aggregator-entity-293318-150.patch | 106.51 KB | Berdir |
#150 | aggregator-entity-293318-150-interdiff.txt | 558 bytes | Berdir |
Comments
Comment #1
mustafau CreditAttribution: mustafau commentedI have updated the patch.
Most of the tests are passing now.
OPML import is working.
Comment #2
Aron NovakClean and nice! I'll test it soon.
Comment #3
Aron NovakI tried to apply the patch but I got rejects. Should I reroll the patch or do you plan to do?
Comment #4
mustafau CreditAttribution: mustafau commentedThis changes every DB query in the Aggregator module. I think we should wait until DB:TNG patch gets committed.
Comment #5
alex_b CreditAttribution: alex_b commentedGreat to see this one being tackled. #4: agree, let's wait until #302936: DB TNG: Convert aggregator.module queries is committed.
Comment #6
alex_b CreditAttribution: alex_b commentedThis 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.
Comment #7
Aron NovakRerolled after the DB:TNG and the pluggable patches.
Comment #8
catchSubscribing. 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.
Comment #9
Aron NovakIf 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 :)
Comment #10
catchIn 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.
Comment #11
moshe weitzman CreditAttribution: moshe weitzman commentedWe 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.
Comment #12
SeanBannister CreditAttribution: SeanBannister commentedsub
Comment #13
Dries CreditAttribution: Dries commentedI 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.
Comment #14
catchReading 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.
Comment #15
alex_b CreditAttribution: alex_b commentedGreat 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?
Comment #16
chx CreditAttribution: chx commentedI 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.
Comment #17
alex_b CreditAttribution: alex_b commentedWorking on #15 now
Comment #18
alex_b CreditAttribution: alex_b commentedThis 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.
Comment #19
alex_b CreditAttribution: alex_b commented* 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
Comment #20
alex_b CreditAttribution: alex_b commentedRerolled patch, updated to latest changes.
Comment #21
alex_b CreditAttribution: alex_b commentedI'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.
Comment #22
bsherwood CreditAttribution: bsherwood commentedI 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
Comment #23
SeanBannister CreditAttribution: SeanBannister commentedThe 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.
Comment #24
catch@specmav and SeanBannister. This patch is about having feeds as nodes, not indvidual feed items - so neither of those concerns are affected.
Comment #25
bsherwood CreditAttribution: bsherwood commentedWell, 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.
Comment #26
catchspecmav, 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.
Comment #27
bsherwood CreditAttribution: bsherwood commentedI 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.
Comment #28
SeanBannister CreditAttribution: SeanBannister commentedOh sorry, kinda got side tracked by specmav's post :)
Comment #29
alex_b CreditAttribution: alex_b commentedChanges:
* 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.
Comment #31
alex_b CreditAttribution: alex_b commentedOops, forgot to cvs add a new file (template for info on feed node pages).
Comment #32
alex_b CreditAttribution: alex_b commentedThis 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.
Comment #33
alex_b CreditAttribution: alex_b commentedSmall 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()
Comment #35
alex_b CreditAttribution: alex_b commentedRerolled to reflect recent changes to core. Still needs work: I'd like to add an API test.
Comment #36
alex_b CreditAttribution: alex_b commentedAdded 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
Comment #37
Aron NovakI tried it out. It's nice that this patch is really solid now :)
I see some minor problems:
Comment #38
Aron NovakHere is the updated patch.
Comment #39
mustafau CreditAttribution: mustafau commentedAdded 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.
Comment #40
mustafau CreditAttribution: mustafau commentedWhat will happen to feed nodes when uninstalling Aggregator?
Comment #41
alex_b CreditAttribution: alex_b commented#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...
Comment #42
catchThat's the same as forum, book and any other module-provided content type in core, so not something which needs to be fixed here.
Comment #43
alex_b CreditAttribution: alex_b commentedI 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.
Comment #44
alex_b CreditAttribution: alex_b commented* Clarify node tab label: "Update feed" instead of "Update items".
Comment #45
Jose Reyero CreditAttribution: Jose Reyero commentedThe 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?
Comment #46
alex_b CreditAttribution: alex_b commentedJose, 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
Comment #47
alex_b CreditAttribution: alex_b commented#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.
Comment #48
catchFor 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).
Comment #49
alex_b CreditAttribution: alex_b commentedcatch, 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.
Comment #50
catchalex_b - yeah taht's what I meant. Thanks for re-opening #15266: Replace aggregator category system with taxonomy.
Comment #51
Aron NovakHere 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.
Comment #53
Aron NovakI messed up the test module under /tests/ at the last patch.
Comment #55
alex_b CreditAttribution: alex_b commentedThe test in #53 failed because of aggregator-feed-info.tpl.php and changes to test module missing. This patch should pass.
Reviewing now.
Comment #56
alex_b CreditAttribution: alex_b commentedJust 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:
Comment #57
tstoecklerI 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.
Comment #58
BerdirJust looking through the patch, some minor things, mostly related to DBTNG
Can't you use isset() && is_object() instead ? @ is ugly and slow...
These should be converted to multi-line calls.
- 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.
Comment #59
alex_b CreditAttribution: alex_b commented#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.
Comment #60
Jose Reyero CreditAttribution: Jose Reyero commentedIt 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
Comment #61
alex_b CreditAttribution: alex_b commented#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() ?
Comment #62
alex_b CreditAttribution: alex_b commented- needs content upgrade path
- catch situation when a content type 'feed' already exists
Comment #63
akahn CreditAttribution: akahn commentedThis 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.
Comment #64
akahn CreditAttribution: akahn commentedIs this right?
Comment #65
alex_b CreditAttribution: alex_b commented#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.
Comment #66
alex_b CreditAttribution: alex_b commentedChanges:
- 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.
Comment #67
tstoecklerThe update doesn't work: I get the following error message which disrupts the update process:
The error page I am then led to says:
Comment #68
alex_b CreditAttribution: alex_b commented- 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.
Comment #70
alex_b CreditAttribution: alex_b commentedResolve recent conflicts. Working on batching now.
Comment #71
alex_b CreditAttribution: alex_b commented- 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.
Comment #72
chx CreditAttribution: chx commentedI 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.
Comment #73
alex_b CreditAttribution: alex_b commented#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.
Comment #74
chx CreditAttribution: chx commentedI think this is ready -- let's see what others say :)
Comment #76
alex_b CreditAttribution: alex_b commentedRerolled to reflect recent changes by #220233: Put node_get_types() out of its misery and #480102: Update aggregator module to use drupal_static().
Comment #77
alex_b CreditAttribution: alex_b commentedTest bot did its work, setting back to RTBC now.
Comment #79
mustafau CreditAttribution: mustafau commentedCoder generated this warning:
Comment #80
Dries CreditAttribution: Dries commentedWhile 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 ...
Comment #81
webchickHm. 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?
Comment #82
NaheemSays CreditAttribution: NaheemSays commentedJust a question (without testing) - to address dries concerns, do the feeds themselves really need to be nodes? Why not use nodes for feed items?
Comment #83
mustafau CreditAttribution: mustafau commentedComment #84
dixon_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?
Comment #85
mustafau CreditAttribution: mustafau commentedThis won't go into D7.
Comment #86
kompressaur CreditAttribution: kompressaur commented#79: 293318-78_aggregator_feeds_as_nodes.patch queued for re-testing.
sorry i never meant to do that :(
Comment #88
Gábor HojtsyTagging for Drupal 8 multilingual initiative.
Comment #89
nkschaefer CreditAttribution: nkschaefer commentedIn 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.
Comment #90
catchYep, these should almost definitely be entities, re-titling.
Comment #91
nkschaefer CreditAttribution: nkschaefer commentedBecause 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.
Comment #92
Gábor HojtsyTagging for the content handling leg of D8MI.
Comment #93
Gábor HojtsyHa, it helps if I do it.
Comment #94
tim.plunkettComment #95
tim.plunkettNevermind.
Comment #96
BerdirOk, 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.
Comment #97
moshe weitzman CreditAttribution: moshe weitzman commentedLooks nice.
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.
ewww. I haven't looked into this, but we generally try to call render($foo) from the template, and not render stuff in preprocess.
Comment #98
tim.plunkettTo 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
Comment #100
BerdirCan'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.
Comment #102
BerdirAh, 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.
Comment #103
fagoWe want to get away from "iid" id properties. This can be a follow up though.
Is the link a proper URI? In that case I think we should add the 'uri_field' which has an URI as primitive value.
This should be a date_field.
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.
Comment #104
moshe weitzman CreditAttribution: moshe weitzman commentedin 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.
Comment #105
Berdir- 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.
Comment #107
BerdirForgot 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.
Comment #108
fagoWe'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.
Comment #109
Berdir#107: aggregator-item-entity-293318-107.patch queued for re-testing.
Comment #111
BerdirRe-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?
Comment #112
BerdirAnd with the patch too.
Comment #114
Berdir#112: aggregator-item-entity-293318-111.patch queued for re-testing.
Failed because of an unrelated deadlock.
Comment #115
ParisLiakos CreditAttribution: ParisLiakos commentedRerolled 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
Comment #117
ParisLiakos CreditAttribution: ParisLiakos commentedSeems 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.
Comment #118
BerdirRe-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?
Comment #119
ParisLiakos CreditAttribution: ParisLiakos commentedhmm, 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 getFatal error: Cannot use object of type stdClass as array in /var/www/d8/core/modules/aggregator/aggregator.admin.inc on line 606
Comment #120
ParisLiakos CreditAttribution: ParisLiakos commentedFound more
Comment #121
Berdirfor 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.
Comment #122
ParisLiakos CreditAttribution: ParisLiakos commentedSeems 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
andaggregator/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 appearComment #123
ParisLiakos CreditAttribution: ParisLiakos commentedI think aggregator categories should be taxonomy..see also #15266: Replace aggregator category system with taxonomy
Comment #124
BerdirUsing 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 ;)
Comment #125
ParisLiakos CreditAttribution: ParisLiakos commentedworking on feeds then.converting em to fieldable content entities with no bundle types, kinda like user module..
Comment #126
ParisLiakos CreditAttribution: ParisLiakos commentedmy 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
Comment #128
BerdirShould be Contains now.
I would suggest to not add the reset argument, I want to get rid of that from load functions anyway...
Should be \Drupal\Core\...
Typo.
This should move to the init function now.
Comment #129
ParisLiakos CreditAttribution: ParisLiakos commentedthis got big pretty fast:/
but i think its ready now..tests should pass
Comment #130
ParisLiakos CreditAttribution: ParisLiakos commenteddone for today
Comment #131
BerdirLooks great!
Some comments below
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
When a namespace is used then it should start with a \
Same here, @param \Drupal...
Another one that should be Contains and have a \ :)
Might be able to search from them with " Drupal\" in the new files....
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!)
Comment #132
ParisLiakos CreditAttribution: ParisLiakos commentedThanks for the review Berdir
Agreed:)
I think i fixed all namespaces
Comment #133
BerdirOk, 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? :)
Comment #134
moshe weitzman CreditAttribution: moshe weitzman commentedJust minor stuff I noticed. Only the first one is a prerequisite for RTBC, IMO.
theme_item_list() now take render arrays as items so we don't need to call drupal_render() here.
Thats pretty old school. Can we not use a title_callback in aggregator_menu()?
Again, can title be set in a title_callback in hook_menu.
Again, can title be set in a title_callback in hook_menu?
Comment #135
ParisLiakos CreditAttribution: ParisLiakos commentedThanks for the review:)
Abouttitle_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
Comment #136
BerdirI 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'].
Comment #137
ParisLiakos CreditAttribution: ParisLiakos commentedI 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
Comment #138
ParisLiakos CreditAttribution: ParisLiakos commentedWell, i removed
aggregator_feed_edit
and used directlyentity_get_form
inhook_menu
..Also added a title callback for view page.not sure if its better now, but i guess #135 could be commited instead:)Comment #139
moshe weitzman CreditAttribution: moshe weitzman commentedThat title_callback is perfect. If you don't mind, there were a couple other instances that need it. See #134.
Comment #140
ParisLiakos CreditAttribution: ParisLiakos commentedYeah, 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
Comment #141
BerdirYes, that is exactly the difference between menu link title and page title, which is not always the same.
Comment #142
ParisLiakos CreditAttribution: ParisLiakos commented#138: aggregator-entity-293318-138.patch queued for re-testing.
Comment #143
BerdirYou can use entity_page_label() for this, instead of having to add a separate helper function.
Comment #144
ParisLiakos CreditAttribution: ParisLiakos commentedpatch no longer applied, here is a rerolled one that incorporates #143 as well
entity_page_label() rocks, thanks Berdir:)
Comment #146
ParisLiakos CreditAttribution: ParisLiakos commentedhad to add a langcode field for tests to pass...no idea what happens there tbh, but now tests pass
Comment #147
ParisLiakos CreditAttribution: ParisLiakos commentedyes..should be aggregator feed item..fixed in the following
Comment #148
BerdirOk, 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! :)
Comment #150
BerdirOuch. langcode, not language...
Comment #151
BerdirAlso removed the langcode field from feeds.
Comment #152
BerdirThis 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.
Comment #153
plach@Berdir asked me to have a look to this hunk: the change looks totally sensible to me.
Comment #154
ParisLiakos CreditAttribution: ParisLiakos commentedNo longer applies, one more reroll
Comment #155
moshe weitzman CreditAttribution: moshe weitzman commentedAwesome work. All recent objections are addressed, AFAICT.
Comment #156
moshe weitzman CreditAttribution: moshe weitzman commented#154: aggregator-entity-293318-154.patch queued for re-testing.
Comment #157
moshe weitzman CreditAttribution: moshe weitzman commentedTrying to avoid reroll hell for a key Entity adoption patch.
Comment #158
ParisLiakos CreditAttribution: ParisLiakos commentedtoday is my birthday..i would love this as present :P
also opened #1905470: Convert Aggregator categories to config entities
Comment #159
Dries CreditAttribution: Dries commentedThis 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.
Comment #160
ParisLiakos CreditAttribution: ParisLiakos commented@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
Comment #161
moshe weitzman CreditAttribution: moshe weitzman commentedIMO 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
Comment #162
Dries CreditAttribution: Dries commentedI 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.
Comment #162.0
ParisLiakos CreditAttribution: ParisLiakos commentedA proper issue summary
Comment #163
ParisLiakos CreditAttribution: ParisLiakos commentedUpdated issue summary with issues above
Comment #163.0
ParisLiakos CreditAttribution: ParisLiakos commentedremove paste failure
Comment #164
Dries CreditAttribution: Dries commentedAlright! Committed to 8.x. Now let's work on the follow-up issues.
Comment #164.0
Dries CreditAttribution: Dries commentedUpdated issue summary.
Comment #165
ParisLiakos CreditAttribution: ParisLiakos commentedI am gonna write a change notice for it later. also added multilingual feeds/feed items to follow up issues
Comment #165.0
ParisLiakos CreditAttribution: ParisLiakos commentedAdd multilingual
Comment #166
ParisLiakos CreditAttribution: ParisLiakos commentedChange notification here http://drupal.org/node/1905910
Comment #167
BerdirMade some small adjustments, looks good otherwise.
Comment #169
sunCan 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?
Comment #169.0
sunadd multilingual issue link
Comment #170
izmeez CreditAttribution: izmeez commentedWould 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.