Problem

RSS view with fields instead of content rendering in RSS view mode results in wrong URLs from path field (with pathauto enabled).

Instead of correct URL with http://mysite.domain/content/my-node-title we got http://mysite.domain/my-node-title.

Reproduced on multiple sites with current d.o D8 pathauto module.

  1. Edit 'Frontpage' view
  2. Select 'Feed' display
  3. Switch display mode from content to fields
  4. Add title, path, author, created field
  5. Configure fields in display mode settings. Link & GUID with permaLink = Path field
  6. Check URLs in preview output

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#172 2673980-120_and_159--3092571.8_7_x.patch14.24 KBdww
#172 2673980-120_and_159.8_7_x.fix-only.patch3.36 KBdww
#172 2673980-120_and_159--3092571.8_7_x.TEST-ONLY.patch10.87 KBdww
#159 2673980.155_159.rawdiff.txt4.83 KBdww
#159 2673980-159.fix-and-test.patch12.86 KBdww
#159 2673980-159.test-only.patch10.64 KBdww
#155 2673980-155-interdiff.txt1.95 KBBerdir
#155 2673980-155.patch9.05 KBBerdir
#150 2673980-150-test-only.patch4.85 KBBerdir
#150 2673980-150.patch9.37 KBBerdir
#148 2673980.147_148.interdiff.txt1.39 KBdww
#148 2673980-148.test-and-fix.patch5.17 KBdww
#147 2673980-147.test-only-will-fail.patch3.65 KBdww
#140 2673980-140.rss-feed-across-languages-revert-69e0b9feb.xml_.txt2.03 KBdww
#3 rss-fields-wrong-url-2673980-3.patch1.38 KBStefan Freudenberg
#10 rss_view_with_fields-2673980-10.patch3.08 KBshadcn
#10 interdiff-2673980-3-10.txt3.01 KBshadcn
#12 rss_view_with_fields-2673980-12.patch3.04 KBshadcn
#12 interdiff-2673980-10-12.txt1.51 KBshadcn
#14 Screen Shot 2017-02-19 at 2.48.08 PM.png87.96 KBneeraj.k
#14 Screen Shot 2017-02-19 at 3.28.56 PM.png45.49 KBneeraj.k
#19 rss_view_with_fields-2673980-19.patch3.03 KBpfaocle
#20 interdiff-2673980-19-20.txt864 byteskekkis
#20 rss_view_with_fields-2673980-20.patch3.03 KBkekkis
#23 rss_view_with_fields-2673980-23.patch1.07 KBAdamPS
#27 rss_view_with_fields-2673980-27.patch1.91 KBfroboy
#28 rss_view_with_fields-2673980-28.patch2.5 KBao2
#36 2673980-36-test-only.patch3.44 KBidebr
#36 2673980-36.patch5.92 KBidebr
#36 interdiff-28-36.txt3.44 KBidebr
#44 2673980-44-testonly.patch3.42 KBvalthebald
#44 2673980-44.patch5.89 KBvalthebald
#44 interdiff-2673980-36-44.txt1.35 KBvalthebald
#45 interdiff-2673980-36-45.txt860 bytesvalthebald
#45 2673980-45.patch5.9 KBvalthebald
#49 2673980-49-testonly.patch3.98 KBvalthebald
#49 2673980-49.patch4.25 KBvalthebald
#49 interdiff-2673980-45-49.patch5.37 KBvalthebald
#53 2673980-53-testonly.patch3.99 KBvalthebald
#53 2673980-53.patch4.26 KBvalthebald
#56 interdiff-2673980-53-56.txt1.07 KBvalthebald
#56 2673980-56-testonly.patch4.1 KBvalthebald
#56 2673980-56.patch4.38 KBvalthebald
#59 2673980-59-testonly.patch813 bytesvalthebald
#59 2673980-59.patch4.52 KBvalthebald
#60 2673980-60-testonly.patch4.23 KBvalthebald
#60 2673980-60.patch4.51 KBvalthebald
#64 interdiff-2673980-60.txt727 bytesvalthebald
#69 rss_view_with_fields-2673980-69.patch4.25 KBAdamPS
#69 rss_view_with_fields-2673980-interdiff-60-69.txt2.52 KBAdamPS
#71 rss_view_with_fields-2673980-71.patch4.2 KBAdamPS
#71 rss_view_with_fields-2673980-interdiff-69-71.txt2.41 KBAdamPS
#74 drupal-8.6.x-rss_view_with_fields-2673980-74.patch4.51 KBhswong3i
#74 drupal-8.6.x-rss_view_with_fields-2673980-74-testonly.patch1.69 KBhswong3i
#78 rss_view_with_fields-2673980-78.patch4.41 KBAdamPS
#78 rss_view_with_fields-2673980-interdiff-71-78.txt3.76 KBAdamPS
#80 rss_view_with_fields-2673980-80.patch4.52 KBAdamPS
#80 rss_view_with_fields-2673980-interdiff-71-80.txt3.65 KBAdamPS
#81 rss_view_with_fields-2673980-81.patch4.43 KBAdamPS
#81 rss_view_with_fields-2673980-interdiff-80-81.txt1.1 KBAdamPS
#83 2673980-83.functional-test-only.patch1.05 KBdww
#83 2673980-83.full-test-only.patch2.86 KBdww
#84 2673980-84.full_.patch5.48 KBdww
#84 2673980.81_84.interdiff.txt926 bytesdww
#85 2673980-85.full_.patch5.48 KBdww
#85 2673980.84_85.interdiff.txt823 bytesdww
#87 2673980-87.views-rss-fields-link.functional-test-only.patch6.59 KBdww
#89 rss_view_with_fields-2673980-89.patch5.48 KBAdamPS
#97 rss_view_with_fields-2673980-97.patch6.33 KBAdamPS
#99 rss_view_with_fields-2673980-99.patch6.99 KBAdamPS
#99 rss_view_with_fields-2673980-interdiff-89-99.txt6.37 KBAdamPS
#111 rss_view_with_fields-2673980-111.patch6.56 KBAdamPS
#111 rss_view_with_fields-2673980-interdiff-99-111.txt1.13 KBAdamPS
#115 2673980-115.patch6.61 KBidebr
#138 2673980-138.views_.view_.rss_feed_across_languages.yml.txt15.57 KBdww
#120 2673980-120.patch6.62 KBdww
#138 2673980-138.rss-feed-across-languages.xml_.txt1.98 KBdww
#120 2673980.115_120.interdiff.txt643 bytesdww
#138 2673980-138.subdirectory-rss-feed-across-languages.xml_.txt2.04 KBdww
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

IT-Cru created an issue. See original summary.

dawehner’s picture

Issue tags: +Needs tests

Sounds like a total valid bug. Let's see whether someone has time to fix it.

Stefan Freudenberg’s picture

The reason for this bug is pretty obvious as you can deduct from the patch. Path aliases start with a slash so prepending them with another slash leads to the first path segment being interpreted as the host part when processing it as a URL further down the line. I am not sure whether the attached patch is the way to solve it. It's still lacking tests, but I would like to gauge people's opinion anyways.

Stefan Freudenberg’s picture

Status: Active » Needs review
azinck’s picture

This fixes the problem for me.

ogggg’s picture

#3 patch works for me

gaele’s picture

Version: 8.0.3 » 8.2.x-dev
Priority: Normal » Major

"Render one feature unusable with no workaround."

gaele’s picture

Status: Needs review » Reviewed & tested by the community

Patch still applies.

shadcn’s picture

Status: Reviewed & tested by the community » Needs work

Hmm, patch in #3 fails if we use an absolute url.

shadcn’s picture

Version: 8.2.x-dev » 8.4.x-dev
Status: Needs work » Needs review
FileSize
3.08 KB
3.01 KB

Here's an updated patch that uses a path validator.

Lendude’s picture

Status: Needs review » Needs work

