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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Damien Tournoud’s picture

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

andypost’s picture

taxonomy still very badly implemented yet

bjaspan’s picture

subscribe

Anonymous’s picture

I probably have some brains for this and #666412: Regression: RSS feed enclosure support lost from core after my criticals.

dropcube’s picture

Subscribing.

mfb’s picture

Any more thoughts on this and #666412: Regression: RSS feed enclosure support lost from core? Is there a contrib module that replaces the features?

mfb’s picture

Status: Active » Needs review
FileSize
1.27 KB

Here's a patch to restore the feature (or else can join RSS enclosure in a little RSS field formatters contrib module).

mfb’s picture

ok here's a contrib module to restore this feature: http://drupal.org/project/rss_field_formatters

quicksketch’s picture

Category: task » support
Priority: Major » Normal
Status: Needs review » Fixed

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

quicksketch’s picture

Category: support » task
Status: Fixed » Needs review

Er, 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?

mfb’s picture

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

mstef’s picture

I'm not seeing any change in the feed after applying patch #7.

EDIT: Seems to be working now. The feed must have been cached.

jessebeach’s picture

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

xjm’s picture

Version: 7.x-dev » 8.x-dev
Status: Needs review » Needs work
Issue tags: +Needs tests, +Novice

This will need to be rerolled against D8 (and for git). Also, we should add test coverage for this.

Tagging novice for the reroll. Thanks!

xjm’s picture

Issue tags: +Needs backport to D7

Missed a tag.

musicnode’s picture

Re-rolled against D8 via git.

xjm’s picture

Status: Needs work » Needs review

Thanks @musicnode!

Sending to the bot. Still need a test.

xjm’s picture

Status: Needs review » Needs work

Alright, 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 and field_ui.test to get ideas. (The test for this patch should be added to taxonomy.module's tests, though).

acouch’s picture

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

jessebeach’s picture

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

acouch’s picture

Status: Needs work » Needs review
FileSize
1.4 KB

Attached is a patch that adds the category by default by using hook_node_view_alter().

mfb’s picture

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

xjm’s picture

Regarding #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. :)

xjm’s picture

Talked to mfb about this a bit on IRC. More thoughts:

  • 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.
  • It's a bit awkward in that there's nothing enforcing only applying RSS field formatters in the RSS view mode, but I don't think there's a backportable fix for that architecturally.
  • Our test coverage should include an access check.
  • I'm not sure the view alter is appropriate for core. The only core implementation anything close to that is rdf_field_attach_view_alter() (which has some relevance here).
acouch’s picture

Are 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?

xjm’s picture

Are 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?

That would probably be the best solution, I think. Just not quite sure how offhand.

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?

Probably not in D7, unfortunately.

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?

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. Since taxonomy.module is providing the functionality (and node.module will soon be optional), it should provide the test cases, I think.

acouch’s picture

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

acouch’s picture

Assigned: Unassigned » acouch

Working on a test.

acouch’s picture

Attached is a test for the RSS feed. Let me know if you have any feedback.

acouch’s picture

Oops. Saw that I mislabeled the new class. Re-submitting.

Status: Needs review » Needs work

The last submitted patch, taxonomy-rss-regression-test-872488-31.patch, failed testing.

acouch’s picture

Assigned: acouch » Unassigned

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

andypost’s picture

acouch’s picture

Status: Needs work » Needs review
no_commit_credit’s picture

FileSize
3.41 KB

Uploading a version of the patch with only the test to expose the failure.

Status: Needs review » Needs work

The last submitted patch, 872488-test-only.patch, failed testing.

xjm’s picture

Excellent, 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:

  1. +++ b/core/modules/taxonomy/taxonomy.testundefined
    @@ -929,6 +929,99 @@ class TaxonomyTermTestCase extends TaxonomyWebTestCase {
    + * Tests for taxonomy term in RSS feed.
    

    Maybe:
    Tests the rendering of term reference fields in RSS feeds.

  2. +++ b/core/modules/taxonomy/taxonomy.testundefined
    @@ -929,6 +929,99 @@ class TaxonomyTermTestCase extends TaxonomyWebTestCase {
    +   * Test that terms added to nodes are displayed in core RSS feed.
    +   *
    +   * Create a node and assert that taxonomy terms appear in rss.xml.
    

    Very minor point, but the verb forms here should be "Tests"/"Creates"/"asserts". (Reference: http://drupal.org/node/1354#functions)

  3. +++ b/core/modules/taxonomy/taxonomy.testundefined
    @@ -929,6 +929,99 @@ class TaxonomyTermTestCase extends TaxonomyWebTestCase {
    +    // Check that the term is displayed when the rss feed is viewed.
    

    Let's capitalize RSS here.

  4. +++ b/core/modules/taxonomy/taxonomy.testundefined
    @@ -929,6 +929,99 @@ class TaxonomyTermTestCase extends TaxonomyWebTestCase {
    +    $this->assertRaw(format_xml_elements(array($test_element)), t('Term is displayed when viewing the rss feed.'));
    

    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, or format_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!

acouch’s picture

Thanks for the review. Attached is the updated patch.

acouch’s picture

Status: Needs work » Needs review

Updating issue status.

xjm’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests

Note for reviewers: The interdiff is backwards. :)

I think this is ready!

catch’s picture

Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

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

xjm’s picture

Mmm, I'd disagree with classifying it as a feature, because I believed the feature existed in D6. :)

catch’s picture

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

Albert Volkman’s picture

Status: Patch (to be ported) » Needs review
FileSize
4.55 KB

D7 backport.

xjm’s picture

Looks like there's an extra blank line after the test case, I think. Backport looks fine other than that.

Albert Volkman’s picture

xjm’s picture

Status: Needs review » Reviewed & tested by the community

Thanks. :)

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 7.x. Thanks!

Status: Fixed » Closed (fixed)
Issue tags: -Novice, -Needs backport to D7

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