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.

Sign up for GitHub

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

Jump to bottom

Add support for redirects from plone.app.redirector #76

Merged
merged 2 commits into from
Jun 29, 2018
Merged

Add support for redirects from plone.app.redirector #76

merged 2 commits into from
Jun 29, 2018

Conversation

lukasgraf
Copy link
Member

@lukasgraf lukasgraf commented Jun 24, 2018 •
edited
Loading

plone.rest will now handle redirects created by plone.app.redirector pretty much the same way as regular Plone.

If a redirect exists for a given URL, a GET request will be answered with 301, and the new location for the resource is indicated in the Location header.

Any other request method than GET (POST, PATCH, ...) will be answered with 308 Permanent Redirect. This status code instructs the client that it should NOT switch the method, but retry (if desired) the request the the same method at the new location.

Fixes #60

@coveralls
Copy link

coveralls commented Jun 24, 2018 •
edited
Loading

Coverage Status

Coverage decreased (-0.04%) to 97.713% when pulling ac0f0fe on redirects into 0078eda on master.

@lukasgraf lukasgraf force-pushed the redirects branch 3 times, most recently from 7c956b5 to 350abc4 Compare June 25, 2018 11:31
@lukasgraf lukasgraf changed the title WIP: Add support for redirects from plone.app.redirector: Add support for redirects from plone.app.redirector Jun 25, 2018
@lukasgraf lukasgraf force-pushed the redirects branch 4 times, most recently from 765c39e to be1fceb Compare June 25, 2018 15:21
@lukasgraf lukasgraf requested review from buchi, erral and tisto June 25, 2018 17:19
erral
erral approved these changes Jun 26, 2018
View reviewed changes
Copy link
Member

@erral erral left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM

tisto
tisto requested changes Jun 27, 2018
View reviewed changes
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.

As always a very well written, well documented and well-tested pull request! I hesitate to merge right now because I would like to figure out the WSGI/Zpublisher issue first (and maybe merge plone.rest(api) into the core first).

README.rst Outdated
*same* method at the new location.

In practice, both the Python ``requests`` library a well as Postman seem to
honour this behavior be default.
Copy link
Member

Choose a reason for hiding this comment

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

Typo: be -> by

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, updated 👍

src/plone/rest/patches.zcml
@@ -13,4 +13,12 @@
preserveOriginal="true"
/>

<monkey:patch
Copy link
Member

Choose a reason for hiding this comment

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

@lukasgraf Plone 5.2 will run on WSGI. I have no idea how well this works with WSGI. Any idea @davisagli @jensens @pbauer?

Copy link
Member Author

Choose a reason for hiding this comment

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

@tisto The patch isn't necessary any more for newer versions of ZPublisher anyway, because the status codes code moved to the zExceptions module and already include the 308 that we need. The patching function is written in a way where it does nothing in this case.

However, I don't think the patch is even needed for 5.1 anymore - I could verify what exact Plone version is the highest that even needs the patch, and then only patch conditionally using a ZCML condition for the relevant (and confirmed working) Plone versions.

Copy link
Member

Choose a reason for hiding this comment

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

@lukasgraf ok, cool. Will give it a try with buildout.coredev 5.2 tomorrow.

@lukasgraf
Add support for redirects from plone.app.redirector:
de7c431 
plone.rest will now handle redirects created by ``plone.app.redirector``
pretty much the same way as regular Plone.

If a redirect exists for a given URL, a ``GET`` request will be answered with
``301``, and the new location for the resource is indicated in the ``Location``
header.

Any other request method than GET (``POST``, ``PATCH``, ...) will be answered
with ``308 Permanent Redirect``. This status code instructs the client that
it should NOT switch the method, but retry (if desired) the request with the
*same* method at the new location.
@lukasgraf
Patch ZPublisher.HTTPResponse.status_codes in a more defensive way:
ac0f0fe 
The patch is only needed for Plone < 5.2 (or Zope < 4.0a1 respectively),
and we shouldn't attempt to apply it. This change make sure that:

- only attempts to apply the patch for Zope < 4.0a1
- the patch function tries to patch more defensively
- c.monkeypatch ignores a possible missing original
@lukasgraf
Copy link
Member Author

@tisto I updated the patching so it only happens for appropriate versions:

The patch isn't required any more with Zope >= 4.0a1, where the status_codes have been moved to zExceptions >= 3.2 which includes 308.

This will be the case with Plone 5.2, older version do require the patch.

I therefore implemented patching in a more defensive way:

  • only attempt to apply the patch for Zope < 4.0a1
  • the patch function tries to patch more defensively
  • c.monkeypatch will ignore a possible missing original

So WSGI or not, the patch won't be applied for Plone 5.2 anyway.

@tisto tisto merged commit 9c6f6db into master Jun 29, 2018
@tisto tisto deleted the redirects branch June 29, 2018 08:48
ale-rt added a commit to plone/plone.app.redirector that referenced this pull request Jan 8, 2020
@ale-rt
Fix http status of the response
9826b34 
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
@ale-rt ale-rt mentioned this pull request Jan 8, 2020
Merged
ale-rt added a commit to plone/plone.app.redirector that referenced this pull request Feb 14, 2020
@ale-rt
Fix http status of the response
717ca99 
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

Refs #8
ale-rt added a commit to plone/plone.app.redirector that referenced this pull request Feb 14, 2020
@ale-rt
Fix http status of the response
22399a6 
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

Refs #8
mister-roboto pushed a commit to plone/buildout.coredev that referenced this pull request Feb 26, 2020
@jensens
[fc] Repository: plone.app.redirector
ac0aad6 
Branch: refs/heads/master
Date: 2020-02-14T18:57:04+01:00
Author: ale-rt (ale-rt) <alessandro.pisa@gmail.com>
Commit: plone/plone.app.redirector@22399a6

Fix http status of the response

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

Refs #8

Files changed:
A news/8.feature
M plone/app/redirector/browser.py
M plone/app/redirector/tests/test_view.py
Repository: plone.app.redirector

Branch: refs/heads/master
Date: 2020-02-26T23:04:48+01:00
Author: Jens W. Klein (jensens) <jk@kleinundpartner.at>
Commit: plone/plone.app.redirector@2f6da29

Merge pull request #22 from plone/8-http-status

Fix http status of the response

Files changed:
A news/8.feature
M plone/app/redirector/browser.py
M plone/app/redirector/tests/test_view.py
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@erral erral erral approved these changes

@tisto tisto tisto approved these changes

@buchi buchi Awaiting requested review from buchi

Assignees
No one assigned
Labels
24 status: ready 32 needs: review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@lukasgraf @coveralls @tisto @erral