Fix http status of the response

[ale-rt]

The http status of the response is changed from 301 (Moved Permanently)
to 302 (Found) for GET requests and to 307 (Temporary Redirect) for
other request methods because nothing prevents the URL to be reused in
the future.

This was inspired by plone/plone.rest#76

[mister-roboto]

@ale-rt thanks for creating this Pull Request and help improve Plone!

To ensure that these changes do not break other parts of Plone, the Plone test suite matrix needs to pass.

Whenever you feel that the pull request is ready to be tested, either start all jenkins jobs pull requests by yourself, or simply add a comment in this pull request stating:

@jenkins-plone-org please run jobs

With this simple comment all the jobs will be started automatically.

Happy hacking!

[ale-rt]

@lukasgraf as author of plone/plone.rest#76 what do you think about this?

[ale-rt]

@jenkins-plone-org please run jobs

[lukasgraf]

I certainly approve of the "do NOT switch method" aspect for non-GET requests. 👍 💚

Not quite sure about switching from permanent- to temporary-style redirects (see comment). Would love to hear some more opinions.

[lukasgraf]

I'm not sure whether this shouldn't be considered a breaking change (instead of a bugfix). Sure, one could argue that the existing behavior was incorrect, and now gets fixed. But it also does have some potential of breaking client apps in subtle ways.

[lukasgraf]

As for switching the response type for non-GET to ones that indicate that the client should NOT switch methods, I definitely agree, since I already did that in plone.rest as you noticed 😉

As for switching from permanent (301, 308) to temporary (302, 307) types of redirects: I do agree with your rationale - Plone doesn't give any guarantees about permanency of the new URL, so it could be reused for something else the next day. So temporary is semantically correct.

But, a side effect of that is, that according to MDN - 301 Moved Permanently, this might possibly lead to detrimental effects in terms of SEO:

A browser redirects to this page but search engines don't update their links to the resource (in 'SEO-speak', it is said that the 'link-juice' is not sent to the new URL).

So, for private intranets this may not matter, but for large public Plone sites with lots of redirects in place this could be a disadvantageous change. But honestly, I'm just not familiar enough with SEO aspects to judge whether this ("link-juice", page-rank, ...) even is relevant today. Just a point to consider.

[lukasgraf]

A separate question is: How should we proceed with the behavior in plone.rest? We should probably keep it in sync, but that would in my opinion result in a breaking change in plone.rest[api]. Though that only becomes an issue if plone.app.redirector does switch from permanent to temporary. If not, it would just send 301 and 308, and then it would be perfectly in sync with that plone.rest already does.

[datakurre]

We have had this 301 => 302 patch for years, because it is too easy to cause (even temporarily) unnecessary redirects while refactoring/moving content around, and once browser has got wrong 301 response there’s no way to revert it, and it is hard to debug customer problem where customer’s browser has for 301, but there’s already new content at the same address.

[mauritsvanrees]

My two cents: the current possibly wrong permanent redirect is worse than a possibly bad effect on SEO. So I would be in favour of this fix.
One person's bug fix is another person's breaking change. Sit in the middle and call it a new feature. Making a difference between those three categories is often helpful, but sometimes arbitrary...

You may want to fix the plone.app.redirector tests. ;-)

[ale-rt]

About the tests I also have another PR pending which is expected to break if this is merged: plone/Products.CMFPlone#3015
Also some more tests need to be added.

About the breaking/feature/bugfix extension for me it is quite the same, we can even call it breaking so that none will complain :).

I am more interested in keeping consistency with plone.rest and removing the code duplication.

@lukasgraf If I am not wrong ErrorHandling is now inheriting from BrowserView and basically copying some of the methods from the FourOhFourView to modify in a couple of places attempt_redirect.

That function can be stripped down in something like

def attempt_redirect(self):
    new_location = self.get_new_location()
    if not new_location:
        return False
    self.request.response.redirect(url, status=self.get_status(), lock=1)
    return True

One customization is the one I shamelessly copied here (the part about getting the status) and the other should be this one
https://github.com/plone/plone.rest/blob/ac0f0fed167b8eb1664a39badb11a83646199383/src/plone/rest/errors.py#L163-L165
where the new path is computed using custom logic.

Theoretically, moving parts of attempt_redirect to separate methods (one of which might be the get_status one) would make it possible for ErrorHandling to inherit from the FourOhFourView and override just a more smaller method.
The code that set's the status based on the request method will then be shared between the two views and so every decision we make on the status code will be consistent.
👍 less duplicated code
👎 some methods of the FourOhFourView will be part of ErrorHandling (we can make them both inherit from a common base view anyway)

Another possibility is to introduce a new adapter to compute the new_path based on the the request (regular requests will have the one used here, IAPIRequest will have the one defined in plone.rest).
This will make it possible to reuse the attempt_redirect in plone.rest replacing:
self.attempt_redirect() with getMultiAdapter((self.context, self.request), name="plone_redirector_view").attempt_redirect()
👍 less duplicated code
👎 looks an overkill to me and I do not like the prolification of adapters

Anyway before taking care of resolving duplicated code we should resolve end the debate between 302/307 and 301/308.
We could even for the moment keep the 301/308 (because it is what we have now since quite a long time) and plan to change it when we agree on how to avoid code duplication.

Thank you all for your input :)

[lukasgraf]

The points made by @datakurre and @mauritsvanrees make sense and are convincing. Instead of being ambivalent, I now fully approve 😉 👍 (of 302/307, so exactly as the PR stands).

[lukasgraf]

I am more interested in keeping consistency with plone.rest and removing the code duplication.

Agreed. I also won't be sad to see the 308 monkey patch go.

@lukasgraf If I am not wrong ErrorHandling is now inheriting from BrowserView

Correct. The background for that is that it mimics ExceptionView, but for API requests. For an otherwise unhandled exception, the Publisher looks for a named multi-adapter adapting the exception and the request, with the name index.html. So a browser view, except that the context here is the exception.

and basically copying some of the methods from the FourOhFourView to modify in a couple of places attempt_redirect.

Yep. Besides the handling of non-GET requests, plone.rest adds finding redirects for named services (like /front-page/@comments/123456) on top, but that's pretty much it I believe.

Theoretically, moving parts of attempt_redirect to separate methods (one of which might be the get_status one) would make it possible for ErrorHandling to inherit from the FourOhFourView and override just a more smaller method.
The code that set's the status based on the request method will then be shared between the two views and so every decision we make on the status code will be consistent.

👍 less duplicated code
👎 some methods of the FourOhFourView will be part of ErrorHandling (we can make them both inherit from a common base view anyway)

Yeah, I think I'd like that approach the best. Much of the computation logic could be pulled up in a mixin class that could be shared by p.a.redirector, and the FourOhFourView could be reduced to really just handle what's going on on the 404 page, like the "find similar" stuff.

Test coverage for the redirect handling in plone.rest is pretty extensive. So I think if you split the changes in plone.rest in two parts

• Switch to sending 302/307 and update tests
• Refactor to eliminate code duplication

it should be pretty easy to see if it still works.

The only issue with this I see is the dependency from plone.rest on p.a.redirector. Currently, that's a soft dependency. But with this change, this would likely have to become a hard dependency, or at least rather convoluted code to make it conditional. It would also mean that plone.rest would need to depend on a particular version of p.a.redirector, which could get quite tricky.

[ale-rt]

As an MVP I just updated the tests.

[ale-rt]

@jenkins-plone-org please run jobs

[ale-rt]

@jenkins-plone-org please run jobs

[mauritsvanrees]

Approved, and ready for merge.

The title of the PR still has 'WIP' in it, so Work In Progress. To me it seems finished.

@lukasgraf Do you also agree this can be merged now? Or do you want to sync this with a PR in plone.rest?
Once merged, I can make a quick release if needed.

[jensens]

The title of the PR still has 'WIP' in it, so Work In Progress. To me it seems finished.

This reminds me to remind you all to use labels and not WIP in title! Reason: So we are able to filter large list of PRs using Githubs label filtering.
Thanks!

[lukasgraf]

@mauritsvanrees I'm a bit late, but: merging this one works for me 👍