Bit of nitpicking:

  1. +++ b/core/modules/views/src/Plugin/views/row/RssFields.php
    @@ -19,6 +22,43 @@
    +   * Constructs a PluginBase object.
    

    Copy/paste leftover, needs to be updated to the current use case.

  2. +++ b/core/modules/views/src/Plugin/views/row/RssFields.php
    @@ -19,6 +22,43 @@
    +   *  The path validtor.
    

    typo

  3. +++ b/core/modules/views/src/Plugin/views/row/RssFields.php
    @@ -19,6 +22,43 @@
    +  public function __construct(array $configuration, $plugin_id, $plugin_definition, PathValidatorInterface $pathValidator) {
    ...
    +    $this->pathValidator = $pathValidator;
    

    Coding standard is underscore_case for local variables and camel case for properties

Back to needs work, mostly for adding tests.

shadcn’s picture

dawehner’s picture

For me the path validator is an internal service and we should continue to try to use the Url class if possible. Maybe we need some pre-validation though.
Such problems should really addressed by writing a good test coverage first and then evaluate possible solutions.

neeraj.k’s picture

Tested this issue in four different scenario -

  • A. Fresh Drupal install and no contrib modules
  • B. Fresh Drupal install with Pathauto module
  • C. Fresh Drupal install, no contrib modules after applying patch#12
  • D. Fresh Drupal install with Pathauto module after applying patch#12

Below are steps followed and finding for each scenario.

A. Fresh Drupal install and NO contrib modules
  1. Install the latest 8.4.x-dev (Released: Feb 19 2017) using the standard profile.
  2. Added few Article content pages. These articles shows up in home page (Frontpage View)
  3. Go to Frontpage view edit page. Same can be accessed here - http://testsite.com/admin/structure/views/view/frontpage/edit
  4. View edit screen opens Page Display. Click "Feed" Display tab to configure rss feed
  5. Under Field section added following fields (minimum required before configuring Feed format
    • Content: Title
    • Content: Path
    • Content: Body
    • Content: Authored by
    • Content: Authored on
  6. Change Show mode under FORMAT tab to Fields (Default is Content)
  7. Follow the next screen or just click settings if you have made the format change earlier
  8. Configure all the fields, click Apply and save the view
    Feed field configuration
  9. Check Preview

Issue / Result

Notice the URL being printed under three fields have nodeID only

<title><a href="/node/3" hreflang="en">Where can I get some?</a></title>
<link>http://testsite.com/3</link>
<guid isPermaLink="true">http://testsite.com/3</guid>

the url for link and guid tags should have had node in the path like http://testsite.com/node/3

B. Lets test this again with after installing Pathauto module (no patch applied yet)
  • Install Pathauto module. You would need to install dependencies. It Requires: Chaos tools & Token.
  • After installing the module, added Pathauto patten for Article content type.

Result

Now if we check RSS feed preview under view Feed tab, we have links with new url alias generated using pathauto module.

<title><a href="/where-can-i-get-some" hreflang="en">Where can I get some?</a></title>
  <link></link>
<guid isPermaLink="true" />

If we compare this with output when Pathauto was not installed, it has three differences,

  • href value under title has changed to autogenerated path of the node
  • link tag has missing values.
  • guid tag has missing values

So RSS feed have correct URL but title shows "null" value.
Feed having null title

Also checked feed output at https://validator.w3.org/feed/check.cgi . In both the cases got error message -

Sorry

This feed does not validate.

C. Now test after applying the patch#12 (NO pathauto module installed)
Followed pretty much same steps as in 1st scenario and did few additional steps -
  1. Downloaded the patch under folder /core/modules/views/src/Plugin/views/row/
  2. Applied the patch
    patch < rss_view_with_fields-2673980-12.patch
  3. Clear the Drupal cache
  4. Check RSS feed again by updating preview

Bingo! Issue is fixed. Now we have

<title><a href="/node/3" hreflang="en">Where can I get some?</a></title>
<link>http://drupal-84x.dd:8083/node/3</link>
<guid isPermaLink="true">http://drupal-84x.dd:8083/node/3</guid>

Though we have values under link tag as well as title tag, still feed shows "null" as title in Firefox browser.

D. Test the feed output again after applying the patch#12 after installing pathauto module

Followed all the steps under scenario B and applied the patch as mentioned just above.

This is what we get under three tags under Feed tab preview

<title><a href="/where-can-i-get-some" hreflang="en">Where can I get some?</a></title>
<link>http://drupal-84x.dd:8083/where-can-i-get-some</link>
<guid isPermaLink="true">http://drupal-84x.dd:8083/where-can-i-get-some</guid>

This looks ok. But Firefox still showing "null" text as title.

And W3C Validator still giving message

Sorry

This feed does not validate.

neeraj.k’s picture

Effectively this patch does fix the issue but only partially as RSS feed generated cannot be validated yet.

This should must be addressed before being considered as fix. Even Drupal planet feed requires this -

Your feed must pass source code validation, since Drupal's aggregator module is very strict about parsing feed source code.

bkosborne’s picture

I don't understand what this patch has to do with the feed validating or not? If that's an existing problem it should be opened in a separate issue.

beltofte’s picture

#16 Fully agree here. This issue is about the URL being wrong when the Path field is used. It has nothing to do with validation of the RSS output at all.

Some of the issues mentioned in #14 is also related to the fact that the in the RSS feed contains HTML-characters. I assume that Firefox don't like that and therefore show "null" instead of the title. We can of course argue if Drupal should strip this by default in the RSS style plugin or users should configure the field to be outputted as plain text. But that has nothing to do with the original issue.

BartNijs’s picture

I cannot apply the patch (Drupal 8.3.3)

patching file core/modules/views/config/schema/views.row.schema.yml
patching file core/modules/views/src/Plugin/views/field/FieldPluginBase.php
Hunk #1 succeeded at 1154 with fuzz 2 (offset 4 lines).
Hunk #2 FAILED at 1361.
1 out of 2 hunks FAILED -- saving rejects to file core/modules/views/src/Plugin/views/field/FieldPluginBase.php.rej
patching file core/modules/views/src/Plugin/views/row/Fields.php
Hunk #1 FAILED at 41.
Hunk #2 FAILED at 62.
2 out of 2 hunks FAILED -- saving rejects to file core/modules/views/src/Plugin/views/row/Fields.php.rej

pfaocle’s picture

Added re-rolled patch for 8.3.x post php array syntax changes in core and 8.4.x

kekkis’s picture

@pfaocle, you had a couple of indentation errors which this patch fixes.

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

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now 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.

Kgaut’s picture

patch #20 is working for me ! thanks

AdamPS’s picture

Review comments:

  1. PathValidator seems to require a Drupal route and rejects valid paths to files: "/README.txt" or more importantly "/sites/default/files/*" which occurs when using a File field as the link.
  2. If URL validation fails, the code uses the original value from getField without any validation. This seems like a security vulnerability.
  3. Presumably could remove the comment "// @todo Views should expect and store a leading /. "

Comment (1) matches the sentiment in #13. The URL class allows paths to files and it seems that we should use it.

Here is another patch that takes a simpler approach.

  • First problem is adding a slash if there already is one. Add an explicit test.
  • Second problem is that the URL is typically already encoded and Url->toString encodes again. Solution is to decode it first.
AdamPS’s picture

Status: Needs work » Needs review

The last submitted patch, 12: rss_view_with_fields-2673980-12.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 23: rss_view_with_fields-2673980-23.patch, failed testing. View results

froboy’s picture

This patch worked for me, but it also needs to be applied to the GUID field. I've used the same method as in #23 for that. I didn't take a stab at why tests are failing, but this works for me.

To test:

- Apply #23
- In a RSS Feed view, set "GUID Field" to "Content: Path"
- Observe that your content has the correct path for the link field, but not for the GUID field.
- Apply new patch
- Observe the URLs in Link and GUID match.

ao2’s picture

Status: Needs work » Needs review
FileSize
2.5 KB

The patch from #27 should really use the string concatenation operator . and not the addition.

However even if the patches proposed here help, they are not complete, I'd like to point out that the current use of Url::fromUserInput is still problematic when the site base path is different from /, because canonical entity URLs returned by fields formatters (e.g. core/modules/views/src/Plugin/views/field/EntityLink.php) would already contain the base path in them, while Url::fromUserInput expects internal paths.

So how about stripping $base_path instead? This also covers the case of the leading slash.

See the attached patch, I am introducing a new method to avoid some duplication.
I am not sure if the @todo comment still applies tho.

jjchinquist’s picture

I can confirm that patch 28 worked for a website that we are developing and is in testing. Even though the view is not the core view, but a custom view, the other steps to reproduce the problem are all met.

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

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now 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.

Lendude’s picture

I just tested this on 8.5.x using the Entity link field (instead of Path) and it seems to works there.

Since we deprecated and disabled the Path field plugin and moved its functionality to Entity link in #2810097: Allow views to provide the canonical entity URL of all entities, not just nodes., I think this problem is fixed in 8.5.x

code-brighton’s picture

I had this problem with a custom view RSS with feeds. It missed the first part of my file path URL in a link created with field of type "Link to Content" set as "Output the URL as text"

I can confirm that on Core 8.5.1 patch #28 worked for me. Thanks.

isa.bel’s picture

I had the same issue with a custom feed and the view was generating an URL with an incorrect path.
This started to happen after 8.5.1 update.

#28 worked for me.

Thanks.

Anonymous’s picture

#28 saved my day too.

I use a "Link to Content" field with "Output the URL as text" option in my RSS feed view.
I updated to 8.5.1 and the rss feed didn't have the right URL anymore, so this patch fixed it.

Thank you so much, Antonio!

Lendude’s picture

Status: Needs review » Needs work

We still need some automated tests for this.

idebr’s picture

Added assertion to \Drupal\views\Tests\Plugin\DisplayFeedTest

Note: xpath parsing of the link attribute does not return the correct value since the parser assumes the source is html rather than xml. This is being fixed in #2876211: Convert \Drupal\views\Tests\Plugin\StyleOpmlTest and \Drupal\views\Tests\Plugin\DisplayFeedTest to PHPUnit tests. As a result, the assertion now uses assertRaw to bypass the html document parsing.

arthur.baghdasar’s picture

Patch #36 worked for me on drupal 8.5.x

dww’s picture

Status: Needs review » Needs work

Confirming that #36 fixes this major bug. Thanks to everyone who got it this far!

However, it's worrisome that 2673980-36-test-only.patch is passing. ;) Either something else fixed this bug between 8.5.3 and 8.6.x (which seems lightly unlikely), or the test is giving us a false positive. I haven't dug in deeply, but it seems this needs work for more test hardening.

Furthermore, I don't love that the entity_link field is going through all the trouble to jump through D8's cryptic Url handling mess, with a nice option to make the link absolute for you, only to turn around and re-decode the link, strip the absoluteness, just so we can appease the nightmare of Url::fromUserInput() so we can convert back to an absolute link in here. Can't we notice if the link is already a legit absolute URL and use it directly, instead of re-doing all this work?

Thanks,
-Derek

AdamPS’s picture

Can't we notice if the link is already a legit absolute URL and use it directly, instead of re-doing all this work?

@dww Yes I certainly understand your point here. However note that Url::fromUserInput calls Url::fromUri which performs detailed processing to validate and tidy-up the URL and potentially protect against various security vulnerabilities. Can you propose a different approach that still satisfies the same goal? How will you test the URL is legit other than by calling this code? Or another approach: can you see a way to get the field in a raw format prior to encoding and conversion to absolute?

The code as stands is a little awkward, but does as least seem safe to preserve existing behaviour in a wide variety of cases.

diddism’s picture

Noticed this bug also when using a rewritten nid field.
When I rewrite it as /node/{{ nid }} (OR output it as link) the same weird path-behaviour as described above happens.
When I rewrite it as node/{{ nid }} it works fine on D8.5 and even gives me the path alias in the feed (without patch).

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

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now 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.

IT-Cru’s picture

@dww: I have this bug already with 8.5.6. So I think it isn't fixed yet on an other way. Perhaps it is possible to mark this as a blocker for next core releases, because it isn't fixed since over 2 years now and in my eyes this is realy ugly...

AdamPS’s picture

This issue is "needs work" because the test-only patch is passing. The next step is to create a better test that does actually fail without the patch.

There is no way that this can block the next core release because Drupal is not a paid-for product with a team of people whose job it is to fix this sort of bug. If the community want it to be fixed, then we need to get a patch to RTBC at which point the core committers will consider it for commit.

valthebald’s picture

Adjusted the test so it fails without the patch

valthebald’s picture

Thanks to interdiff, fixed formatting of docblock introduced by the last patch

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

The last submitted patch, 44: 2673980-44-testonly.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 45: 2673980-45.patch, failed testing. View results

valthebald’s picture

valthebald’s picture

The last submitted patch, 49: 2673980-49-testonly.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 49: interdiff-2673980-45-49.patch, failed testing. View results

valthebald’s picture

Alright, I've digged into the issue deeper. In the original test, there was only language, and no path aliases, so links always were output as 'node/1', 'node/2' etc.

This output is language-independent, and can be processed by `Url::fromUserInput()`

Instead of adding more modules to `DisplayFeedTest`, I dared moving the logic of sanitizing user input into separate protected methos, and having kernel test instead

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

Status: Needs review » Needs work

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

valthebald’s picture

Fix CS errors

The last submitted patch, 56: 2673980-56-testonly.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 56: 2673980-56.patch, failed testing. View results

valthebald’s picture

Status: Needs work » Needs review
FileSize
813 bytes
4.52 KB

Fixed wrong test namespace that lead to tests failure, moved wrapper plugin to `views_test_formatter` module.

valthebald’s picture

The last submitted patch, 59: 2673980-59-testonly.patch, failed testing. View results

The last submitted patch, 59: 2673980-59.patch, failed testing. View results

The last submitted patch, 60: 2673980-60-testonly.patch, failed testing. View results

valthebald’s picture

FileSize
727 bytes

Here's the difference between test-only and full patches

idebr’s picture

Status: Needs review » Needs work

@valthebald Thanks for taking the time to work this issue! A few remarks:

  1. +++ b/core/modules/views/src/Plugin/views/row/RssFields.php
    @@ -158,9 +156,7 @@ public function render($row) {
    -      // @todo Enforce GUIDs as system-generated rather than user input? See
    -      //   https://www.drupal.org/node/2430589.
    

    This @todo is still valid and should not be removed.

  2. +++ b/core/modules/views/src/Plugin/views/row/RssFields.php
    @@ -207,4 +203,33 @@ public function getField($index, $field_id) {
    +   *   The field value retrieved with RssFields::getField().
    

    This line should document the input parameter; how it is retrieved or called it not very relevant.

    Suggestion: A user entered relative path without a trailing slash.

  3. +++ b/core/modules/views/src/Plugin/views/row/RssFields.php
    @@ -207,4 +203,33 @@ public function getField($index, $field_id) {
    +   * Retrieves a URL from a field value.
    

    This method docblock was probably copy/pasted from ::getFieldUrl() and does not cover the content of the method.

  4. +++ b/core/modules/views/tests/src/Kernel/SanitizeUrlFieldTest.php
    @@ -0,0 +1,28 @@
    +    $result = $displayPlugin->testRssLink('/<front>');
    +    $this->assertTrue(substr($result, 0, 1) === '/', 'Link starts with slash');
    +    $this->assertTrue(substr($result, 0, 2) !== '//', 'Link starts with only one slash');
    

    Let's add an assertion for a node as well, since that is the most obvious use case for an RSS feed.

ahmadhalah’s picture

Patch #60 worked for me Thank you

stacypendell’s picture

Patch #60 worked for us also. Thanks @valthebald! Would love to see this fix move into core someday.

alison’s picture

Seconded, worked perfectly, thank you!

Is there anything I/we can help with to get this patch to "Needs review" (or RTBC)?

AdamPS’s picture

The remaining step is to apply the review comments from #65. There are also some valid comments in #38, which I think we shouldn't tackle in this issue (it is an existing problem and we're not making it worse), but do deserve some explanation in the comments.

Here is a new patch that attempts to cover these.

The previous patch is excellent in many ways, but I did find it necessary to combine sanitizeUrlField into getFieldUrl. They always run together apart from in tests and when it came to writing comments, the division seemed unintuitive and awkward to explain. I propose that instead of altering the code to fit the tests, we should alter the tests to fit the code:-) Hence SanitizeUrlFieldTest.php should be altered to test getFieldUrl.

Sorry, this patch is going to break the tests, as I haven't time to fix them right now. However I hope people will agree that code readability is important so it is the right direction to go. I hope that someone else can take the next step of getting the tests back working again.

Status: Needs review » Needs work

The last submitted patch, 69: rss_view_with_fields-2673980-69.patch, failed testing. View results

AdamPS’s picture

OK I think my thinking is clearer after sleeping, and I have a modified patch.

Step 1 - completed in new patch

The first thing I did was to clarify the problem we are trying to solve:

  • INPUT = string containing a rendered URL
  • OUTPUT = string with same URL converted to absolute

So I have changed the function name to getAbsoluteUrl, and moved the code ->setAbsolute()->toString() inside the function.

The two tests that call sanitizeUrlField will fail as before but we can come back to that once we've got the code finalised. Instead we can have tests that call getAbsoluteUrl with a variety of different forms of URLs and check the response.

Step 2 - not yet done, would like feedback

The second step then is to write code to implement this function. Looking at the comments in #38, I'm not sure that the current code is the best solution. It's complicated and I think still not complete as it fails if the URL is already absolute.

For comparison, look at RssResponseRelativeUrlFilter, which calls Html::transformRootRelativeUrlsToAbsolute, which does a simple string test then if needed a simple string concatenate. How about if we did that here too?

Status: Needs review » Needs work

The last submitted patch, 71: rss_view_with_fields-2673980-71.patch, failed testing. View results

Shawn DeArmond’s picture

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

This is a bug fix, so it should go in 8.6.x.

hswong3i’s picture

Status: Needs work » Needs review
FileSize
4.51 KB
1.69 KB

Just simply renaming and reformat the testonly patch from #60, and reroll via 8.6.x-dev for checking.

No additional changes since #60 so no interdiff.

Status: Needs review » Needs work
AdamPS’s picture

See backport policy

The primary target for bug fixes is the latest development branch of 8.x. Patches will be committed to the development minor dev branch, then cherry-picked backwards

@hswong3i note that patches to Drupal core need to address/solve all comments and concerns of the community. It's not constructive to hide another person's patch without any explanation.

  • Patch #74 goes back to a copy of #60.
  • 4 detailed, specific comments from #65 are not answered
  • Changes suggested in #71 to address those comments are ignored and patch file #71 is hidden
  • 2 wider strategy comments from #71 are not answered
  • There is no explanation/reason given

It's true that #71 is not finished. Yes the tests don't work yet. However I feel that the procedure naming is much clearer, and the code comments have been fixed.

I raised two new points in #71

  1. It looks like the current code fails with absolute URLs
  2. I am concerned that fully decoding and encoding again it's not the best approach, and shown some existing code that could be a better way.
Pierrere’s picture

Until the Patch is finalized and backported to 8.6.x, here is a workaround for anyone who's interested:

One can use the path to the actual node (node/123) via a global textfield. I added the nodeId to the RSS view, as well as Global Textfield and replaced it's output with: node/{{ nid }}

The expected output of "node/123" will be converted to "https://example.com/books/alice_in_wonderland" by the view.

AdamPS’s picture

New patch which hopefully has tests passing and addresses the review comments from #65.

Notes

  • I was worried earlier about absolute paths, but in fact it's OK as the description for link field says "This must be a drupal relative path. "
  • I left the @todo in for "Simplify the inefficient and needlessly complex logic here."
  • I deliberately removed "@todo Views should expect and store a leading / https://www.drupal.org/node/2423913". Although the code does add a leading slash, it is nothing to do with that issue, but rather because the leading slash was removed by removing the base path.

Status: Needs review » Needs work

The last submitted patch, 78: rss_view_with_fields-2673980-78.patch, failed testing. View results

AdamPS’s picture

Sorry it seems that I was wrong to remove "@todo Views should expect and store a leading /"

AdamPS’s picture

Tidy up the @todo markers and I also updated the linked issue

kedramon’s picture

Last patch works for me

dww’s picture

Yes, the fix is still working nicely for me. However, I think the tests could be stronger.

Here's a pair of new test-only patches.

2673980-83.functional-test-only.patch is a trivial change to an existing functional test to assert that the link in an actually rendered views RSS feed matches the absolute URL for the test node.

For reasons I can't explain, trying to run this locally (with or without my change) gives me this:

    There were 4 errors:

    1) Drupal\Tests\views\Functional\Plugin\DisplayFeedTest::testFeedOutput
    Behat\Mink\Exception\UnsupportedDriverActionException: Response headers are
    not available from Drupal\FunctionalJavascriptTests\DrupalSelenium2Driver

which makes absolutely no sense since this is a Functional test class, not FunctionalJavascript. WTF? :( I'm assuming this is some weirdness on my local install, so let's see what the bot thinks of it. I'm hoping it fails, but for the right reason. ;)

2673980-83.full-test-only.patch is the same Functional test change, along with the new Kernel tests from #81 and prior. The fact the Kernel tests fail in test-only is extremely uninteresting, since they simply die calling an undefined function.

I'll upload 2673980-84.full.patch next, which is #81 plus the small functional change.

dww’s picture

This should hopefully be green.

dww’s picture

FileSize
5.48 KB
823 bytes

Noticed an incredibly small nit. We should use @see and no trailing period for that comment.

The last submitted patch, 83: 2673980-83.full-test-only.patch, failed testing. View results

dww’s picture

Well, crap.

a) I missed. ;) This is only a bug if the display is using fields (as the title of this issue clearly says). Thankfully, there's already another test display and a functional test for that, too. Sadly, it was misconfigured and reusing the title field for the link. So this patch is now a lot bigger, but it's mostly to reconfigure that display to add the view_node "field" and tell the RSS display to use that for the link.

b) I fixed my local testing world, but I just can't seem to get this to fail, neither in 8.6.x nor 8.7.x. :/ I definitely see the bug on the local clone of the site I built that made me interested in this bug in the first place. But on the clean core test site, I don't see it. The only difference I can tell is that the full site is a composer build, so the site lives at http://localhost/sw-d8/web whereas the core-only site is a raw git checkout and lives at http://localhost/drupal-8_6 (or 8_7 for my 8.7.x site). On the "sw-d8" site (8.6.7 core), unpatched, my feed contains links like this:

  <link>http://localhost/sw-d8/web/web/2018/11/26/what-migrants-in-the-caravan-want-the-world-to-hear</link>

(note the double web/web).

With patch #81 applied, they correctly look like this:

  <link>http://localhost/sw-d8/web/2018/11/26/what-migrants-in-the-caravan-want-the-world-to-hear</link>

But, if I go to the totally clean drupal-8_7 site using 8.7.x, I can't seem to break the links in the feed, neither with unaliased nodes, nor using path aliases. I thought maybe some other change in 8.7.x branch fixed this, but I have the same "problem" on a clean 8.6.x site.

Before I sink way too much more time into this, I'll upload this other test-only functional patch here. I wish it would fail, but I bet it's going to pass. Perhaps someone else who has time to pick up the torch can build on it to get a "successfully failing" test out of it. ;)

