Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ignore redirects when warming or invalidating static cache #7687

Closed
wants to merge 5 commits into from

Conversation

marijoo
Copy link
Contributor

@marijoo marijoo commented Mar 11, 2023

Warming the cache via the static:warm command could result in some failing requests if there are redirect-only entries:

Visiting 78 URLs...
[✗] /a/redirect → cURL error 3:  (see https://curl.haxx.se/libcurl/c/libcurl-errors.html) for /a/redirect
…

This PR fixes this issue by using absoluteUrlWithoutRedirect() instead of absoluteUrl() when collecting entry URLs.

@what-the-diff
Copy link

what-the-diff bot commented Mar 11, 2023

  • The absoluteUrl() method was changed to the absoluteUrlWithoutRedirect()

@marijoo
Copy link
Contributor Author

marijoo commented Mar 14, 2023

The problem also appears when invalidating the static cache on entries that are redirects. This will lead to a Server Error right now. That’s why I applied absoluteUrlWithoutRedirect() to DefaultInvalidator, too.

@edalzell
Copy link
Contributor

@marijoo tests are failing, are you able to resolve those?

@marijoo
Copy link
Contributor Author

marijoo commented Mar 14, 2023

Fixed the test case. ✌🏻

@marijoo marijoo changed the title Ignore redirects when warming static cache Ignore redirects when warming or invalidating static cache Mar 14, 2023
@jasonvarga
Copy link
Member

I'm not sure this is the behavior we want.

The redirects that are failing - are they redirecting to an external site?

@marijoo
Copy link
Contributor Author

marijoo commented Mar 23, 2023

Nope, all internal redirects.

I wonder why Statamic would want to receive the http response for redirects? For me it seems reasonable to omit those by using absoluteUrlWithoutRedirect().

@aerni
Copy link
Contributor

aerni commented Apr 24, 2023

I agree with @marijoo. I got a template where I redirect to a 404 in certain scenarios. Right now, I'm getting hundreds of failed jobs because of this 😅

@jasonvarga
Copy link
Member

jasonvarga commented Apr 24, 2023

A 404, fine, but a regular redirect I'm not sure I understand. You'd want the final URL to be warmed too.

@aerni
Copy link
Contributor

aerni commented Apr 24, 2023

I read this PR again, and I don't quite follow the reasoning either. I'd also expect Statamic to warm the redirects. Except 404.

The only scenario I could see where you wouldn't want this behavior is when you redirect to a page, say /login, which is already warmed because it's part of the pages collection. And now you are trying to warm it as many times as you are redirecting to that page. But maybe Statamic is already smart enough to omit subsequent warmings. Not sure.

@jasonvarga
Copy link
Member

I see the issue though.

If an entry is a redirect, $entry->absoluteUrl() would output a non-absolute url. That's what curl error 3 is about. Redirecting to /the-final-url is invalid. It should be http://site.com/the-final-url.

So I'm going to close this PR as it's not the correct approach.

The real issue is that if an entry is a redirect, absoluteUrl() should also return an absolute URL.

title: The Redirect
redirect: /final
$entry->absoluteUrl(); // returns "/final", but should return http://site.com/final

@jasonvarga jasonvarga closed this Apr 24, 2023
@marijoo
Copy link
Contributor Author

marijoo commented Apr 25, 2023

The real issue is that if an entry is a redirect, absoluteUrl() should also return an absolute URL.

You are absolutelyRight(). I think I found the wrong solution to the right problem. Thanks!

@jasonvarga
Copy link
Member

Looks like #7173 is handling this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants