Problem/Motivation
I've been working with core's Views RSS tests recently:
#2673980: RSS view with fields give wrong URL from path field
#3092125: Add test coverage of Views RSS feeds with translated content
The default view used for these RSS tests is here:
core/modules/views/tests/modules/views_test_config/test_views/views.view.test_display_feed.yml
The title field is configured with the "Link to the Content" checkbox enabled, which doesn't make sense for RSS feeds. It results in XML output like so:
<item>
<title><a href="/drupal-8_8/pt-br/node/1" hreflang="pt-br">pt-br</a></title>
<link>http://localhost/drupal-8_8/pt-br/node/1</link>
<description><p>Algo em Português</p>
</description>
<pubDate><a href="/drupal-8_8/pt-br/node/1" hreflang="pt-br">pt-br</a></pubDate>
<dc:creator><a href="/drupal-8_8/pt-br/node/1" hreflang="pt-br">pt-br</a></dc:creator>
<guid isPermaLink="true">http://localhost/drupal-8_8/pt-br/node/1</guid>
</item>
<item>
<title><a href="/drupal-8_8/es/node/1" hreflang="es">es</a></title>
<link>http://localhost/drupal-8_8/es/node/1</link>
<description><p>Algo en Español</p>
</description>
<pubDate><a href="/drupal-8_8/es/node/1" hreflang="es">es</a></pubDate>
<dc:creator><a href="/drupal-8_8/es/node/1" hreflang="es">es</a></dc:creator>
<guid isPermaLink="true">http://localhost/drupal-8_8/es/node/1</guid>
</item>
<item>
<title><a href="/drupal-8_8/node/1" hreflang="en">en</a></title>
<link>http://localhost/drupal-8_8/node/1</link>
<description><p>Something in English.</p>
</description>
<pubDate><a href="/drupal-8_8/node/1" hreflang="en">en</a></pubDate>
<dc:creator><a href="/drupal-8_8/node/1" hreflang="en">en</a></dc:creator>
<guid isPermaLink="true">http://localhost/drupal-8_8/node/1</guid>
</item>
Sadly, some of the tests themselves are using this fact in xpaths to find an item's title:
$this->getSession()->getDriver()->getText('//title/a'));
Proposed resolution
- Change the configuration of the default view.
- Update the xpaths in the tests.
We should see output like this:
<item>
<title>es</title>
<link>http://localhost/drupal-8_8/es/node/1</link>
<description><p>Algo en Español</p>
</description>
...
Remaining tasks
Decide if we should do the minimal possible change, or a full clean export of the view config. (see #2 vs. #3)@Lendude and @dww agree that minimal is better. #3 (or #6 - a re-upload)Update tests.Upload patch.Watch it pass.Review / RTBC.- Commit.
User interface changes
None.
API changes
None.
Data model changes
None.
Release notes snippet
N/A
Comment | File | Size | Author |
---|---|---|---|
#6 | 3092571-3.minimal-changes.patch | 2.88 KB | Lendude |
#3 | 3092571.2_3.interdiff.txt | 2.85 KB | dww |
#3 | 3092571-3.minimal-changes.patch | 2.88 KB | dww |
#2 | 3092571-2.full-export.patch | 5.08 KB | dww |
Comments
Comment #2
dwwHere's what happens if you load this view, unselect the "Link to the Content" checkbox, and re-export. It's a *lot* of changes to the view config, since this view hasn't been re-saved in ages. :/
Updated the xpaths to be more careful and accurate. This passes locally.
Comment #3
dwwAnd here's the minimum change to the view config that still lets us clean up the xpaths.
Not sure what's better. :/
Comment #4
dwwp.s. Note that #3092125: Add test coverage of Views RSS feeds with translated content includes a @todo pointing here. If this patch lands first, cool. But if that patch lands first, this will need work to fix the selector in that test, too, and remove the comments. Didn't seem worth blocking either one on the other, but they're interrelated.
p.p.s. Updated the first remaining task to be this:
Input most welcome.
Thanks!
-Derek
Comment #5
dww#3092125: Add test coverage of Views RSS feeds with translated content is now RTBC. Assuming that lands first, here are re-rolls of the two approaches for after that's committed. All passes locally. Bot can't tell us until #3092125 is in Git.
Cheers,
-Derek
Comment #6
LendudeI think we need to do the minimal change here, this makes it much clearer what we are trying to fix here. Most test views are fairly minimal compared to a full export, this is fine for tests I think, makes it easier to read the yml file when trying to figure out what a test view is supposed to be doing.
Reupping #3 since that is the patch I'm RTBC'ing, I also think we need to lands this before #3092125: Add test coverage of Views RSS feeds with translated content to not introduce new @todo's that we can avoid
Comment #7
dwwGreat, thanks for the review/RTBC! Agree on minimal. Also makes it more backporty, since some of the new keys from full are 8.8.x-only:
These (at least) would break a backport.
We'd also need to backport the parent fix for this to apply to 8.7.x. Maybe not worth the trouble. But, yeah, minimal seems better, anyway. :)
Comment #8
dwwComment #9
alexpottCommitted and pushed ee0772beb9 to 9.0.x and fcc1f0caea to 8.9.x and f034e52179 to 8.8.x. Thanks!
Backported to 8.8.x as a test only fix.