#412518: Convert taxonomy_node_* related code to use field API + upgrade path happily removed all taxonomy RSS element additions while it did not add any replacement to make it work any other way. Grepping for rss_elements (the only way to add RSS items to core node RSS feeds), only comment module and node module appear. So looks like no alternate way to do it via standard field mechanisms either. I honestly don't know how this is supposed to work, and consider the removal of features post feature-freeze an issue. Also, the lack of mention/discussion of this feature removal on the issue itself makes me uncomfortable, making this look like a total accident.
Comment | File | Size | Author |
---|---|---|---|
#46 | taxonomy_tags_rss_feeds-872488-d7-46.patch | 4.55 KB | Albert Volkman |
#44 | taxonomy_tags_rss_feeds-872488-d7-44.patch | 4.55 KB | Albert Volkman |
#38 | taxonomy-rss-regression-test-872488-38.patch | 4.59 KB | acouch |
#38 | interdiff.txt | 674 bytes | acouch |
#35 | 872488-test-only.patch | 3.41 KB | no_commit_credit |
Comments
Comment #1
Damien Tournoud CreditAttribution: Damien Tournoud commentedRelated: #666412: Regression: RSS feed enclosure support lost from core
I'm not sure how to do that, actually. One way to do this could be to define RSS-specific formatters that output stuff to
$entity->rss_elements
instead of returning something. That would definitely work, but that doesn't feel very pretty.Comment #2
andyposttaxonomy still very badly implemented yet
Comment #3
bjaspan CreditAttribution: bjaspan commentedsubscribe
Comment #4
Anonymous (not verified) CreditAttribution: Anonymous commentedI probably have some brains for this and #666412: Regression: RSS feed enclosure support lost from core after my criticals.
Comment #5
dropcube CreditAttribution: dropcube commentedSubscribing.
Comment #6
mfbAny more thoughts on this and #666412: Regression: RSS feed enclosure support lost from core? Is there a contrib module that replaces the features?
Comment #7
mfbHere's a patch to restore the feature (or else can join RSS enclosure in a little RSS field formatters contrib module).
Comment #8
mfbok here's a contrib module to restore this feature: http://drupal.org/project/rss_field_formatters
Comment #9
quicksketchYou can do this in Drupal 7 by doing the following:
- Add a taxonomy term reference field to your content type (Taxonomy is now a field)
- Click "Display fields" tab on the content type.
- Under the "Show Custom display settings" fieldset, enable a custom display setting for RSS feeds
- Click on the new "RSS" sub-tab.
- Configure the Taxonomy term reference field to show up with the "Link" formatter.
Alternatively you don't even need to add a custom display setting for RSS feeds at all if you would like them to appear the same as the the default (typically "Full version") view.
Comment #10
quicksketchEr, actually sorry I misunderstood this request. I didn't realize that RSS feeds commonly provide "category" tags that are separate from the content. Patch in #7 looks pretty good. Any takers on reviewing?
Comment #11
mfbIn Drupal 8 it'd be nice if view modes and field formatters were aware of their output format. Then this field formatter could indicate that it outputs XML, and it'd only be a selectable option for view modes that have XML output. Other output formats might include HTML, plain text e-mail, iCalendar, PDF, CSV, etc. There's probably another issue for this, but thought I'd mention it here as an example use case.
Comment #12
mstef CreditAttribution: mstef commentedI'm not seeing any change in the feed after applying patch #7.
EDIT: Seems to be working now. The feed must have been cached.
Comment #13
jessebeach CreditAttribution: jessebeach commentedThe patch works. The tags apply correctly in the default view as well as the RSS view.
Screenshot of RSS
<category>
tags:https://skitch.com/jesse.beach/g8pye/gardens.dev-rss.xml
The configuration is a little clunky with the field formatter, but I'd rather have this in and working, so let's print it!
Comment #14
xjmThis will need to be rerolled against D8 (and for git). Also, we should add test coverage for this.
Tagging novice for the reroll. Thanks!
Comment #15
xjmMissed a tag.
Comment #16
musicnode CreditAttribution: musicnode commentedRe-rolled against D8 via git.
Comment #17
xjmThanks @musicnode!
Sending to the bot. Still need a test.
Comment #18
xjmAlright, the patch in #16 passed, so the next thing we need is an automated test.
Take a look at the tests for RSS in
node.test
,comment.test
andfield_ui.test
to get ideas. (The test for this patch should be added totaxonomy.module
's tests, though).Comment #19
acouch CreditAttribution: acouch commentedI can confirm that this works, but the implementation I imagine would be really confusing for users. I think most would not know enough to add the rss view mode and change the formatter for the term reference field.
Since the term/[term-id]/feed is added by default shouldn't the
<category>
in the feed be added by default as well?I'd propose adding an alter to node_feed so taxonomy and file and any other modules can add their additional channels the node feeds. This would address #666412: Regression: RSS feed enclosure support lost from core as well.
In that issue @mfb suggested http://drupal.org/node/666412#comment-3289312 having modules define default formatters for view modes which might make sense for other pages utilizing default view modes but would mean updating (as far as I can tell) field api which is a bigger issue.
Comment #20
jessebeach CreditAttribution: jessebeach commentedacouch, I'd love to see this patch get committed as soon as possible. We can refine the method in subsequent issues (such as #666412: Regression: RSS feed enclosure support lost from core). Until we get this patch in, we have no support for tags in RSS feeds, which is a huge gaping hole in Drupal 7.
Comment #21
acouch CreditAttribution: acouch commentedAttached is a patch that adds the category by default by using hook_node_view_alter().
Comment #22
mfbJust wanted to point out that there is a Drupal 7 contrib module - http://drupal.org/project/rss_field_formatters - so it's a not such a huge gaping hole.
Re: the patch at #21, is there any concern about field permissions? What if the user does not have permission to see that term reference? Also, doing a database query per node is not happy.
Comment #23
xjmRegarding #20, the query in #21 maybe needs the
term access
tag?I should say I personally prefer the approach in #16. I do see the point about it not being intuitive, but I wonder if we could supply a good default for the view mode as a compromise?
Edit: Need to test it myself and see what the UX is like, I guess.
Edit 2: And whatever approach we take, we need a test. :)
Comment #24
xjmTalked to mfb about this a bit on IRC. More thoughts:
standard.profile
.Comment #25
acouch CreditAttribution: acouch commentedAre we sure there isn't a way to make the formatter from #872488-16: Regression: no way to get taxonomy tags into RSS feeds a default for RSS feeds?
| I'm still leaning toward the field formatter/view mode approach, but at a minimum maybe we should bake a formatter for the article tags field into standard.profile.
That would be an improvement. Wondering if there would be anything else to let users know they need to add this formatter to get rss feeds to show terms in rss feeds. A hook_help() entry would help ;) . Is there something else we can add to the UI?
| Our test coverage should include an access check.
I noticed there this already a 'Node feed' test that tests the output of the node_feed() function. Should we create a new test or add
if(module_exists('taxonomy'))
to that test?Comment #26
xjmThat would probably be the best solution, I think. Just not quite sure how offhand.
Probably not in D7, unfortunately.
Well, ideally, we'll have a full test for the RSS feed in
taxonomy.test
that's similar to the node RSS test, including an access check. Sincetaxonomy.module
is providing the functionality (andnode.module
will soon be optional), it should provide the test cases, I think.Comment #27
acouch CreditAttribution: acouch commentedI'll work on the test in the taxonomy module. I guess testing is not a place to worry too much about duplication of effort.
I'll also look into updating the standard profile.
Comment #28
acouch CreditAttribution: acouch commentedWorking on a test.
Comment #29
acouch CreditAttribution: acouch commentedAttached is a test for the RSS feed. Let me know if you have any feedback.
Comment #30
acouch CreditAttribution: acouch commentedOops. Saw that I mislabeled the new class. Re-submitting.
Comment #32
acouch CreditAttribution: acouch commentedCan someone help explain this result: http://qa.drupal.org/pifr/test/218438 ?
Since the test failed for testHtmlEntitiesSample() in the aggregator module test does that mean this patch test passes? Thanks!
Comment #33
andypostThis caused a file messed in commit #61456-87: Aggregator titles display quotes and other characters with HTML entity equivalents badly (write tests)
Comment #34
acouch CreditAttribution: acouch commented#30: taxonomy-rss-regression-test-872488-31.patch queued for re-testing.
Comment #35
no_commit_credit CreditAttribution: no_commit_credit commentedUploading a version of the patch with only the test to expose the failure.
Comment #37
xjmExcellent, that test looks good to me. The appropriate failures without the fix are exposed above. (The earlier test failures were just due to the branch being broken at the time.)
A few minor nitpicks for code style cleanup and docs clarifications:
Maybe:
Tests the rendering of term reference fields in RSS feeds.
Very minor point, but the verb forms here should be "Tests"/"Creates"/"asserts". (Reference: http://drupal.org/node/1354#functions)
Let's capitalize RSS here.
In general, it's best not to use
t()
on assertion messages (the last argument), because no one will be translating them. A plain string literal can be used, orformat_string()
if there's variables to be formatted/sanitized. Reference: #500866: [META] remove t() from assert message(Note that this only applies to the message argument itself; strings that are part of the expected verbose output should still use t() where appropriate.)
Also, let's capitalize RSS here too.
Aside from those things, the patch looks ready to me. Thanks!
Comment #38
acouch CreditAttribution: acouch commentedThanks for the review. Attached is the updated patch.
Comment #39
acouch CreditAttribution: acouch commentedUpdating issue status.
Comment #40
xjmNote for reviewers: The interdiff is backwards. :)
I think this is ready!
Comment #41
catchOK this looks fine to me. The test probably could've been done as a pure functional test (i.e. all API calls instead of web test case), but many taxonomy tests are already like that so it's not worth holding the patch up for until we start cleaning up our tests in general.
Committed/pushed to 8.x.
This is more missing feature than bug, so not sure what webchick will think about backport, but that's up to her :)
Comment #42
xjmMmm, I'd disagree with classifying it as a feature, because I believed the feature existed in D6. :)
Comment #43
catchWell I said the lack of it is a "missing feature", not that adding it back is adding a new one. But it's not a straight bug fix. Several Drupal 6 features were removed from Drupal 7, and in the case of this one it could be done via contrib if it's not already.
Comment #44
Albert Volkman CreditAttribution: Albert Volkman commentedD7 backport.
Comment #45
xjmLooks like there's an extra blank line after the test case, I think. Backport looks fine other than that.
Comment #46
Albert Volkman CreditAttribution: Albert Volkman commentedOopsie.
Comment #47
xjmThanks. :)
Comment #48
webchickCommitted and pushed to 7.x. Thanks!