The link alter hook (login_destination_link_alter()) adds the current path to the url for any non-external, non-routed links. For example if you have a link using the base: scheme. It seems as though the altering is only meant to be done on the user.login and user.logout routes but the way the conditional logic is written it will pass through anything that isn't routed or external which actually could be a lot of other things since there can be more unrouted, but internal schemes like base:

Patch incoming!

CommentFileSizeAuthor
#10 only_alter_specific_link_routes-test-3031467-9.patch1.04 KBdabblela
#9 only_alter_specific_link_routes-test-3031467-8.patch1 KBdabblela
#7 only_alter_specific_link_routes-test-3031467-7.patch1021 bytesdabblela
#5 only_alter_specific_link_routes-test-3031467-5.patch822 bytesdabblela
#2 login_destination-only_alter_specific_link_routes-3031467-2.patch806 bytesdrclaw

Comments

drclaw created an issue. See original summary.

drclaw’s picture

drclaw commented
Title: Adds ?current=<current-url> to non-external, non-routed links » Adds the current url to the query string of any internal but unrouted links
Status: Active » Needs review
StatusFileSize
new login_destination-only_alter_specific_link_routes-3031467-2.patch806 bytes

Here's a patch that just applies the current path to the specific user login/logout routes.

Status: Needs review » Needs work

The last submitted patch, 2: login_destination-only_alter_specific_link_routes-3031467-2.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

megachriz’s picture

megachriz
Primary language Dutch
commented
Issue tags: +Needs tests

I'm not entirely understanding this issue, but the issue status was set to "Needs work" by the testbot because there were no automated tests yet at the time this patch was posted.

@drclaw
Maybe you want to give some concrete examples that emphasize the difference between expected behavior and actual behavior?

Sounds like this could also use automated tests. Therefore leaving the status to "Needs work".

dabblela’s picture

dabblela commented
StatusFileSize
new only_alter_specific_link_routes-test-3031467-5.patch822 bytes

Ran into this issue as well. Basically, if you are constructing a '#type' => 'link', but the URL object points to an internal destination that is not a route or an alias of a route, the module will add the "?current=" parameter. For instance, a user entering a path alias for a node into a link field, and then the node/alias getting deleted.

Giving a patch with an example test a shot...

dabblela’s picture

dabblela commented
Status: Needs work » Needs review
dabblela’s picture

dabblela commented
StatusFileSize
new only_alter_specific_link_routes-test-3031467-7.patch1021 bytes

Trying again

Status: Needs review » Needs work

The last submitted patch, 7: only_alter_specific_link_routes-test-3031467-7.patch, failed testing. View results

dabblela’s picture

dabblela commented
StatusFileSize
new only_alter_specific_link_routes-test-3031467-8.patch1 KB
dabblela’s picture

dabblela commented
Status: Needs work » Needs review
StatusFileSize
new only_alter_specific_link_routes-test-3031467-9.patch1.04 KB

Status: Needs review » Needs work

The last submitted patch, 10: only_alter_specific_link_routes-test-3031467-9.patch, failed testing. View results

dabblela’s picture

dabblela commented
Status: Needs work » Needs review

Sorry for the patch spam, the failing test looks correct now.

rsvelko’s picture

rsvelko commented

@dabblela, can you give me a real life example of why do we need that patch? With real life links/urls and example module config.
I can't understand the patch/issue.
I've read the login_destination.module patch and the test case, but I don't get it.

dabblela’s picture

dabblela commented

@rsvelko Not sure how to give a link for an environment which shows the issue, but steps to reproduce:

1. Enable login_destination module (module config does not matter)
2. Add a Link field to a content type
3. Create a node with a link in the Link field with a URL that does not resolve to a route. For the purposes of the test, the link should lead to a 404 (eg, "/asdf12341234123afakelink")
4. Save the node and view it - the link will have the ?current= parameter

rsvelko’s picture

rsvelko commented

I get it now. Thanks!
Commiting.

  • rsvelko committed 1e4de0a on 8.x-2.x authored by dabblela 
    Issue #3031467 by dabblela, drclaw, rsvelko: Adds the current url to the...
  • rsvelko committed a5dad84 on 8.x-2.x authored by drclaw
    Issue #3031467 by dabblela, drclaw, rsvelko: Adds the current url to the...

  • rsvelko committed 101fa87 on 8.x-1.x authored by dabblela 
    Issue #3031467 by dabblela, drclaw, rsvelko: Adds the current url to the...
  • rsvelko committed d9fc287 on 8.x-1.x authored by drclaw
    Issue #3031467 by dabblela, drclaw, rsvelko: Adds the current url to the...
rsvelko’s picture

rsvelko commented
Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

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