Problem/Motivation

Currently the Aggregator RSS feed located under aggregator/rss is returning empty feed items

<?xml version="1.0" encoding="utf-8" ?>
<rss version="2.0" xml:base="https://dfm2o.ply.st/aggregator/rss" xmlns:dc="http://purl.org/dc/elements/1.1/" xmlns:content="http://purl.org/rss/1.0/modules/content/" xmlns:foaf="http://xmlns.com/foaf/0.1/" xmlns:og="http://ogp.me/ns#" xmlns:rdfs="http://www.w3.org/2000/01/rdf-schema#" xmlns:schema="http://schema.org/" xmlns:sioc="http://rdfs.org/sioc/ns#" xmlns:sioct="http://rdfs.org/sioc/types#" xmlns:skos="http://www.w3.org/2004/02/skos/core#" xmlns:xsd="http://www.w3.org/2001/XMLSchema#">
  <channel>
    <title>Aggregator RSS feed</title>
    <link>https://dfm2o.ply.st/aggregator/rss</link>
    <description></description>
    <language>en</language>
    
    <item>
      <title></title>
      <link>https://dfm2o.ply.st/</link>
      <description></description>
      <pubDate />
      <dc:creator />
    <guid isPermaLink="true">https://dfm2o.ply.st/</guid>
    </item>
...

How to reproduce

  1. Enable aggregator core module.
  2. Go to aggregator/sources/add to add a feed.
  3. Fill Title with Drupal and URL with https://www.drupal.org/rss.xml and save.
  4. Go to admin/config/services/aggregator and Update items.
  5. Check that now you have some items and open aggregator/rss in a browser.
  6. See the resulting XML source code.

Remaining tasks

  1. Write test
  2. Write patch
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

geertvd created an issue. See original summary.

geertvd’s picture

Status: Active » Needs review
FileSize
2.56 KB
geertvd’s picture

Status: Needs review » Needs work
Issue tags: +VDC, +Needs tests

Still needs test coverage so setting back to needs work.

geertvd’s picture

Only added a test so no need for an interdiff

geertvd’s picture

Status: Needs work » Needs review

The last submitted patch, 4: aggregator_rss_feed-2552037-4-test.patch, failed testing.

geertvd’s picture

Issue tags: -Needs tests
dawehner’s picture