Sorry for the noise / trouble. I *would* love to get this bug officially fixed in core, and I can confirm it *does* fix the problem on the site(s) I actually care about. But apparently there's another thing that has to be true for core to see the bug itself.

AdamPS’s picture

@dww I agree it would be great to have better tests and thanks for trying. I hope someone else might take up the work, but I'm afraid it's not me right now.

Note that we do have basic tests so it's possible that we could defer improving the tests to a separate issue. Therefore I propose we reload the previous patch, looks like #85 and get it into RTBC state. We can then hope to get a comment from a core committer to confirm at least that we are going in the right direction.

AdamPS’s picture

AdamPS’s picture

Status: Needs review » Reviewed & tested by the community

I have reviewed it. NB I'm not cheating to RTBC by own patch - it was written by @dmm in #85.

The last submitted patch, 74: drupal-8.6.x-rss_view_with_fields-2673980-74.patch, failed testing. View results

bwoods’s picture

#77 worked for me as well.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 89: rss_view_with_fields-2673980-89.patch, failed testing. View results

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

alison’s picture

#89 works great for me on 8.6.10!

dvaccaro’s picture

#77 worked on 8.6.13 after a lot of other failed attempts.

AdamPS’s picture

Status: Needs work » Needs review
FileSize
6.33 KB

