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

Should redirection response be temporarily instead of permanent #135

Merged
merged 4 commits into from
Jan 28, 2023

Conversation

mamico
Copy link
Member

@mamico mamico commented Oct 2, 2022

@mister-roboto

This comment was marked as resolved.

@mamico
Copy link
Member Author

mamico commented Oct 2, 2022

@jenkins-plone-org please run jobs

Copy link
Member

@davisagli davisagli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM (assuming the full Plone test jobs are green)

@tisto tisto self-requested a review October 16, 2022 13:46
Copy link
Member

@tisto tisto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's wait for this discussion before we decide if we merge this PR:

plone/plone.app.redirector#8 (comment)

@mamico
Copy link
Member Author

mamico commented Oct 16, 2022

Let's wait for this discussion before we decide if we merge this PR:

plone/plone.app.redirector#8 (comment)

My opinion.
There is a clear distinction between the discussion in plone/plone.app.redirector#22 and plone/plone.app.redirector#8 compared to here. In the other cases we are talking about redirecting web pages, with a possible impact on SEO, here they are called APIs, which I don't think have a direct impact on SEO. On the other hand, the problem is that browsers also cache 301s for an undefined time (https://stackoverflow.com/questions/9130422/how-long-do-browsers-cache-http-301s), so in this case I only see advantages to using temporary redirects instead of permanent ones.

Copy link
Member

@davisagli davisagli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested that this resolves an issue where renaming an item once and then renaming it back to the old name redirects indefinitely in Volto. This change is only for the API, and not server-side rendered pages, so I don't see how it can cause an SEO problem. I think we should go ahead and merge it.

news/135.bugfix Outdated Show resolved Hide resolved
@davisagli
Copy link
Member

@jenkins-plone-org please run jobs

@tisto
Copy link
Member

tisto commented Nov 1, 2022

I'd like to discuss this with @sneridagh and @robgietema to gain a better understanding of how we can support a proper redirect on the frontend for SEO. I guess we would also have to amend the Volto frontend to take the new HTTP response code into account. It could be that we can handle API requests in a different way than regular HTTP requests to the frontend.

@tisto
Copy link
Member

tisto commented Nov 22, 2022

@sneridagh @robgietema can we schedule a meeting today or tomorrow to discuss this?

@davisagli
Copy link
Member

@tisto and I discussed this today.

Currently if volto's server-side rendering does not serve an actual redirect response even if it got a redirect from the API. (The location is updated on the frontend afterward if the content's @id does not match the current path.) We want to change that, and implement a real redirect from the server, for SEO reasons. It should be a permanent redirect for anonymous users, and a temporary redirect for logged-in users. That way search engines get the permanent redirect which is better for SEO, but editors don't run into problems if they rename an item and then rename it back.

This would be easiest if we can put the logic in plone.rest to decide whether to use a permanent or temporary redirect based on whether the user is logged in. Then volto can simply pass along the response status from the API, instead of trying to guess which one is right.

@tisto
Copy link
Member

tisto commented Jan 27, 2023

Discussion notes

Attendees

Notes

  • merging this PR won't make things worse on the SEO side of things
  • Volto currently returns the old content in case there is a redirect
  • this means Google is indexing content twice which is bad for the search ranking
  • changing the permanent redirect to a temporary redirect will solve the problem for the editors and not make things worse for SEO
  • we can and should solve this for crawlers
  • @davisagli idea: differentiate between anonymous users (-> do a permanent redirect) and logged-in users (-> do a temporary redirect)
  • we merge this PR and do a release and then fix this problem for Plone
  • this is important to fix for "Plone the product"

@tisto
Copy link
Member

tisto commented Jan 27, 2023

@marciomazza will try to review/merge/release this today or over the weekend. Sorry, it took me ages to reach a conclusion on this and make a final decision...

@tisto
Copy link
Member

tisto commented May 13, 2024

@davisagli we never created a ticket in plone.rest or anywhere else to tackle returning the correct 302 response code for anonymous users when they access a plone.app.redirector redirect URL, is that correct?

@davisagli
Copy link
Member

@tisto What do you mean? Based on the discussion above I thought the remaining work was to make Volto SSR return a permanent redirect to anonymous users. There's some related discussion in plone/volto#4800

This PR changed plone.rest to always return a 302 redirect for API requests that involve a plone.app.redirector redirect. As discussed above, we can either update plone.rest to make a distinction in the response code based on whether the user is logged in or not -- or we can just handle that distinction in Volto.

@tisto
Copy link
Member

tisto commented May 13, 2024

@davisagli ok, I am confused now. Let's talk about this in a video call. :)

@tisto
Copy link
Member

tisto commented May 13, 2024

Notes:

  • Volto returns 200 OK even if it gets a 302 from the backend
  • Rob attempted to fix this in a client project but it returned the "++api++" URL instead of the regular URL
  • Do it in core, test it in the context of the client project

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