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>&lt;p&gt;Algo em Português&lt;/p&gt;
</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>&lt;p&gt;Algo en Español&lt;/p&gt;
</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>&lt;p&gt;Something in English.&lt;/p&gt;
</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>&lt;p&gt;Algo en Español&lt;/p&gt;
</description>
  ... 

Remaining tasks

  1. 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)
  2. Update tests.
  3. Upload patch.
  4. Watch it pass.
  5. Review / RTBC.
  6. Commit.

User interface changes

None.

API changes

None.

Data model changes

None.

Release notes snippet

N/A

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dww created an issue. See original summary.

dww’s picture

Status: Active » Needs review
FileSize
5.08 KB

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

dww’s picture

And here's the minimum change to the view config that still lets us clean up the xpaths.

Not sure what's better. :/

dww’s picture

p.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:

  1. Decide if we should do the minimal possible change, or a full clean export of the view config. (see #2 vs. #3)

Input most welcome.

Thanks!
-Derek

dww’s picture

#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

Lendude’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
2.88 KB

I 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

dww’s picture

Issue summary: View changes

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

+++ b/core/modules/views/tests/modules/views_test_config/test_views/views.view.test_display_feed.yml
@@ -163,6 +210,8 @@ display:
+            operator_limit_selection: false
+            operator_list: {  }

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

dww’s picture

Issue summary: View changes
alexpott’s picture

Status: Reviewed & tested by the community » Fixed

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

  • alexpott committed ee0772b on 9.0.x
    Issue #3092571 by dww, Lendude: Fix RSS test view (and related tests) to...

  • alexpott committed fcc1f0c on 8.9.x
    Issue #3092571 by dww, Lendude: Fix RSS test view (and related tests) to...

  • alexpott committed f034e52 on 8.8.x
    Issue #3092571 by dww, Lendude: Fix RSS test view (and related tests) to...

Status: Fixed » Closed (fixed)

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