Continuing from #87 - yes I can confirm that the test is correctly failing when I run it locally too.

Here is a patch with a couple of dummy asserts added so that it definitely will fail and we can see what is different when the test runs on drupal.org.

Status: Needs review » Needs work

The last submitted patch, 97: rss_view_with_fields-2673980-97.patch, failed testing. View results

AdamPS’s picture

OK I think I understand what's going on. The original code calls Url::fromUserInput('//aa/bb/cc') which treats 'aa' as a path which gets discarded and the remainder is prefixed with the base URL.

The result varies depending on how many sub-directories are in the base URL:

  1. In the most common scenario of Drupal running at the root directory of a domain (example.com/), then /node/1 becomes example.com/1 => BUG.
  2. If Drupal is running in a single-level sub-directory (example.com/dir1), then dir1/node/1 becomes example.com/dir1/node/1 and the bug "miraculously" doesn't occur.
  3. With a double-level sub-directory (example.com/dir1/dir12), then dir1/dir2/node/1 becomes example.com/dir1/dir2/dir2/node/1 => BUG again.

I think this explain what @dww found in #87 - the different sites had either single or double sub-directories.

Unfortunately case 2 is what the testbot always seems to do. As far as I can see we can't write any tests that fail unless there is some way to prevent the testbot running in a subdirectory.

However the tests we are adding are still good. They will fail if any future patch causes this scenario to break again unless the future patch somehow manages to break the code in the exact same completely bizarre way that it is broken right now. There I propose that this code is ready to commit.

ivavictoria’s picture

Just dropping in to say that patch #99 works for me on 8.6.13. Thanks! :-)

AdamPS’s picture

@Aurif3x Thanks in that case please set the status to "Reviewed and tested by the Community"

AdamPS’s picture

Status: Needs review » Reviewed & tested by the community

RTBC Based on #100, plus the current patch is very similar to #87 by @dww.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 99: rss_view_with_fields-2673980-99.patch, failed testing. View results

AdamPS’s picture

Status: Needs work » Reviewed & tested by the community

Test fail was just a glitch

temkin’s picture

Tried patch from #99. It works, with the only exception. If "Use absolute link (begins with "http://")" is set, then I'm getting the following error:

InvalidArgumentException: The internal path component 'https://www.example.com/news/press-release/article-title' is external. You are not allowed to specify an external URL together with internal:/. in Drupal\Core\Url::fromInternalUri() (line 410 of /mnt/www/html/whitecase/docroot/core/lib/Drupal/Core/Url.php).

AdamPS’s picture

@temkin Check the description of "Link field":

The field that is going to be used as the RSS item link for each row. This must be a drupal relative path.

isramv’s picture

Patch #99 worked for me, on Drupal 8.6.16, many thanks.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. I've also manually tested this using as base path it looks good. The test fails locally when run without the fix.
  2. +++ b/core/modules/views/src/Plugin/views/row/RssFields.php
    @@ -207,4 +203,31 @@ public function getField($index, $field_id) {
    +    global $base_path;
    

    Let's not use the global like this - there's a method base_path() - it just fetches the global but using that will allow us to remove the global at some point in the future without changing this method.

  3. One concern I have is that we're unnecessarily regenerating URLs. Can the implementation of getAbsoluteUrl() be something like:
    return \Drupal::request()->getSchemeAndHttpHost() . $url_string;
    

    Like is $this->getField($row_index, $this->options['link_field']) - the docs for that are 'The field that is going to be used as the RSS item link for each row. This must be a drupal relative path.' - so it's not user input is it?

AdamPS’s picture

@alexpott thanks for reviewing this issue.

3. One concern I have is that we're unnecessarily regenerating URLs. ... - so it's not user input is it

I agree. At the moment this is covered in a separate issue #2430589: Improve URL conversion in RSS field view .

I would be happy to write a patch that uses \Drupal::request()->getSchemeAndHttpHost(). However I was discouraged by the comment on that method:

Note: The use of this wrapper in particular is especially discouraged....

Please can you confirm that in this scenario it is valid to use this method?

idebr’s picture

AdamPS’s picture

New patch fixes comments from #108

AdamPS’s picture

Please can someone review? - the changes from last time are very simple so it won't take long. This issue seems very close to commit so it would be great if we can get it in.

agentrickard’s picture

Status: Needs review » Reviewed & tested by the community

Works as desired. Code seems fine.

drumm’s picture

Status: Reviewed & tested by the community » Needs work

This patch no longer seems to apply.

idebr’s picture

AdamPS’s picture

Status: Needs review » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

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

AdamPS’s picture

Status: Needs work » Reviewed & tested by the community

Random fail

dww’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

No longer applying to 8.8.x branch. Alas.

dww’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
6.62 KB
643 bytes

Should apply cleanly now.

Also fixed a very minor nit (proper use of @see in a code comment).

Probably back to RTBC, but I'll let someone else mark as such.

AdamPS’s picture

Status: Needs review » Reviewed & tested by the community

  • catch committed 69e0b9f on 8.8.x
    Issue #2673980 by valthebald, AdamPS, dww, idebr, arshadcn, hswong3i,...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed 69e0b9f and pushed to 8.8.x. Thanks!

AdamPS’s picture

Great thanks @catch

gaurav_manerkar’s picture

Status: Fixed » Needs work

Patch doesn't work properly
Not working for translated content having diff URL alias

ex
if my content's source language is english and content have URL alias for ex /content-english
and i have its translated content in Spanish language with URL alias /content-spanish

then both for content it shows /content-english URL.

see image: https://screenshot.net/eny65t0

dww’s picture

Status: Needs work » Fixed

@gauravmanerkar: Please open a separate issue for that bug. It's out of scope here. The use-case you describe clearly never worked in D8. This is already committed and fixed. Calling it 'needs work' now is sort of confusing. Let's leave this 'fixed' and have a clean issue (with a hopefully simple test and patch) to demonstrate and solve the bug you describe.

Thanks!
-Derek

Status: Fixed » Closed (fixed)

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

xjm’s picture

Status: Closed (fixed) » Needs work

Actually @gauravmanerkar was totally right, the bug reported was not out of scope, and the report shouldn't have been dismissed. From discussion with @plach about this issue:

it seems that it’s hardcoding an absolute URL without allowing for outbound processors to kick in, which includes those adding language prefixes or per-language domains, but not only those.

previously it’d be handled as an internal path and rewritten accordingly, if the path was valid

we’re trading a Url object with an absolute URL string, that can’t be good unless the URL is meant to stay untouched

We haven't had consensus on whether to revert or file a followup, but reopening at NW for one or the other.

xjm’s picture

My preference is actually to revert, FWIW. If I'm following, I think we fixed some monolingual sites using pathauto at the expense of breaking all multilingual sites. There were probably more broken monolingual pathauto sites than there are broken multilingual sites now, but still, it's introducing a regression for core itself, using a not-best-practice implementation, and regressing Drupal's commitment to multilingual. I think it's the wrong fix.

AdamPS’s picture

Thanks @xjm. Do you have a proposal for what the right fix would be?

Please can someone describe step by step the scenario that has become broken that worked before? I get the rough idea of it but not with enough clarity to reproduce. In particular I'm not sure how feeds work generating content for the desired language. Is there a different URL for each language?

The current approach came from @alexpott comment #108 item 3

One concern I have is that we're unnecessarily regenerating URLs.

I expect that the previous patch #99 would not have the same problem. I have un-hidden that patch. If you like I can reroll it so that it applies on top of current core. Or you can revert the current patch then I reroll #99 to work in that state.

xjm credited plach.

xjm’s picture

Thanks @AdamPS.

It's been nearly four years since I spent a lot of time working on the D8 URL and routing APIs so things are a bit foggy. :) But a better solution will use the URL generation API rather than bypassing it. Places to start in the API documentation:

Adding credit for @plach also WRT needing a better solution here (I should have done that previously).

AdamPS’s picture

@xjm thanks for your response.

Important request

I think at the moment this discussion is missing a vital element: please can we have a clear step-by-step description of the scenario that has become broken and yet worked before the patch, as I requested in #130.

1) The points in #128 don't take into account that as @alexpott (#108) and @dww (#38) pointed out the committed code contains $this->getField(). This function is required to generate valid (translated) markup suitable for display, so it will presumably call the URL API in order to generate the result. Hence the committed patch doesn't bypass the URL generation API; in fact it avoids unnecessarily calling the API twice.

Patch #99 calls Url::fromUserInput() on the result of getFIeld.

2) The comment #125 does not contain enough information for me to reproduce the problem.

Proposed resolution

If we get a confirmed bug, then I propose that we go back to #99. Because it retains the call to Url::fromUserInput() then I would expect it fixes the bug (and I will confirm this in testing).

It's true that the double URL generation is not ideal, but the D8.7 code also had this same disadvantage and we already have a separate issue to tackle it: #2430589: Improve URL conversion in RSS field view .

AdamPS’s picture

Status: Needs work » Fixed

Resetting this issue to Fixed because a patch has been committed.

The concerns raised were answered in #133. If anyone thinks this patch has introduced a bug/problem that didn't already exist then please provide a clear description of how to reproduce. It might actually be easier to raise a new issue.

catch’s picture

Status: Fixed » Needs work

This still needs discussion about whether to revert the initial patch or not. I do agree with AdamPS that clear steps to reproduce would really help here.

AdamPS’s picture

The patch has been worked on for more than 2 years with over 40 patches, 135 comments and 65 followers.

In #125 one person says the patch doesn't work but doesn't provide enough info to reproduce
In #126 there is a response that says "The use-case you describe clearly never worked in D8."

At least to me it doesn't seem like we really have evidence that this patch has introduced a bug.

dww’s picture

As the author of #126, I should clarify -- I don't believe this feature has ever worked, hence this bug report and years of effort to fix it. However, it's possible that using language prefixes on your URLs somehow masked the bug and a configuration I'm not aware of actually used to work and no longer does. That's definitely possible. I make mistakes all the time and I could absolutely be wrong again here. But we'll only know once we have instructions on how to reproduce this that worked before the patch and are broken by this change. I've re-read #125 at least 3 times, and I see that there are language prefixes in the URLs (both in the comment, and the linked screenshot), but it's not clear to me how to setup a site to have a feed with the same content in multiple languages in such a way that an RSS feed of the content used to give you the right paths and no longer does. I'm about to get on a plane, and maybe on the flight I'll have a chance to setup a clean 8.9.x dev site, enable multiple languages, and try to reproduce. If so, I'll post the results when I land and am online again.

Also, I don't believe this has anything to do with pathauto. It's about sites that live at the root of a given (sub)domain, not those installed in a subdirectory. If your site lives in a (single level deep) subdirectory, it used to work. @see the debugging in #99. So perhaps that's what the multilingual case is really about -- those language prefixes act like a single-level subdir (scenario 2 in #99), and that masked the underlying bug, since we stripped off the language prefix, but then re-added it (?). Maybe?

I still believe that at this point, a new issue about this would be easier for everyone involved. We could have a much simpler time understanding the problem, have a (presumably small) test that demonstrates it, and a (hopefully) small patch to fix it. But I'm not authorized to make those decisions, and defer to @catch, @xjm, et al for direction on how to best proceed.

Thanks/sorry,
-Derek

dww’s picture

I tried and was unable to reproduce what I understand to be the bug reported in #125:

  1. Clean 8.9.x checkout site
  2. Enabled content_translation + language
  3. Added Spanish and Portuguese - Brasil languages to the site.
  4. Created a couple of article nodes.
  5. Translated one of them into all 3 languages (more or less). ;)
  6. Created a view of articles w/ a field-based RSS feed (attached).
  7. The feed contains correct links to each language (see attached XML: 2673980-138.rss-feed-across-languages.xml_.txt).

Then I moved the whole thing into a subdirectory (drupal-8_9):

  1. Cleared caches.
  2. Reloaded the feed.
  3. All the links are still correct (see other attached XML:
    2673980-138.subdirectory-rss-feed-across-languages.xml_.txt).

I'm at a loss as to what the problem is. I believe this issue should be marked fixed. I think if there was a problem from #125, it's from a mis-configured view, not the fault of this code.

Can we call this fixed? Otherwise, someone needs to provide a view that demonstrates the problem.

Thanks!
-Derek

dww’s picture

p.s. I just used @gauravmanerkar's contact tab to send this request:

Hi Gaurav!

Would you be willing to provide more information about the bug you reported at "RSS view with fields give wrong URL from path field"?

https://www.drupal.org/project/drupal/issues/2673980#comment-13265848

We've been trying to understand if/how the code that was committed to core is actually broken. Please see my comment #138:

https://www.drupal.org/project/drupal/issues/2673980#comment-13325706

If you're still having trouble, we'd really appreciate more info about how to reproduce the problem. Especially helpful would be an export of the view that's providing the RSS feed giving you incorrect results.

There's talk of reverting the fix, but no one really knows how to proceed without understanding the bug you reported.

Any help would be tremendously appreciated.

Thanks!
-Derek (dww)

Hopefully that helps. I'm doing all I can to make sure this isn't a regression for multilingual sites, and that we can mark this fixed with a clean conscience.

Cheers,
-Derek

dww’s picture

p.p.s. It's possible that if you don't use the 'Link to content' field that something goes wrong. But if you don't use that field, I'm not sure how you expect to get links to your node translations in each language. That clearly seems like the intended use-case for a field-based RSS feed.

Meanwhile, if I use the "Use absolute link" option, I end up with links like this:
http://localhost/http://localhost/drupal-8_9/pt-br/node/2

So that seems fragile and wrong. However, if I revert commit 69e0b9feb, clear caches, and try again (with 'Use absolute link' enabled) my site spectacularly explodes like so:

[25-Oct-2019 23:57:47 UTC] Uncaught PHP Exception InvalidArgumentException: "The internal path component 'http://localhost/drupal-8_9/pt-br/node/2' is external. You are not allowed to specify an external URL together with internal:/." at /Applications/MAMP-common/htdocs/drupal-8_9/core/lib/Drupal/Core/Url.php line 411

So that much clearly is broken before and after this patch. So, I opened a child issue about it, since it's not caused/improved by this commit:

#3090267: Improve behaviour of RSS view with fields if you enabled "Use absolute link"

Meanwhile, with my site after reverting commit 69e0b9feb, I returned to the working view I attached above, cleared caches, reloaded my feed. Now I get (as anticipated) very broken behavior:

    <item>
  <title>Em Portugûese (tradução)</title>
  <link>http://localhost/drupal-8_9/node/2</link>
...
<item>
  <title>In English (translation)</title>
  <link>http://localhost/drupal-8_9/node/2</link>
...
<item>
  <title>En Español</title>
  <link>http://localhost/drupal-8_9/node/2</link>

(Full feed output attached).

So, as best as I can tell, this was indeed broken before commit 69e0b9feb and working properly now.

I'm out of ideas.

Meanwhile, I'd love input on the right way to fix #3090267: Improve behaviour of RSS view with fields if you enabled "Use absolute link"... ;) See y'all over there.

Thanks,
-Derek

AdamPS’s picture

Thanks @dww

Regarding absolute links, this was raised earlier in the issue, for example #106, but is ruled out by the description of "Link field":

The field that is going to be used as the RSS item link for each row. This must be a drupal relative path.

dww’s picture

@AdamPS Thanks for the pointer! I somehow missed #106. However, I still think #3090267: Improve behaviour of RSS view with fields if you enabled "Use absolute link" is worth addressing. We shouldn't just silently put garbage in your feed (or crash your site if the fix here gets reverted) if you ignore the fine print. We should either prevent you from selecting that option, or remove this restriction and be able to handle absolute URLs. Off topic for here, since all this really is independent from this issue.

Cheers,
-Derek

dww’s picture

It's been 10 days since I asked for clarification on what the problem is that is causing us to consider reverting this. Still no word.

Instead of relying on my manual testing to ease our fears, I figured I should write a test, so I did. ;)

I didn't want to spam everyone in here as the testbot churns on it, so I submitted it as a child issue of this:
#3092125: Add test coverage of Views RSS feeds with translated content

There are 2 patches in there:
Comment #2 has the new test plus reverting this fix. It fails locally, and I assume the testbot will confirm this.
Comment #3 has only the new test. If you run it against the current state of the 8.8.x branch, it passes.

Once the testbot confirms these results, I think we can commit that test, mark this fixed, and move on. Reviews over there are most welcome.

Thanks!
-Derek

dww’s picture

Status: Needs review » Reviewed & tested by the community

Okay, bot is now happy:

#3092125-5: Add test coverage of Views RSS feeds with translated content proves that if we revert this fix, we break RSS feeds including multilingual content.
#3092125-6: Add test coverage of Views RSS feeds with translated content proves that if we keep this fix, RSS feeds with multilingual content get the right links.

I think we should (optionally refine, then) commit that test, and mark this issue fixed.

