Syndicate block outputs wrong feed URL

I was just to upload a test for this, but I missed the Novice tag, so this is a great exercise for someone who wants to learn about writing tests for Drupal!

For those interesting in how we can add a test for this, there is an existing test at core/modules/node/tests/src/Functional/NodeSyndicateBlockTest.php.

Within that test we have a

testSyndicateBlock

method.

which already adds the block and asserts that it exists, so the things left do is:

1. Click on the link in the block, that would be "Subscribe to"
2. And then assert that the URL is correct

At this point we would have a failing test without the patch from #5 applied. Apply that patch and run the test again. It should pass. Now upload only the test as a patch, and then the test with the patch from #5 so we prove that the test fixes the issue in the first place.

Ping @johndevman on Slack, if you need some assistance :)

Needs work for test by the way!

Added the test for checking Subscribe to link and that the url is correct.

Patch #5 with test #8

Patches goes to 8.8 first

I have modified the Url to base:uri instead of internal:uri to include /subdirectory/ in case it exists in the url configuration.

Version update

Found that the /subdirectory/ problem is solved when assertUrl receives Url object.

This block is really odd. I think we need a follow-up to address the fact that the rss.xml comes from a view so this block only works when views and node is installed and the views.view.frontpage exists. I think the block should be provided by the view and we should deprecate SyndicateBlock. Or we need to do the work to provide an fallback for rss.xml in the Node module.

That said this fix is a valid fix for the problem at hand.

1. +++ b/core/modules/node/tests/src/Functional/NodeSyndicateBlockTest.php
   @@ -34,4 +36,20 @@ public function testSyndicateBlock() {
   +  /**
   +   * 1. Click on the "Subscribe to" link
   +   * 2. Assert that the Subscribe to url is correct
   +   */
   

   This should be something like Tests the syndicate block RSS link.

2. +++ b/core/modules/node/tests/src/Functional/NodeSyndicateBlockTest.php
   @@ -34,4 +36,20 @@ public function testSyndicateBlock() {
   +    $this->drupalGet('');
   

   I think rather then requesting '' we should be explicit about requesting a page like user. Yes it is the same result BUT this bug is about the syndicate block not working on pages other than the frontpage.

3. +++ b/core/modules/node/tests/src/Functional/NodeSyndicateBlockTest.php
   @@ -34,4 +36,20 @@ public function testSyndicateBlock() {
   +    $link = Url::fromUri('internal:/rss.xml');
   +    $this->assertUrl($link);
   

   I think this can be $this->assertSession()->addressEquals('rss.xml'); - I don't think we need to generate a Url object for this and we can use the non legacy methods.

@Chi nope we shouldn't be using t() in tests - nice spot.

Attaching a new patch (and an interdiff) with the changes that @alexpott suggested, plus fixing some CS.

I also agree that it makes more sense for this block to by provided by views.view.frontpage.

Sorry, wrong paths in the patches. Trying again.

+++ b/core/modules/node/tests/src/Functional/NodeSyndicateBlockTest.php
@@ -34,4 +37,18 @@ public function testSyndicateBlock() {
+    $this->assertSession()->addressEquals('rss.xml');

not sure it will work for CI tests - they running in subdir for that purpose - "checkout/" iirc

Is there a way to check if it works on CI tests?

But it works)

Rerolled patch #24 for 9.1.x and also addressed comment from #31, please review.

Point 2 from #31 was not addressed therefore fixed the same from #32

Just come across this due to adding the block as per the new Olivero theme beta, which uses the block.

Probably worth noting that "Given that the block was broken for many years I think no one uses it." in #19 won't be true for long!

Okay, I'm testing the patch by actually running it on https://rachelnorfolk.me and it works really well - no matter the current path, now the main site RSS feed in the syndication block remains correct. It works!

The test seem reasonable and they pass.

I see no code issues reported in the code related to this change.

I say RTBC.

Reviewing RTBC issues as part of Bug Smash Initiative.

This looks good, just a few things to tidy up.

I read the issue summary and it explains the problem but there is no solution listed. Adding tag for IS update.

On reading the issue there is a request for a followup in #18 that sill needs to be done. The other changes to the patch have all been made. I didn't spot any other unfinished work. Yay!

There is a 'needs reroll' tag on the issue but that seems to have been done. Please remember to update tags as work is completed.

Next I looked at the patch and found a line > 80 chars.

+++ b/core/modules/node/tests/src/Functional/NodeSyndicateBlockTest.php
@@ -39,4 +39,17 @@ public function testSyndicateBlock() {
+    // Place the "Syndicate" block and confirm that the user is redirected to the subsciption page.

Line > 80 characters.

Fix #36 and optimize test suite, updated IS, filed follow-up #3174990: Test that SyndicateBlock works when views disabled

+++ b/core/modules/node/tests/src/Functional/NodeSyndicateBlockTest.php
@@ -39,4 +39,17 @@ public function testSyndicateBlock() {
     $this->assertFieldByXPath('//div[@id="block-test-syndicate-block"]/*', NULL, 'Syndicate block found.');
...
+    $this->clickLink('Subscribe to');
...
+    $this->assertSession()->addressEquals('rss.xml');

Any reason for separate assertion? it just one more install and slowdown of whole suite

Hello, the previous patch failed to apply to 9.2.x-dev. See below,

I've re-rolled the patch in #37

Please review.

Patch still applies in 9.3.x, and works as intended.

Before patch I was able to reproduce the error:

After patch, the error is gone. Tests are more efficient now and test-only patch fails. So, I'm moving to RTBC.

Updating issue credits

Committed acb621d and pushed to 9.3.x. Thanks!