FieldPluginBase::renderAsLink() loses language prefix for tokenized paths

Happened to have this file open. What about something like this?

See if the path is routable with PathValidator, otherwise use it as-is? This is kinda for discussion, as I can't 100% remember which of those variables has the path w/o the scheme.

This patch fixes the problem described above when I reproduced the situation in the summary (import .views.view.test_dropbutton.yml, and enable Language).

However, we need to add coverage to FieldPluginBaseTest to cover this situation.

We may want to wait for #2426447: Views no longer supports {{ something }} as twig placeholder for a path, only {{something}} to get in before working on the tests, as that issue adds coverage for tokenized paths, which we can then build off of.

This test kinda feels like cheating, but it passes locally for me.

Minor cleanup to fix some coding standard things.

Some advanced trickery going on here! :D

1. +++ b/core/modules/views/src/Plugin/views/field/FieldPluginBase.php
   @@ -1468,8 +1476,19 @@ protected function renderAsLink($alter, $text, $tokens) {
   +    // otherise use is as-is.
   

   s/otherise/otherwise/
   s/is/it/
   :)

2. +++ b/core/modules/views/src/Plugin/views/field/FieldPluginBase.php
   @@ -1468,8 +1476,19 @@ protected function renderAsLink($alter, $text, $tokens) {
   +    if ($i = strpos($path, 'base:') === 0) {
   

   Wow — we're being brave — relying on PHP's operator precedence :D

3. +++ b/core/modules/views/src/Plugin/views/field/FieldPluginBase.php
   @@ -1468,8 +1476,19 @@ protected function renderAsLink($alter, $text, $tokens) {
   +      if (!$final_url = $this->getPathValidator()->getUrlIfValidWithoutAccessCheck(substr($url['path'], 5))) {
   +        $final_url = CoreUrl::fromUri($path, $options);
   +      }
   +      else {
   +        $final_url->setOptions($options);
   +      }
   +    }
   +    else {
   +      $final_url = CoreUrl::fromUri($path, $options);
   

   Wow!

   I think this can use some additional comments. At least speaking for myself, this looks too confusing/black magic-ish.

   (I kind of understand why it's necessary given the IS, but future readers of this code will say "lolwut" in unison I think :D)

Thanks for the review Wim.

#1 taken care of by the rewrite in #3.

I can assure you that #2 was by accident. I don't have the operator precedence chart memorized :) Added parens to make it more readable.

Any sufficiently advanced technology is indistinguishable from magic. -- Clarke

Added big comment block to account for #3.

Also attached screenshot showing a view-source of the test-dropbutton view that shows the language prefix on the custom text link, and that it matches the path prefix on all the other links in that row.

I can assure you that #2 was by accident. I don't have the operator precedence chart memorized :)

:D

1. +++ b/core/modules/views/src/Plugin/views/field/FieldPluginBase.php
   @@ -1476,9 +1476,15 @@ protected function renderAsLink($alter, $text, $tokens) {
   +    // current Url object may be unroutable and use the 'base:' scheme,
   

   Why "may"? Isn't this *always* the case in case of a token replacement?

2. +++ b/core/modules/views/src/Plugin/views/field/FieldPluginBase.php
   @@ -1476,9 +1476,15 @@ protected function renderAsLink($alter, $text, $tokens) {
   +    // extract the path portion and run it throught the path validator service
   

   s/portion/component/

   But this is not actually extracting the path component, it also includes querystring and fragment, if any.

   So URI reference is probably more accurate.

   OTOH, maybe just parse_url() the path component only? That'd be clearer.

#9-1, no. views.view.files.yml has a path defined for a page display with a Twig token in it (that is the view path that started the chain reaction of Url() related issues). Right before token replacement, the URI string in FieldPluginBase::renderAsLink is 'route:view.files.page_2;arg_0={{fid}}', and ends up being treated as a routable URI through the $final_url logic (I double checked this with debug).

#9-2. Took another stab at that comment, and changed the logic to use parse_url(); that will be less fragile.

+++ b/core/modules/views/src/Plugin/views/field/FieldPluginBase.php
@@ -1468,8 +1476,25 @@ protected function renderAsLink($alter, $text, $tokens) {
+      }
+      else {
+        $final_url->setOptions($options);
+      }
+    }
+    else {
...
+    }

The following is simpler and should do the same:

$final_url = FALSE;

if (parse_url($path, PHP_URL_SCHEME) === 'base') {
  if ($final_url = $this->getPathValidator()->getUrlIfValidWithoutAccessCheck(parse_url($path, PHP_URL_PATH))) {
    $final_url->setOptions($options);
  }
}

// If it was not yet set, get it fromUri
if (!$final_url) {
  $final_url = CoreUrl::fromUri($path, $options);
}

All that said, views should not hide this and 'cheat'.

While still strongly discouraged, there seems to be a need for a generic helper function to turn a path - potentially from base: to a full path - even in core.

Display Suite has the same problem. (with the exact same use case of using tokens)

And panels will probably, too.

We surely don't want to force all of them to find and re-implement this magic code.

Thanks for the review @Fabianx; that will likely read better. I'll double check it when I can locate my development laptop...

`Url::fromUserInput()` will take a path and run the pathValidator on it to get a routable Url object if possible via `Url::fromUserPathUri()` (see #2426181: Add a Url::fromUserInput() wrapper method for generating URLs from user-entered paths). The difference here is that as part of #2418139: Add a toUriString method to Url class and add a route: scheme and #2404603: Add proper support for Url objects in FieldPluginBase::renderAsLink(), so we can remove EntityInterface::getSystemPath(), `FieldPluginBase::renderAsLink()` works with the URI string and not a path, since token replacement may need to happen on on the route arguments and/or query options and not just the path component.

I am setting this back to Needs Work, though, as I want to see if the final version of #2426181: Add a Url::fromUserInput() wrapper method for generating URLs from user-entered paths will require any logic changes here.

Simplified some of the logic per #11.

#2426181: Add a Url::fromUserInput() wrapper method for generating URLs from user-entered paths didn't affect this one way or another.

Added a test to FieldDropButtonTest to reproduce the one described in the issue summary.

Patch looks good to me. Thanks for the explanation, has a test, therefore RTBC.

+++ b/core/modules/views/src/Plugin/views/field/FieldPluginBase.php
@@ -1472,8 +1480,26 @@ protected function renderAsLink($alter, $text, $tokens) {
+    // We now have a heavily processed path as a URI string and set of options.
+    // If the original path was user entered, or had token replacement done to
+    // it, the current Url object may be unroutable and use the 'base:' scheme,
+    // which means outbound path processors won't run (e.g., path prefixing
+    // won't occur).  So, check for to see if we have a 'base:' URI, and then
+    // extract the path component and run it throught the path validator service
+    // to try to turn it into a routable Url object, so output path processors
+    // will run.
+    $final_url = FALSE;
+    if (parse_url($path, PHP_URL_SCHEME) === 'base') {
+      if ($final_url = $this->getPathValidator()->getUrlIfValidWithoutAccessCheck(parse_url($path, PHP_URL_PATH))) {
+        $final_url->setOptions($options);
+      }
+    }

Mh I try to understand it. So it would have been possible that we end up with base:node/1 right? In that case running the path validator is fine, but the documentation kinda talks about path processing. In that case the UnroutedUrlAssembler has a flag to force path processing to run.

@dawehner, in this view, we are getting `base:node/1` as a URI. What would you have the documentation say for this?

Updated IS; added Beta eval.

So should we be using the UnroutedUrlAssembler here?

I am not sure why we would want UnroutedUrlAssembler, as that more or less is the problem.

If we are ending up with 'base:node/1' as the final URI after token replacement, then we have a valid, routable path and should pass a routable Url() object to the link generator. Otherwise 'node/1' gets treated as an unrouted path, and gets sent off to the UnroutedUrlAssembler().

?

+++ b/core/modules/views/src/Plugin/views/field/FieldPluginBase.php
@@ -1472,8 +1480,26 @@ protected function renderAsLink($alter, $text, $tokens) {
+    // We now have a heavily processed path as a URI string and set of options.
+    // If the original path was user entered, or had token replacement done to
+    // it, the current Url object may be unroutable and use the 'base:' scheme,
+    // which means outbound path processors won't run (e.g., path prefixing
+    // won't occur).  So, check for to see if we have a 'base:' URI, and then
+    // extract the path component and run it throught the path validator service
+    // to try to turn it into a routable Url object, so output path processors
+    // will run.
+    $final_url = FALSE;
+    if (parse_url($path, PHP_URL_SCHEME) === 'base') {
+      if ($final_url = $this->getPathValidator()->getUrlIfValidWithoutAccessCheck(parse_url($path, PHP_URL_PATH))) {
+        $final_url->setOptions($options);
+      }
+    }
...
+    // If it was not yet set, use the Uri as-is and convert it into a Url object
+    // with ::fromUri()
+    if (!$final_url) {
+      $final_url = CoreUrl::fromUri($path, $options);
+    }
 

I'm curious whether we should just expand the options to set ['path_processing'] = TRUE. Don't we then ge the basic same result?

OK, here is the simple idea from #23. The patch is a clean checkout from HEAD with that option added + the new test. Passes locally for me.

Let's pretend that didn't happen.

I kinda like the way how it solved now, but on the other hand yeah the unbound URL assembler was never intended to always generate path aliases.

I kinda like the way how it solved now, but on the other hand yeah the unbound URL assembler was never intended to always generate path aliases.

Blerg. I forgot about all of the mocking that is needed in FieldPluginBaseTest, which would need to be addressed. So, which option do we want, real routes or unrouted paths + outbound processing? If the later, I'll fix the unit test.

No work unfortunately for a long time, removing sprint tag :/

If someone above my pay grade choose a direction (unrouted w/ path processing in #26 vs routes in #14), I'll wrap this up.

Personally, I think #14 is the best option.

FYI, Error still occurs in RC2.

Is this still broken?

Quick hand re-roll of test-only patch to see what happens, but we should check that it is still a valid test (mainly, that the class names haven't changed).

Looks like this is still an issue. Existing test passes. New one fails.

We need to pick

- #14: use the path validator to convert the post-token / post-processed path into a URL object, which will then get all of the related URL goodness
- #26: update the url generation in view plugin so that outbound path processing runs, regardless of whether the path is routable or not

Personally, I think the approach is #14 is better.

I think we should prefer #26 as per @dawehner's choice.

Both #14 (use path validator) and #26 (run outbound path processing) are outdated. Rerrolled #26.

This is a bug, so this should be 8.0.x eligible. I was going to work on this at DCNJ, esp since @dawehner will be there, too.

Looking at this now. Concerned that FieldDropButtonTest fails now, which should be fixed by the patch.

Blocked on #2657932: Url::toUriString() doesn't work properly for user input.

There has been no activity here for 9 years.

Can someone confirm that this is still a proeblem?

If we don't receive additional information to help with the issue, it may be closed after three months.

Thanks!

Wanted to bump this one more time

Since there's been no follow up going to close out. If still an issue please re-open