Bumping back to RTBC to put this on the committer team's radar.

Thanks,
-Derek

Berdir’s picture

Status: Reviewed & tested by the community » Needs review

I did just run into this in our install profile tests, as our RSS feed links were no longer using aliases and my tests failed (yay tests. they are also the only tests to fail on D8.8)

While there might have some edge cases where multilingual prefixes made it work, I don't think that's the core of the issue.

Like other things in views, the URL handling here was still dealing with D7-style path strings, what still kind of exists as \Drupal\Core\Url::getInternalPath(). That's what it expected and is why it has the leading / there. So using a field that actually returns a generated URL that is root-relative and starts with a / resulted in a mess.

There was however a somewhat weird workaround. And that was to get views to generate an internal path without leading /. The easiest way to do so was to use the nid field, and use the rewrite feature to change it to node/{{ nid }}. Ugly, but it worked well, resulting in a valid, always working user-input-path that was then outbound-path-processed, giving you whatever domain/language-prefix/subdirectory that you needed.

That was broken by the committed patch.

But I think it is fairly easy to keep support for those weird-but-previously-working-internal-paths as well as adding support for root-relative paths. We simply check if the first part is a /, then we use the old logic, otherwise we use the new.

There is one thing that we then don't support, and that's the D8-style user input path with a leading / but that is *not* root-relative and is used on link fields, aliases and so on. That is unfortunate, but it's impossible to support both root-relative and drupal-relative /-starting paths. See #2646744: \Drupal\Core\Url does not accept root-relative (file) URLs, making it impossible to let LinkGenerator create root-relative file URL links.

Since this isn't strictly user-input but field-output, I think supporting these two including updating the description in the UI to provide specific examples is an acceptable solution. I'll try to work on that tomorrow.

dww’s picture

Title: RSS view with fields give wrong URL from path field » [PP-2] RSS view with fields give wrong URL from path field
Status: Needs review » Postponed

Yeah, @Berdir and I just chatted about this in Slack. I'm slightly worried that conditionally testing for a leading / is going to be a fragile way to change the logic of RssFields::getAbsoluteUrl() but maybe that's our only hope of trying to let this work for everyone.

That said, before we do *anything* else in here, can we please get the improved test coverage I've added in the child issues committed?

Both are RTBC. That will make it easier to do additional changes in here and ensure we don't further break anything.

We should also add test coverage of the node/{{ nid }} approach explained in #145, but that'll be easier once those child issues are in.

Thanks,
-Derek

dww’s picture

Meanwhile, here's a test that proves we broke @Berdir's work-around. :( Fails locally for me with:

1) Drupal\Tests\views\Functional\Plugin\DisplayFeedTest::testFeedFieldOutput
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'http://localhost/drupal-8_8/node/1'
+'http://localhost/node/1'

It should also fail on the testbot, per #99.

We could consider further robustifying this test so that it sets a path alias on the node it creates.

Should we continue this here or in a new child issue? This issue is getting confusing. :(

dww’s picture

Locally testing this in a single subdirectory, if we revert the fix parts of #120 but leave the new tests from #120 and #147, both the new and old tests pass.

If we try in a root directory or a test site in 2 subdirs, we get the original fails again:

1) Drupal\Tests\views\Functional\Plugin\DisplayFeedTest::testFeedFieldOutput
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'http://localhost/drupal-8_8-sub/web/node/1'
+'http://localhost/drupal-8_8-sub/web/web/node/1'

So we can't just revert #120 or we're back to the original bug. You'll only see all the tests pass if you happen to be in a single subdir.

Here's the proposed fix that @Berdir and I discussed and is explained in #145. It's kinda ugly, but we see no way around it so that this works regardless of how you configured your view.

With this applied, both tests pass when in a 2-level subdirectory, and a single subdir. So I think this is what we ultimately need. What a mess. :/

I'd still love to see the test coverage from the child issues in, too. Locally, DisplayFeedTranslationTest still passes with this patch applied, too.

Berdir’s picture

Thanks @dww, I didn't expect this to be already done when I wake up :)

I've started a test run with that patch, but I expect it to work.

> I'm slightly worried that conditionally testing for a leading / is going to be a fragile way to change the logic of RssFields::getAbsoluteUrl() but maybe that's our only hope of trying to let this work for everyone.
> It's kinda ugly, but we see no way around it so that this works regardless of how you configured your view.

I'm not as worried about this as you are, but I might be impacted by all the mess we went through with URL's in D8 development and the slow, incomplete and painful transition from D7 internal paths to what we have in D8. Specifically, this isn't the first/only place we have a check like that in core, see for example \Drupal\Core\EventSubscriber\RedirectResponseSubscriber::getDestinationAsAbsoluteUrl(). Not quite the same, but has the same is-first-character-a-/ check that treats it as root-relative then.

Berdir’s picture

I've added a description to the link field in the UI to explain this a bit better, might be a little technical but hopefully this will help users to set it up correctly.

I've also added an alias to the functional test, to have that tested as well, then the test with node/ID would also fail on a site without subdirectory because right now it wouldn't.

Additionally, from what I understand, this issue did not actually have a test that failed on testbot, due to the weirdness with subdirectories and protocol-relative parsing. So I also added a kernel test which allows us to test it without a subdirectory, as the domain/base url is just http://localhost there. I'm uploading a fake-test-only patch that disables the check in getAbsoluteUrl() and always uses the old logic.

alexpott’s picture

Status: Postponed » Needs review

Setting the status appropriately.

Status: Needs review » Needs work

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

Berdir’s picture

Status: Needs work » Needs review

Failed as expected, I've uploaded the patches in the wrong order.

Lendude’s picture

Status: Needs review » Needs work

@Berdir, nice!

