Problem/Motivation

It is not always desired to pass the destination to the redirected route (for instance when using SSO)

Steps to reproduce

Set the redirect path to a custom landing page, the destination is used to redirect users back to the originating page.

Proposed resolution

Add an option to omit the destination in the url query argument

CommentFileSizeAuthor
#3 autologout_optional_destination_3195164_3.patch2.78 KBsaso.sotlar
#2 autologout_optional_destination_3195164_2.patch3.11 KBthierry.beeckmans

Issue fork autologout-3195164

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

thierry.beeckmans created an issue. See original summary.

thierry.beeckmans’s picture

thierry.beeckmans commented
StatusFileSize
new autologout_optional_destination_3195164_2.patch3.11 KB

Added config setting, form element and check in autologout.module

saso.sotlar’s picture

saso.sotlar commented
StatusFileSize
new autologout_optional_destination_3195164_3.patch2.78 KB

Fixed patch so it applies to latest version of module.

gombi’s picture

gombi commented
Status: Active » Needs work

gombi opened merge request !14

gombi’s picture

gombi commented
Status: Needs work » Needs review

I've added the missing schema information for the new setting, which was causing the tests to fail.

ngkoutsaik’s picture

ngkoutsaik commented
Status: Needs review » Reviewed & tested by the community

I have reviewed this ticket and the provided code. The patch works and allows you to ignore the destination parameter if you wish.

I am marking this as tested.

nginex made their first commit to this issue’s fork.

nginex’s picture

nginex commented

I did rebase against the latest module changes in 8.x-1.x, now MR can be applied

DeaOm made their first commit to this issue’s fork.

deaom’s picture

deaom commented
Status: Reviewed & tested by the community » Needs review

Rebased it again and added the update hook that sets the include destination to true. As existing users will not yet have that option and with updated with this change, it will default to false, which we don't want.

admirlju’s picture

admirlju commented
Status: Needs review » Reviewed & tested by the community

The changes work fine. The error here fails because of a core issue #3192893: [META] Serialization issues in Migration tests. Since it works moving to RTBC.

admirlju’s picture

admirlju commented
Status: Reviewed & tested by the community » Needs work

So some other tests are also failing. Checking out what is going on.

admirlju’s picture

admirlju commented
Status: Needs work » Postponed

This works. Before RTBC, I think it would be good to write some tests, but we should wait for #3388601: Use GitLab CI for testing to be fixed and merged, for now postponing.

admirlju’s picture

admirlju commented
Status: Postponed » Needs review

Had some hiccups there with automated tests, now if something changes in the future, we'll make sure not to break this functionality. Since we moved everything to the issue fork and testing is now done with githubCI, hiding the patch files. Needs review.

deaom’s picture

deaom commented
Status: Needs review » Reviewed & tested by the community

The test added is passing, so see no issue with this being merged.

boshtian made their first commit to this issue’s fork.

  • boshtian committed a0af4ee6 on 8.x-1.x authored by gombi 
    Issue #3195164 by admirlju, DeaOm, thierry.beeckmans, gombi, saso.sotlar...
boshtian’s picture

boshtian commented
Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

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