When language-specific path prefixes are used on 8.0.6 and later (code seems unchanged including 8.2.x-dev) the destination parameter does not include them and they are removed.

Comments

grahl created an issue. See original summary.

grahl’s picture

StatusFileSize
new765 bytes

The attached path includes the pathPrefix, I did not separately test this with a site without language prefixes, assuming that encodePath is fine with null values.

Related issue: https://www.drupal.org/node/2639822

dawehner’s picture

Could we use the Drupal.url method directly for it?

droplet’s picture

Status: Needs review » Needs work
Issue tags: +JavaScript

#3++

Drupal.url(drupalSettings.path.currentPath)

droplet’s picture

reekris’s picture

StatusFileSize
new706 bytes

I'm experiencing the same issues in my current project. Here is a patch that uses Drupal.url which seems to work fine.

reekris’s picture

Status: Needs work » Needs review
dawehner’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

It would be nice to add some javascript test coverage to prove that things are working.

jherencia’s picture

Drupal.url does not encode the path, would it be better to do this?

--- a/core/modules/contextual/js/contextual.js
+++ b/core/modules/contextual/js/contextual.js
@@ -55,7 +55,7 @@
       .prepend(Drupal.theme('contextualTrigger'));

     // Set the destination parameter on each of the contextual links.
-    var destination = 'destination=' + Drupal.encodePath(drupalSettings.path.currentPath);
+    var destination = 'destination=' + Drupal.encodePath(Drupal.url(drupalSettings.path.currentPath));
wim leers’s picture

#10 is right.

gngn’s picture

Status: Needs work » Needs review
StatusFileSize
new724 bytes

I also think #10 is the best way to solve this - it solved my editors' problems.
Patch attached and setting to need-review.

gngn’s picture

Version: 8.0.x-dev » 8.3.x-dev

Aslo added tests and set to drupal 8.3.x-dev (that's what I'm running) - issue was 8.0.x-dev

droplet’s picture

Status: Needs review » Needs work
Issue tags: -Needs tests +Needs JS tests

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

casey’s picture

Status: Needs work » Needs review
StatusFileSize
new1.04 KB

Reroll for 8.4

casey’s picture

Status: Needs review » Needs work
ludo.r’s picture

Patch #12 works for me with Drupal 8.3.x

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

noemi’s picture

StatusFileSize
new1.02 KB
yogeshmpawar’s picture

Status: Needs work » Needs review

Triggering Bots.

Status: Needs review » Needs work

The last submitted patch, 20: contextual_links_do_not-2707879-20.patch, failed testing. View results

wim leers’s picture

Looks okay, but this is causing tests to fail and it's modifying only core/misc/drupal.js, it should also modify core/misc/drupal.es6.js.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

hctom’s picture

Status: Needs work » Needs review
StatusFileSize
new1.28 KB
new1.8 KB
new1.42 KB
new1.02 KB

Here are some new patches that also target the ES6 script files. I provide two different patch versions for 8.5.x and 8.6.x, because in 8.6.x the ES6 file is slightly different to 8.5.x.

The last submitted patch, 25: contextual_links_do_not-8.5-2707879-25.patch, failed testing. View results

hctom’s picture

The last submitted patch, 25: contextual_links_do_not-8.6-2707879-25.patch, failed testing. View results

hctom’s picture

I guess there are some tests that need to be updated for this change ;)

sam152’s picture

Version: 8.6.x-dev » 8.7.x-dev
StatusFileSize
new1.19 KB
new4.55 KB

I hit this today with path prefixes. #25.1 fixes this problem for me and still applies to 8.7.x. Digging into the two failing test cases, I think both of them demonstrate this bug:

$this->assertTrue(strstr($href, '/admin/structure/block/manage/custom/settings-tray?destination=user/2') !== FALSE);

$this->assertContains("/admin/structure/block/manage/$block_id/settings-tray?destination=user/2", $link->getAttribute('href'));

Both fails are because after this patch, the new destination param is subdirectory/user/2 which I believe is correct, given Drupal CI runs under a subdirectory.

Uploading a new test case so we have explicit coverage and fixing the existing tests.

The last submitted patch, 30: 2707879-TEST-ONLY-30.patch, failed testing. View results

fenstrat’s picture

Status: Needs review » Reviewed & tested by the community

This looks good. Manually tested and it does what it says on the box. New test is great.

larowlan’s picture

adding review credits

  • larowlan committed 12c3109 on 8.7.x
    Issue #2707879 by hctom, Sam152, grahl, reekris, casey, gngn, Noemi,...
larowlan’s picture

Version: 8.7.x-dev » 8.6.x-dev
Status: Reviewed & tested by the community » Fixed

Committed 12c3109 and pushed to 8.7.x. Thanks!

c/p as 83e35e57aa and pushed to 8.6.x

  • larowlan committed 83e35e5 on 8.6.x
    Issue #2707879 by hctom, Sam152, grahl, reekris, casey, gngn, Noemi,...

Status: Fixed » Closed (fixed)

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

avpaderno’s picture

Issue tags: -JavaScript, -Needs JS tests +JavaScript, +Needs JavaScript testing