Full views support, admin/content/feed powered by views
| Project: | FeedAPI |
| Version: | 6.x-1.x-dev |
| Component: | Code views integration |
| Category: | task |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed |
It looks like a lot of the views code was written back before the views relationships were finished so there's a bunch of redundant code that can be streamlined. Most notably the way that feed.views.inc's {feed} table is duplicated by feed_node.views.inc under a different name. Since feed_node depends on feed we can use feed's views support and provide a relationship so the user can access fields from both the feed and feed_item nodes.
In the process I was able to remove all the link handlers and replace them with a single one that gives the option to use the node title. Unfortunately I couldn't figure out the correct way to have the patch remove the handlers so it just empty the files.
Also, I updated the default view but if people have build custom views they'll need to make some changes to them. I'd be happy to help write some documentation on how to use the relation, or provide some more complicated views that we could ship as defaults that are disabled by default.
| Attachment | Size |
|---|---|
| feedapi_views.patch | 26.15 KB |
#1
I closed a couple of related issues: #289065: Additional fields in feedapi_node_views.module and #334636: Blocks, Views, or Views Argument for each Feed Item
#2
Closed a few more: #264338: Feed List, #308981: Can FeedAPI store RSS feeds for individual users?, #251957: Filter by Parent Feed Title
#3
The default view for the 'feed-item' path seemed mangled to me when I use the patch (It shows 'Invalid' view type). Removing $view->display['page'] = $display; from line 124 seemed to resolve it.
#4
After talking to alex_b I went ahead and filled out the views support so that I could replace the admin page with a view. I dropped the statistics since alex_b said they didn't scale with many feeds and weren't really views accessible (I'd be willing to help with refactoring the statistics so that they can be computed and stored in a more views friendly manner in a subsequent issue).
Jody Lynn, thanks, this should fix that bug.
#5
Patch applied cleanly on latest dev using OS X from command line. I installed patched module on a D6.8 system that already had FeedAPI 1.5 running with a few test feeds as well as Feed Element Mapper. I had changed the default view so reverted in order to pick up the new Views stuff provided by the patch. I am pretty new to Relationships in Views but it appeared to work fine. The only thing I saw was the Parent Feed was also showing on the Feed-Item view until I went into Relationship settings and clicked "Require this relationship" - After I did that the Parent Feed no longer showed up in the list (which is the behavior I wanted). Otherwise it appears to be working very well.
Patched module demo here: http://d6.metanomy.org/feed-item and if you go deep into the pager you can see Flickr and YouTube being mapped that still work fine.
On the Admin View: The concept of providing "admin" functions via a View caught me by surprise. Perhaps because Views is not listed as a dependency for FeedAPI? It may not be clear how the permissions on this View are handled. Access was set to "unrestricted" in the View. If you set the "access all views" permission for anonymous they can see the view but not the fields that provide the admin function - a good thing. I guess my feeling is that this View, if it has "admin" in the title, should be set to access>>permission>>administer feedapi by default. Or this could be called Parent Feeds (or something) and not given a path to or name that has anything to do with "admin" - It is a helpful view for anyone who would like to see a tabular listing of parent feeds on the site, and the edit/delete column would only show for people with the right permissions?
Anyway, nice work.
R,
Coby
#6
drewish: The patch seems to be perfect. Nice job! :)
I modified the following:
- only privilegised (administer feedapi) users can view the admin/content/feed page
- Above the commands, the column header was Delete, i modified it to Commands
I have only one question now to everyone: is it a problem if FeedAPI depends on Views?
If this patch will be committed, this dependency has to be introduced. :(
#7
At the admin views, there is a filter:
'type' => array('operator' => 'in',
'value' => array(
'feed' => 'feed',
),
This is not appropiate. The user can create unlimited feed content-types. I imagine it's very hard to filter to a serialized variable in the variable table.
Also, the simpletest tests are need to be updated because of the admin page change.
#8
#5: I think the admin/content/feed view should stay in the admin section: It should show all feeds, regardless whether they are published or not.
#6: I see your concern, but a hard dependency (.info) on views doesn't seem necessary. With this patch, FeedAPI will still work fine, there's just less functionality without views. Which is ok, given the fact that only very specific sites run without views. (and - it doesn't take away from FeedAPI given the fact that admin/content/feed is broken on sites with many feed items at the moment).
#7: Looks like the type array needs to be built dynamically. Which means we'll have to flush the views cache every time we enable/disable feedapi on a content type. This will mean some views related code in feedapi.module. A toggle in feedapi_content_type_submit() could flush the views cache if a _change_ in $form_state['feedapi']['enabled'] is detected (I'm winging this variable here, don't nail me on it). We should still keep feedapi and views loosly coupled so the view dependent code should be wrapped in module_exists('views').
Nice work. Really great to see this coming together.
#9
I am OK with the dependency if it goes that way. I don't use FeedAPI without Views, but that may not be the case for everyone. If I understand correctly some of this FeedAPI architecture is heading for D7 core aggregator? Not sure if that makes a difference for upgrade path etc.
In my testing even with the admin view set to access>>permission>>administer feedapi an anonymous user can still see the view if the global permission "access all views" is selected via admin panel. Probably need to make sure that is documented.
And I don't think Views does become a dependency as long as we do not use it to replace an otherwise available admin function. In this case I don't think I can see a list of feeds to easily perform admin operations on them without Views. It doesn't break it, but makes it harder for non Views users to admin feeds.
m2c
R,
Coby
#10
Not sure if this is because I had 1.5 installed before I applied this patch, but I am getting some errors for the following fields:
FeedAPI Feed Url - Error: handler for feedapi > url doesn't exist!
FeedAPI Feed Link - Error: handler for feedapi > link doesn't exist!
FeedAPI Feed Item URL - Error: handler for feedapi_node_item > url doesn't exist!
I think that most of these have been replaced by using the new relationship to parent feed with node xxx, but they are still showing up in my list of choices under fields.
[edit] sorry, that was for patch in #4
R,
Coby
#11
#9 The fact that the admin section of feedapi doesn't work without views doesn't bother me so much as this one:
This could be a real problem as it can expose unpublished feeds without the builder really being aware of it. I don't know what peope use the 'access all views' permission for, but using a view in the admin section might end up screwing up some build strategies.
Given these problems, I'd like to suggest to step back from making admin/content/feeds a view (I know, drewish, I gave you the initial go on this :-( ) or at least make it a separate issue from "full views support".
#12
I applied the patch in #6. I could not get get the "Parent feed" to work as a filter, but it does work when given the parent feed node ID as an argument.
Edit: It does work as a filter if the "Parent Feed" relationship is not used with the filter.
#13
@jtsnow - you should be able to establish the parent feed relationship and then filter by node id or node tile or node xyz using said relationship in the configuration settings of your filter. wow, not sure that makes sense now that i reread. hard to explain without a screenshot.
Which makes me wonder if Feed Item: Parent Feed should be removed from everything but the Relationship part of Views? Not sure.
#14
cglusky, Yep, works great! Now that I understand relationships, it's not confusing. But, initially, having 'Parent Feed' if several places caused some confusion.
#15
#14: I had the impression that there is a parent feed option too many somewhere - can't the parent feed filter or argument be built with a relationship now? Should we skip it therefore altogether or is it a good shortcut for a very common task? (Think of the author options on nodes, they do have an implicit relationship between node and user table as well).
#16
alex_b,
Yes, it should all be able to be built with a relationship now. Not sure about how to best cover the possible use cases and/or if leaving the parent feed references in options besides relationship may impact functionality.
r
c
#17
for some reason i wasn't subscribed to this issue.
looks like jtsnow had forked this code and was working on it on #251957: Filter by Parent Feed Title. i marked that as a duplicate so we can stay focused on one patch.
#18
okay, removed some unrelated changes to the parser_common_syndication that were in the patch on #6.
i left the admin default view but made it disabled by default. views does it's menu additions in hook_menu_alter() so people could overwrite the default admin pages with views if they'd like to.
re #9 and #11: the 'access all views' permission affects all views. If you're foolish enough to grant it with out understanding it what it's for then of course it'll have side effects.
re #7: since the admin views are disabled by default at this point i think it's fair enough to assume that people who are going to enable it would know how to change the node type filter.
#19
forgot to fake add those new .inc files.
#20
thanks drewish. comments about view permissions seem fair enough to me. i should be able to test this in the next week. have another project going with mail2web.
#21
Nice work.
#18: I agree with your comments on #9 and #11.
In regards to #7 : FeedAPI's views integration should offer a filter "is feed type" that the default view uses. That way the admin view can be left unchanged when feed content types are added or removed. Would that work?
#22
that'd be a nice addition but not required for this patch since the admin view isn't enabled by default.
#23
also should add that it's a feature that isn't present in the current version. i'd be happy to drop the default admin view if that would help get this committed.
#24
#23: shouldn't be necessary: #360061: Create views filter for whether a content type is used as feed or not
#25
#19: I like renaming feedapi_standalone to feedapi. This change will cause some breakage on existing sites though, right? How could we avoid that?
#26
#19 applied cleanly against latest dev and is working fine with a fresh D6.9 install.
#27
the whole thing will break views if people have customized them but the default views should keep working. i'd say just document it in the release notes.
#28
Fixed/improved:
* admin/content/feed uses #360061: Create views filter for whether a content type is used as feed or not
* added description to admin/content/feed view
* change the name of feedapi_handler_filter_feed_type to feedapi_handler_filter_feed_content_type to avoid a namespace collision
#29
#28 applies cleanly against latest dev version and works on fresh install of D6.9. lightly tested new and improved views support. nothing to report other than good job:)
#30
Maybe this applies cleanly if you're operating a cv archive, but I got 18/1 dev out and tried to patch and noooo waaay. :(PSEUDO-STRIKE I could really use this immediately, so if there's any danger of rolling it into dev for "easier" testing, that would be sweet. END PSEUDO-STRIKE
Please use
patch -p0 < file#31
#27: I agree. I will put up a clear warning and upgrade instructions for the release containing this patch so that people realize that they will need to reconfigure feedapi related views when they upgrade.
Not ideal, but the naming convention proposed here is much clearer.
#32
One thing I saw this morning was the description does not show in admin. I have the feed admin view enabled.
SS attached
#33
#32: I'm not even sure whether this can be done: #293832: Adding a description to a menu pointing at a view. ? - wouldn't be a show stopper for this patch as the admin view is off by default anyway.
#34
#33 agreed; not a show stopper; just a note; have no idea if it can be done either.
#35
I just posted a patch to #293832: Adding a description to a menu pointing at a view. so we can revisit that point once the issue is resolved.
#36
I did a full review of all filters, fields and arguments and they to be all sound except one:
* Fixed: had to rename *feedapi_handler_feedapi_url* to *feedapi_handler_field_url* because otherwise views couldn't find handler with the same name.
In feedapi/views/handlers we now have field handlers with the following names:
feedapi_handler_field_url
feedapi_handler_field_node_link_purge
feedapi_handler_field_node_link_refresh
feedapi_handler_filter_feed_content_type
feedapi_handler_filter_url_unique
The following will be removed from feedapi/views/handlers:
(to be removed) feedapi_handler_filter_feed_type
(to be removed) feedapi_handler_feedapi_title_link
(to be removed) feedapi_handler_feedapi_title_nid
(to be removed) feedapi_handler_feedapi_title_url
All handlers from feedapi/feedapi_node/views/handlers will be removed:
feedapi_node_handler_feedapi_item_title_url
feedapi_node_handler_feedapi_title_link
feedapi_node_handler_feedapi_title_nid
feedapi_node_handler_feedapi_title_url
#37
New .inc files included.
#38
applied it to my site, and the default views all looked good as did a custom one i'd created. i'm sure there'll be small followup issues but i'd say this is RTBC
#39
#37 patch needs to be applied to dev, or on top of the previous patch(es), please? (this is rarely clear... maybe I should know, but...)
#40
NikLP it applies to DRUPAL-6--1 cleanly.
#41
Committed.
Thanks drewish for the great work. Thanks everybody else for testing and input.
#42
#293832: Adding a description to a menu pointing at a view. just got committed so we could do a followup issue to add descriptions to the default views.
#43
Automatically closed -- issue fixed for 2 weeks with no activity.