Intersting that we never had proper test coverage for it.

  1. +++ b/core/modules/aggregator/config/optional/views.view.aggregator_rss_feed.yml
    @@ -68,61 +68,24 @@ display:
    +      sorts:
    +        timestamp:
    +          id: timestamp
    

    Agreed the timestamp makes more sense by default

  2. +++ b/core/modules/aggregator/src/Tests/AggregatorRenderingTest.php
    @@ -127,6 +135,17 @@ public function testFeedPage() {
    +    /** @var Item $item */
    

    We use an FQCN here usually

geertvd’s picture

Fixed the feedback from #8

jibran’s picture

Do we need an upgrade path here? if not then it is ready.

dawehner’s picture

Issue tags: +Needs update path

There are some possiblities regarding an update path:

  • We override the existing configuration completely ... this would be a bit sad, as it could break existing adaptions out there.
  • We try to manually update the config entity to fix just the bug.
  • We display some form of message, and explain what people would have to fix for existing sites
geertvd’s picture

Note that this particular view has been broken since beta-12, so that's before there was an upgrade path I think.
In that way it's safe to assume that anyone using this view has probably overwritten it.

Still worth a discussion though.

geertvd’s picture

My preference goes to

We try to manually update the config entity to fix just the bug.

I'd like to hear someone else's opinion on this though.

Status: Needs review » Needs work

The last submitted patch, 9: aggregator_rss_feed-2552037-9.patch, failed testing.

geertvd’s picture

Status: Needs work » Needs review
FileSize
4.54 KB

reroll

dcam’s picture

I tried out #16 and it does restore the feed content, but not all of it. My feed items are still missing content in the item description fields. I haven't figured out why yet, but I'm trying to work on that this morning.

Since the test passed and I can see that it is checking for the item description, I'm going to leave this at Needs Review. It could be that my feeds are just weird. They do have descriptions though. They're visible on the front end at /aggregator.

dcam’s picture

The descriptions were broken due to #2500931-45: Views feed doesn't encode embedded HTML anymore being applied a few days ago. I imagine that if I re-tested #16 it would come back as failing the new assertion where it tries to find the description. I'm going to reply there and let them know about the issue.

dcam’s picture

Status: Needs review » Needs work

Actually, nevermind. See the change record for that other issue at https://www.drupal.org/node/2601028. From now on item descriptions must be provided as render arrays. #16 will have to be updated.

dcam’s picture

#2500931: Views feed doesn't encode embedded HTML anymore created a catch-22. We can't genuinely test that feeds are working properly because descriptions were broken by the other issue, but we can't create a separate issue and therein test that descriptions are working because the whole feeds are broken. I propose that we just fix the description field rendering problem in this issue, even though I believe that they should be separate issues.

To that end, I updated the patch in #16.

dcam’s picture

Status: Needs work » Needs review

Status. Sorry.

dcam’s picture

Priority: Normal » Major

I'm bumping this to major for the following reasons:
1. This issue renders the Aggregator module's RSS feed unusable.
2. If this part of the code had proper tests, it would certainly be failing them right now.

Lendude’s picture

Status: Needs review » Needs work

Patch and fix look good. Manually tested this and fixes the issue.

I agree with @dcam in #20, that it makes sense to do both the description render array fix and the views config fix in one issue.

Back to 'Needs work' for the upgrade path.

dawehner’s picture

+++ b/core/modules/aggregator/src/Plugin/views/row/Rss.php
+++ b/core/modules/aggregator/src/Plugin/views/row/Rss.php
@@ -53,6 +53,11 @@ public function render($row) {

@@ -53,6 +53,11 @@ public function render($row) {
       $item->{$name} = $field->value;
     }

So shouldn't that code here have the same check basically?

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

xjm’s picture

Issue tags: +D8 major triage deferred

When the core committers and views maintainers discussed this issue we weren't all sure it qualified as major. On the one hand, it sounds like the particular feature is pretty broken; on the other hand, it's just a small feature of one module. So deferring triage for later on this issue.

NW for #24 and the upgrade (update) path.

jmsosso’s picture

Issue summary: View changes

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

drupgirl’s picture

Version: 8.6.x-dev » 8.6.7

Hi. This is major imo as utilizing RSS feeds with third party services (like Zapier) are truly the only way currently folks can overcome the limitations of D8 in terms of connection/sharing info with 3rd party APIs.

RSS feeds are a staple feature of Web sites. Not being able to control what is output in the is a major issue minimally. In my case its actually critical.

From the issue queues (aggregator & views) this looks to be a lingering issue and it doesn't appear to be consensus on how to move this forward so that Drupal can offer RSS feeds reliably.

The path 9 in #20 did not apply for me cleanly. I had to manually insert this piece.

+    // Item descriptions must be render arrays.
+    if (isset($item->description) && !is_array($item->description)) {
+      $item->description = ['#markup' => $item->description];
+    }
+

After I do, the rss.xml that is generated shows the description field, but when I go to validate all I see is <description/> without the intended content.

Has anyone solved this?

Version: 8.6.7 » 8.6.x-dev

Core issues are now filed against the dev versions where changes will be made. Document the specific release you are using in your issue comment. More information about choosing a version.

Version: 8.6.x-dev » 8.9.x-dev

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Branches prior to 8.8.x are not supported, and Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

Bug reports should be targeted against the 8.9.x-dev branch from now on, and new development or disruptive changes should be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.2.x-dev

Drupal 8 is end-of-life as of November 17, 2021. There will not be further changes made to Drupal 8. Bugfixes are now made to the 9.3.x and higher branches only. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.2.x-dev » 9.3.x-dev
quietone’s picture

Project: Drupal core » Aggregator
Version: 9.3.x-dev » 1.x-dev
Component: aggregator.module » Code

The aggregator module has been removed from Core in 10.0.x-dev and now lives on as a contrib module. Issues in the Core queue about the aggregator module, like this one, have been moved to the contrib module queue.

larowlan’s picture

Issue tags: +Needs reroll

Needs reroll for contrib

dcam’s picture

Version: 1.x-dev » 2.x-dev
Issue tags: -, -Needs reroll
FileSize
5.44 KB

This is a reroll of #20.

dcam’s picture

FileSize
1.97 KB

Something went wrong and the tests-only patch didn't get uploaded with #40.

dcam’s picture

Status: Needs work » Needs review

The last submitted patch, 40: 2552037-40.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Status: Needs review » Needs work

The last submitted patch, 41: 2552037-40-tests-only.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

dcam’s picture

Status: Needs work » Needs review
FileSize
2.94 KB
6.41 KB

This needed another reroll due to recent changes in AggregatorRenderingTest. Also, the patch's test was written before the conversion from SimpleTest to PHPUnit. So that part of the test had to be updated as a result since Mink doesn't support XPath queries for XML.

The last submitted patch, 45: 2552037-45-test-only.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

dcam’s picture

Status: Needs review » Needs work

NW for the update path

dcam’s picture

Status: Needs work » Needs review
Issue tags: -Needs update path
FileSize
80.27 KB
73.87 KB

I added an update path with a test. I don't believe that we can safely update most sites' view without risking data loss. This isn't like most other view updates that I could find in Core that change something about a plugin setting or some other piece of well-defined configuration. But I wanted to try. The only way I could think of to do that is to compare a hash of the view in active configuration to its default_config_hash. If it's the same, then we're safe to update it. If not, then we display a message to the admin telling them that they need to do it manually (which also applies if they've deleted the view).

To be honest, I expect that the majority of sites will have altered configuration. Even on my sandbox the view somehow got altered, removing what looks like two unused language context keys. I don't know how or when that happened.

I need feedback on the content of the "you have to manually upgrade" message. Should we provide a documentation link? It could possibly be just a link to this issue. The version in the message, "2.0.x", will have to be updated on commit when it's clear which version the change will be released in.

This new update path test uses the exact same dump file as #2552495: Refactor aggregator to use processed_text. When one gets committed, the other will probably have to be rerolled. Also, the hook_update_N() numbers are the same, so they'll have to be changed including the @covers in the associated test.

dcam’s picture

FileSize
11.96 KB

This is a reroll without the fixture, which was committed in #3381799: Add a fixture for database update tests. I'm not going to bother with an interdiff since that was the only change.

larowlan’s picture

Status: Needs review » Reviewed & tested by the community
+++ b/aggregator.install
@@ -11,3 +11,77 @@
+  if (!aggregator_rss_view_config_is_default()) {

We have a pattern in core of using the a reusable class for handling updates to views config (see \Drupal\views\ViewsConfigUpdater)
We then call that code both from an update hook, and from a hook_view_presave.

The later is to handle the case of where an install profile has its own config and it is now invalid.

In this case I don't think this applies.

We're updating default config provided by the aggregator module only, rather than generic config possibly used in multiple places.

E.g. those type of updates are for when the config changes for e.g. a views filter plugin and there is not a known set of views to update.

So I think this approach is fine in this case.

  • dcam committed 620743b3 on 2.x
    Issue #2552037 by dcam, geertvd, dawehner, larowlan, Lendude: Aggregator...

  • dcam committed b87d1e99 on 1.x
    Issue #2552037 by dcam, geertvd, dawehner, larowlan, Lendude: Aggregator...
dcam’s picture

Status: Reviewed & tested by the community » Fixed

Thank you to everyone who worked on this!

Status: Fixed » Closed (fixed)

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