Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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
- Enable aggregator core module.
- Go to aggregator/sources/add to add a feed.
- Fill Title with Drupal and URL with https://www.drupal.org/rss.xml and save.
- Go to admin/config/services/aggregator and Update items.
- Check that now you have some items and open aggregator/rss in a browser.
- See the resulting XML source code.
Remaining tasks
- Write test
- Write patch
Comment | File | Size | Author |
---|---|---|---|
#49 | 2552037-49.patch | 11.96 KB | dcam |
#45 | 2552037-45-test-only.patch | 2.94 KB | dcam |
#2 | aggregator_rss_feed-2552037-2.patch | 2.56 KB | geertvd |
Comments
Comment #2
geertvd CreditAttribution: geertvd at XIO commentedComment #3
geertvd CreditAttribution: geertvd at XIO commentedStill needs test coverage so setting back to needs work.
Comment #4
geertvd CreditAttribution: geertvd at XIO commentedOnly added a test so no need for an interdiff
Comment #5
geertvd CreditAttribution: geertvd at XIO commentedComment #7
geertvd CreditAttribution: geertvd at XIO commentedComment #8
dawehnerIntersting that we never had proper test coverage for it.
Agreed the timestamp makes more sense by default
We use an FQCN here usually
Comment #9
geertvd CreditAttribution: geertvd at XIO commentedFixed the feedback from #8
Comment #10
jibranDo we need an upgrade path here? if not then it is ready.
Comment #11
dawehnerThere are some possiblities regarding an update path:
Comment #12
geertvd CreditAttribution: geertvd at XIO commentedNote 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.
Comment #13
geertvd CreditAttribution: geertvd at XIO commentedMy preference goes to
I'd like to hear someone else's opinion on this though.
Comment #16
geertvd CreditAttribution: geertvd at XIO commentedreroll
Comment #17
dcam CreditAttribution: dcam as a volunteer commentedI 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.
Comment #18
dcam CreditAttribution: dcam as a volunteer commentedThe 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.
Comment #19
dcam CreditAttribution: dcam as a volunteer commentedActually, 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.
Comment #20
dcam CreditAttribution: dcam as a volunteer commented#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.
Comment #21
dcam CreditAttribution: dcam as a volunteer commentedStatus. Sorry.
Comment #22
dcam CreditAttribution: dcam as a volunteer commentedI'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.
Comment #23
LendudePatch 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.
Comment #24
dawehnerSo shouldn't that code here have the same check basically?
Comment #26
xjmWhen 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.
Comment #27
jmsosso CreditAttribution: jmsosso at servinube commentedComment #33
drupgirl CreditAttribution: drupgirl commentedHi. 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.
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?
Comment #38
quietone CreditAttribution: quietone at PreviousNext commentedThe
aggregator
module has been removed from Core in10.0.x-dev
and now lives on as a contrib module. Issues in the Core queue about theaggregator
module, like this one, have been moved to the contrib module queue.Comment #39
larowlanNeeds reroll for contrib
Comment #40
dcam CreditAttribution: dcam as a volunteer commentedThis is a reroll of #20.
Comment #41
dcam CreditAttribution: dcam as a volunteer commentedSomething went wrong and the tests-only patch didn't get uploaded with #40.
Comment #42
dcam CreditAttribution: dcam as a volunteer commentedComment #45
dcam CreditAttribution: dcam as a volunteer commentedThis 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.
Comment #47
dcam CreditAttribution: dcam as a volunteer commentedNW for the update path
Comment #48
dcam CreditAttribution: dcam as a volunteer commentedI 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.
Comment #49
dcam CreditAttribution: dcam as a volunteer commentedThis 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.
Comment #50
larowlanWe 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.
Comment #53
dcam CreditAttribution: dcam as a volunteer commentedThank you to everyone who worked on this!