Mostly just nit picks for the kernel test.

  1. +++ b/core/modules/views/src/Plugin/views/row/RssFields.php
    @@ -54,7 +55,7 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
    +      '#description' => $this->t('The field that is going to be used as the RSS item link for each row. This must either be an internal unprocessed path like "node/123" or a processed, root-relative URL as produced by fields like "Link to content".'),
    

    Yeah a bit technical, but it is correct, so ¯\_(ツ)_/¯

  2. +++ b/core/modules/views/src/Plugin/views/row/RssFields.php
    @@ -212,11 +213,20 @@ public function getField($index, $field_id) {
    +    if (strpos($url_string, '/') === 0) {
    

    Like most URL juggling in Views, not pretty but ¯\_(ツ)_/¯

  3. +++ b/core/modules/views/tests/src/Kernel/Plugin/RssFieldsTest.php
    @@ -0,0 +1,86 @@
    +use Drupal\Tests\Traits\Core\PathAliasTestTrait;
    ...
    +use Drupal\views_test_data\Plugin\views\row\RowTest;
    +use Drupal\views\Plugin\views\row\Fields;
    +use Drupal\views\ResultRow;
    +use Drupal\views_test_data\Plugin\views\style\StyleTest as StyleTestPlugin;
    ...
    +  use PathAliasTestTrait;
    

    These are unused or unneeded.

  4. +++ b/core/modules/views/tests/src/Kernel/Plugin/RssFieldsTest.php
    @@ -0,0 +1,86 @@
    +    $type = $this->createContentType(['type' => 'article']);
    

    Unused variable $type

  5. +++ b/core/modules/views/tests/src/Kernel/Plugin/RssFieldsTest.php
    @@ -0,0 +1,86 @@
    +    // This run use the test row plugin and render with it.
    

    Not really a proper sentence, and looking at the unused use statements probably left over from a previous attempt :) We can probably just take it out, clear enough what it's doing now.

Berdir’s picture

Thanks, yeah, I copied and then tried some things that didn't work like aliases (because kernel tests by default don't resolve aliases and it's not important for this test). Cleaned that up.

Lendude’s picture

Title: [PP-2] RSS view with fields give wrong URL from path field » RSS view with fields give wrong URL from path field
Status: Needs review » Reviewed & tested by the community

Assuming this comes back green, looks great. I'm assuming this also fixes #125 but since we never heard back on that, that is just a guess obviously.

Taking the PP-2 off, since we have a clean use case that the initial patch broke I think getting the fix in should not be postponed.

And huge ++ to @dww for keeping at this one! Awesome work.

alexpott’s picture

I've committed the two additional issues that added test coverage. So let's queue a retest of this one.

dww’s picture

Assigned: Unassigned » dww
Status: Reviewed & tested by the community » Needs work

Nice work, @Berdir, thanks!

Hate to do this, since we're so close to finally putting this to rest. But I found some problems with my "fix" patch.
I also want to improve the tests some more.
I've gotta deal with some other things for the next hour or so, but I'll upload something better soon.

Thanks/sorry,
-Derek

dww’s picture

Among other things, this needed a re-roll now that #3092571: Fix RSS test view (and related tests) to not put links in the title element landed.

This was the thing I was thinking about in the middle of the night when I couldn't sleep:

+++ b/core/modules/views/src/Plugin/views/row/RssFields.php
@@ -212,11 +213,20 @@ public function getField($index, $field_id) {
+    if (strpos($url_string, '/') === 0) {
+      $host = \Drupal::request()->getSchemeAndHttpHost();
+      // @todo Views should expect and store a leading /.
+      // @see https://www.drupal.org/node/2423913
+      return $host . '/' . ltrim($url_string, '/');
+    }

the ltrim() dance here is now pointless. We already tested that $url_string starts with a '/'.

Since #3092125: Add test coverage of Views RSS feeds with translated content is also now in, we can/should improve that test coverage, too. It now fetches the feed twice -- once with raw node paths (node/1 and friends) and once with path aliases. All passes locally.

So here's a new test-only and test-with-fix pair that fixes all of this. Unfortunately, since #155 doesn't apply anymore, interdiff is confused. :( So here's a raw diff relative to #155. Ugly, but better than nothing (which seems to be the theme of this issue). ;)

dww’s picture

Assigned: dww » Unassigned

p.s. Unfortunately, I couldn't get the node/{{ nid }} approach to work for multi-lingual content. :( I have no idea if/how that's supposed to work. I wanted to have core/modules/views/tests/src/Functional/Plugin/DisplayFeedTranslationTest.php do even more permutations to test the node/nid thing like we do in DisplayFeedTest. I was going to make another loop where we do the same reconfig of the display. But that always fails, since by the time we're inside RssFields::getAbsoluteUrl() we have no idea what langcode you want. :/ So if you're re-writing nid, you always end up with the defaultcurrent language (edit: I previously miswrote "default" here), even though we're passing it through Url() like @plach wanted. ;) I still don't think this ever worked. It still doesn't. If you want language prefixing to work, you should be using node_view, not rewriting a nid into node/{{ nid }}. Thankfully, node_view now works (it has since #120 landed) so I believe we now do more to support multilingual feeds than we have up through 8.7.x...

dww’s picture

p.p.s. Since we've been re-using this bloated parent issue, the suggested commit message is wrong. IMHO, the follow-up commit should look something like this:

git commit -m 'Issue #2673980 by dww, Berdir, AdamPS, Lendude, xjm, alexpott, plach: Follow-up fixes for paths in RSS views based on fields.'

More or less. ;)

Thanks,
-Derek

The last submitted patch, 159: 2673980-159.test-only.patch, failed testing. View results

Berdir’s picture

Status: Needs review » Reviewed & tested by the community

We discussed #160, that is expected, that approach can only work with the current default language. The comment initially mentioned "default language", we discussed that, what it does use is the current (type url) language, which is OK and is how it worked so far. The new support for root-relative links does in fact support keeping the row language if content of more than one language is shown.

I did work quite a bit on the patch myself, but that was already RTBC'd in #156, and the changes since then look fine to me. But maybe @Lendude or or someone else still wants to leave a +1 :)

PS: To be able to still have a regular interdiff, I always resolve conflicts first, then stage those changes, then work on additional things, then create the patch + interdiff with git diff HEAD + git diff.

Lendude’s picture

RTBC +1, still looks good

alexpott’s picture

Status: Reviewed & tested by the community » Patch (to be ported)

Committed and pushed 889377448d to 9.0.x and 6b2bc14e54 to 8.9.x. Thanks!

Going to discuss with other cxommitters for a +1 to backport to 8.8.x

I used @dww's commit message.

  • alexpott committed 8893774 on 9.0.x
    Issue #2673980 by dww, Berdir, AdamPS, Lendude, xjm, alexpott, plach:...

  • alexpott committed 6b2bc14 on 8.9.x
    Issue #2673980 by dww, Berdir, AdamPS, Lendude, xjm, alexpott, plach:...

  • alexpott committed c8b260b on 8.8.x
    Issue #2673980 by dww, Berdir, AdamPS, Lendude, xjm, alexpott, plach:...
alexpott’s picture

Status: Patch (to be ported) » Fixed

@catch +1'd the backport.

dww’s picture

Fantastic, thanks everyone! Very happy to finally see this fixed and done. :) Also very glad to see this in 8.8.x branch, since #120 indeed introduced a regression for the folks in the work-around situation like @Berdir in #145.

Also thanks @Lendude for the kind words at the end of #156 -- I missed those earlier. :) Thanks to you for all your prompt reviews of this (and the child issues).

Cheers,
-Derek

alison’s picture

Hi folks, and thank you all so much!!

Is it "allowed" to backport to 8.7.x, or no? If the patch needs a "backwards-reroll" to be able to apply to 8.7.x, I would be happy to do that, I just wasn't sure if it could be committed in, or if we (a D8.7x site affected by this issue) need to stick it out til we update to 8.8.x.

Thanks muchly!

dww’s picture

@alisonjo315 Good question. I really don't know if this is eligible for 8.7.x backport at this point. Given how close we are to releasing 8.8.0 (target 2019-12-04), 8.7.x is about to become security-only backports. I don't know if another "regular bug fix" release is planned. So my guess is no.

But just in case... ;) here's a test-only, fix-only, and test+fix version of #120 + #159 (with a few relevant chunks from #3092571: Fix RSS test view (and related tests) to not put links in the title element to keep it working). I also had to do some more significant changes to the tests since path_alias changed *a lot* between 8.7.x and 8.8.x branches. :/

Running the test-only locally in a raw test site (http://localhost) fails like so:

./vendor/bin/phpunit -v -c core/phpunit.xml core/modules/views/tests/src/Functional/Plugin/DisplayFeedTest.php
...
There was 1 failure:

1) Drupal\Tests\views\Functional\Plugin\DisplayFeedTest::testFeedFieldOutput
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'http://localhost/the-article-alias'
+''

Running it locally in a 2-subdir test site fails like so:

./vendor/bin/phpunit -v -c core/phpunit.xml core/modules/views/tests/src/Functional/Plugin/DisplayFeedTest.php
...
There was 1 failure:

1) Drupal\Tests\views\Functional\Plugin\DisplayFeedTest::testFeedFieldOutput
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'http://localhost/drupal-8_7/web/the-article-alias'
+'http://localhost/drupal-8_7/web/web/the-article-alias'

Passes in both cases with the fix applied. We won't see that on the testbot, which runs in a single subdir (which magically passes, see #99)

The kernel test fails regardless (thanks @Berdir) so hopefully the d.o testbot will agree. ;)

alison’s picture

@dww Thank you so much, that was so cool of you to do, not to mention explaining the test results, etc.

Tbh, I forgot (a) 8.8 target is Dec 4; and, (b) how incredibly soon Dec 4 is (😱). So, even more of a thank you for providing this patch for 8.7 users! (As it happens, the site in question is a monster site for which all the things that could go wrong usually go wrong (of course), so it's quite possible we won't be able to get 8.8 on the site right away, so again, much gratitude :) )

dww’s picture

@xjm Replied in a Slack thread. Pasting here with permission:

There are no more backports to 8.7. 8.8 is coming out in two weeks and serves as the next bugfix release.
...
8.7 has been security-only since the first Wed. of the month :)

So no, this isn't getting committed to 8.7.x. ;)

But composer-patches is your friend if you're still on 8.7.x and want this...

Enjoy,
-Derek

SocialNicheGuru’s picture

I am also getting filter elements if I use this with views when the feed url is given.
The feed effects the output given the state of the filters.
I would like to have the option to include the filter paramaters in the feed url or not

Status: Fixed » Closed (fixed)

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

amarincolas’s picture

I am trying to apply any of the 8.8.x patches but none of them work on 8.8.1.

EDIT: It seems that the code is already in the 8.8.1 version.

alison’s picture

(belated) Thanks so much, @dww (and @xjm)!