Coming from #2681381: URL Alias is lost when paging forward

Problem/Motivation

When a URL alias is defined for a node/page and the page is accessed through the alias URL, drupalSettings.path.currentPath holds the route's original URL and not the alias one.

Steps to reproduce:

  • create a normal content node: node/1
  • add an URL alias to the node in the 'URL path settings' box: test/test
  • call the URL alias link from the browser: http://[mysite]/test/test
  • watch the page source through the browser, drupalSettings.path.currentPath holds "currentPath":"node\/1" where you would expect "currentPath":"test\/test"

Proposed resolution

tbd

Remaining tasks

tbd

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#23 2687453-23.patch3.16 KBmondrake
#23 interdiff_20-23.txt3.35 KBmondrake
#21 2016-03-18 11_30_03.png7.5 KBmondrake
#20 2687453-20.patch3.24 KBmondrake
#20 interdiff_9-20.txt3.43 KBmondrake
#9 2687453-9.patch2.83 KBmondrake
#9 interdiff_8-9.txt1.89 KBmondrake
#8 2687453-7.patch955 bytesmondrake
#8 interdiff_3-7.txt901 bytesmondrake
#6 2687453-3.patch767 bytesmondrake
#3 2687453-3.patch767 bytesmondrake
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mondrake created an issue. See original summary.

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake commented
Title: drupalSettings.currentPath refers to the original URL even when an URL Alias is defined » drupalSettings.path.currentPath refers to the original URL even when an URL Alias is defined
Issue summary: View changes
mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake commented
Title: drupalSettings.path.currentPath refers to the original URL even when an URL Alias is defined » drupalSettings.path.currentPath refers to the original URL even when a URL alias is defined
Version: 8.0.x-dev » 8.2.x-dev
FileSize
2687453-3.patch767 bytes

Let's see what this would break...

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake commented
Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, 3: 2687453-3.patch, failed testing.

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake commented
Status: Needs work » Needs review
FileSize
2687453-3.patch767 bytes

Reuploading for 8.2.x

Status: Needs review » Needs work

The last submitted patch, 6: 2687453-3.patch, failed testing.

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake commented
Status: Needs work » Needs review
Issue tags: +Needs tests
FileSize
interdiff_3-7.txt901 bytes
2687453-7.patch955 bytes

Should be better. Will need a test with an aliased URL.

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake commented
Issue tags: -Needs tests
FileSize
interdiff_8-9.txt1.89 KB
2687453-9.patch2.83 KB

Added tests in PathAliasTest. Not sure about aliases with non-ascii characters, though :(

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake commented
Title: drupalSettings.path.currentPath refers to the original URL even when a URL alias is defined » drupalSettings.path.currentPath refers to the route's URL even when page is accessed through an URL alias
Issue summary: View changes
dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer commented 
+++ b/core/modules/system/system.module
@@ -690,7 +690,10 @@ function system_js_settings_alter(&$settings, AttachedAssetsInterface $assets) {
-  $current_path = $route_match->getRouteName() ? Url::fromRouteMatch($route_match)->getInternalPath() : '';
+  $current_path = $route_match->getRouteName() ? substr($request->getPathInfo(), 1) : '';
+  if ($pathPrefix && strpos($current_path, $pathPrefix) === 0) {
+    $current_path = substr_replace($current_path, '', 0, strlen($pathPrefix));
+  }

Can't we just use the current path service here?

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

Isn't this a BC break? Isn't this how it used to work in D7?

(Not sure about either, but those are my initial thoughts.)

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake commented

#11

Can't we just use the current path service here?

Well, \Drupal::service('path.current')->getPath() returns the URL to the canonical route and not to URL alias.

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake commented

#12

Isn't this how it used to work in D7?

AFAICS, in D7 there is no currentPath variable in Drupal.settings. Only basePath and pathPrefix.

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake commented

#12

Isn't this a BC break?

I dont' know :( I think it's a matter of any contrib expecting the actual URL that has been called vs. the standard URL for the route.

In the Pagerer module we expect the 'actual' one, so to build pager links on the client side via Drupal.url(drupalSettings.path.currentPath). That's the reason we have the parent issue.

With this fix, the parent issue will (should) be fixed as well.

Alternatively #2154911-6: Lost the Home button when on Admin pages suggests introducing an additional currentPathUrlAlias variable to drupalSettings.path. That could also work for Pagerer but will require a change there.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Assigned: Unassigned » nod_
Issue tags: +JavaScript

Let's get @nod_'s thoughts.

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake commented

BTW, and do not know if related - it seems like nodes accessed via the URL alias do not get their active links (to self) appended with the 'is-active' class.

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ as a volunteer commented
Assigned: nod_ » Unassigned
Status: Needs review » Needs work

Ideally we'd have both the alias and system path exposed in settings.

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake commented
Assigned: Unassigned » mondrake

OK I'll give a try

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake commented
Assigned: mondrake » Unassigned
Status: Needs work » Needs review
FileSize
interdiff_9-20.txt3.43 KB
2687453-20.patch3.24 KB

Here we go.

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake commented
Issue summary: View changes
FileSize
2016-03-18 11_30_03.png7.5 KB

Screenshot of the drupalSettings.path content after applying the patch

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ as a volunteer commented

I'd name it currentAlias, it's already in drupalSettings.path, pretty clear it's not about something else. The rest of the names are redondant because they were ported as is from D7.

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake commented
FileSize
interdiff_20-23.txt3.35 KB
2687453-23.patch3.16 KB

Done

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake commented
Issue tags: +needs backport to 8.0.x

The patch for 8.2.x won't apply to 8.0.x

droplet’s picture

droplet CreditAttribution: droplet commented

Don't you should get it from clientside ? If it needed a helper:

  Drupal.url.requestPath = function () {
    return location.pathname.slice(drupalSettings.path.baseUrl.length);
  };
mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake commented

@droplet

thanks for #25; in fact it turned out the right way to find the current request path was

currentRequestPath = location.pathname.slice(drupalSettings.path.baseUrl.length + drupalSettings.path.pathPrefix.length)

I implemented that for the parent issue in Pagerer contrib module.

Not sure we want to do anything here anymore. I'd be fine with a won't fix.

droplet’s picture

droplet CreditAttribution: droplet commented
Status: Needs review » Closed (won't fix)

Cool. I think we can close it since no usages in CORE at the moment. And Core doesn't maintain any Util helpers in JS side.

Not sure if anyone want to build another http://left-pad.io/ for this little feature :)

Thanks All!