Do not save temporary redirect URLs

MRs are not currently properly for some reason. So I turned MR3 back into a patch.

Sorry, I goofed and uploaded the wrong one.

Looks like we need a test update here

Yeah, its acting like the returned status code isn't 301 or 308. I need to get my debugger operating so that I can inspect the value during the test because I have no idea what that value is and why it's causing the condition to be false. But comment #3 makes it sound like this was a problem with the change from the start.

The test didn't need to be updated. Unfortunately, it was the code that was bugged. The request's status code was always 200 after the redirect happened. The redirect code has to be saved in the on_redirect callback of the request. I did go ahead and expand the test to check all four of the 30x redirect codes mentioned in this issue.

I forgot the issue status.

The last submitted patch, 23: 2987167-23-test-only.patch, failed testing. View results →

After doing my own review I wanted to tighten up the code a bit. This just moves the re-assignment of the feed URL into the on_redirect callback. I can't see any downside to doing it. The only thing that gave me pause was the early return in the case of a 304 response, but I can't see how this change affects it.

I also wanted to move the Url::fromRoute() generation into the data provider, but it broke the test because the container isn't available when the data provider is called.

+++ b/tests/src/Functional/FeedParserTest.php
@@ -96,15 +96,49 @@ class FeedParserTest extends AggregatorTestBase {
+  public function provideRedirectData(): array {

The only comment I have here is that this is a functional test, so we're doing a full Drupal install for each test case - the base class looks to install the testing profile + node + block + views. So we're adding some overhead to the test suite here.

We could inline this into testRedirectFeed with a loop and pay that only once.

Or we could try to convert it to a kernel/unit test.

> Or we could try to convert it to a kernel/unit test.

I want to convert a lot of things to kernel/unit tests. I'm literally working on a foundation for that right now. But this should be easy because it doesn't depend on anything. I'll work on it.

This isn't as easy as I'd hoped. In a kernel test the URLs generated by Url::fromRoute() are incorrect, lacking the base URL of the site. I assume this is because the SIMPLETEST_BASE_URL only applies to functional tests. When the fetcher requests the bad URL it gets a 404 response. I haven't found a way around it yet or any examples of kernel tests that generate a URL and then proceed to request it.

Just a reroll of #26. Still working on changing the functional test to a kernel.

Turning the redirect test into a kernel test turned out to be a huge pain. DefaultFetcher->fetch() creates its own HTTP client instead of using the service. So we can't just replace the service with a mocked client. I considered a lot of options for getting around this limitation. It might be possible, but I don't think we can do it without changing how the fetcher plugins work, like injecting a client, injecting the feed, or something. I don't know what yet, but I'm pretty sure any solution would break backward compatibility. So I think it will have to wait.

So in the end I defaulted to looping through the feed options in the functional test.

Thank you to everyone who worked on this issue!

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