Rewrite external links in views fields

As I understood you use "Override the output of this field with custom text" option to replace the link text with the node title text, yes? Could You provide some screenshots of your link field output settings, because I could not reproduce this bug.

There is a patch which fix this bug. Please check if it works for you.

Updated the patch in #5 to pass the existing test. (Hopefully..)

Setting to needs review to fire up the testbot, but this still needs test.

Back to needs work for adding tests, and tagging for Views team.

I noticed a logic issue with this code that was sending it down the 'no url scheme' path regardless of whether it had one or not. This path coincidentally didn't use the previously parsed url. I've uploaded a diff containing the fix.

@joshuajleonard that bit is actually intentional because $path will always contain a scheme after $path = $alter['url']->setOptions($options)->toUriString();

In order for that to work the scheme check and $alter['external'] needs to be done earlier, so changed the patch to reflect that.

Also added a test that fails if you only use a token that contains a valid path.

That failing patch is empty, can you re-upload that.

Ah yeah, lets try that again.

Back to needs review since #14 passed, so that is the one that needs reviewing.

Tested this manually.

Steps taken (I wil add these to the IS):

1. Add a link field (field_link) to the Basic Page content type
2. Create a basic page node, set the link to "https://www.drupal.org"
3. Create a new view, outputting nodes
4. Uncheck the "Link to the Content" checkbox on the title field
5. Add the link field to the view
6. Check the "URL only" and "Show URL as plain text" checkboxes
7. Check the "Override the output of this field with custom text" checkbox, fill in "{{ title }}" in the textarea
8. Check the "Output this field as a custom link" checkbox, set "Link path" to "{{ field_link }}" and check "External server URL"

Behaviour on HEAD:

Behaviour after Patch:

Doing a code review next.

1. +++ b/core/modules/views/src/Plugin/views/field/FieldPluginBase.php
   @@ -1367,7 +1367,18 @@ protected function renderAsLink($alter, $text, $tokens) {
   +    // This allows a url of 'www.example.com' to be converted to 'http://www.example.com'.
   

   This exceeds 80 chars.

2. +++ b/core/modules/views/src/Plugin/views/field/FieldPluginBase.php
   --- a/core/modules/views/tests/src/Unit/Plugin/field/FieldPluginBaseTest.php
   +++ b/core/modules/views/tests/src/Unit/Plugin/field/FieldPluginBaseTest.php
   
   +++ b/core/modules/views/tests/src/Unit/Plugin/field/FieldPluginBaseTest.php
   @@ -478,6 +493,7 @@ public function providerTestRenderAsLinkWithPathAndTokens() {
   +    $data[] = ['{{ foo }}', ['{{ foo }}' => 'http://www.drupal.org'], '<a href="http://www.drupal.org">value</a>'];
   

   Shouldn't we add an external path without a schema also, so $path = "http://" . $path; is tested?

3. +++ b/core/modules/views/tests/src/Unit/Plugin/field/FieldPluginBaseTest.php
   @@ -441,17 +441,32 @@ public function testRenderAsLinkWithPathAndTokens($path, $tokens, $link_html) {
   +      $build =[
   ...
   +      $build =[
   

   These need a space after '='

@geertvd, thanks for the review.

#21.1 fixed
#21.2 is covered by \Drupal\views\Tests\Handler\FieldWebTest::testAlterUrl(). Not using a token, but that logic route is covered by tests.
#21.3 fixed

#21.2 is covered by \Drupal\views\Tests\Handler\FieldWebTest::testAlterUrl(). Not using a token, but that logic route is covered by tests.

Looks like that has enough test coverage there indeed.

Added beta eval

+++ b/core/modules/views/src/Plugin/views/field/FieldPluginBase.php
@@ -1367,7 +1367,19 @@ protected function renderAsLink($alter, $text, $tokens) {
+    $path = Html::decodeEntities($this->viewsTokenReplace($alter['path'], $tokens));

@@ -1396,7 +1408,7 @@ protected function renderAsLink($alter, $text, $tokens) {
       // However, we need to preserve special characters like " that
       // were removed by SafeMarkup::checkPlain().

This bit of the comment belongs with the decode entities... in fact can' we move the strip_tags up too?

As per #25, moved strip_tags up along with all the comments related to the things moved up.

While looking at that, occurred to me that we can lose the preg_replace for escaped twig tokens too, since all tokens will have been replaced before getting to that point, and with the new order won't be escaped before going to viewsTokenReplace, so no need to unescape them anymore.

Lets see if the testbot agrees.

Even though the tests seem to pass.
When I follow the steps to reproduce, and also add a node with link to ""

I get the following error:

InvalidArgumentException: The internal path component 'http:///' is external. You are not allowed to specify an external URL together with internal:/. in Drupal\Core\Url::fromInternalUri() (line 430 of /var/www/html/core/lib/Drupal/Core/Url.php).

So something is going wrong there, and we should add test coverage for also.

This also happens on #22 so it's not related to the latest changes.

This fixes that case, I'm not sure why UrlHelper::isExternal($path) was added there in the first place?
We will need extra test coverage.

Note: that was failing for all relative paths, not just .

UrlHelper::isExternal($path) was a left over from the first patch. With all the reshuffling that stayed in, but you are right, that can go.

I'll look into adding more tests.

Ran into #2567339: PHP Warning when using link field tokens in a view and got sidetracked.

Added some tests to work with empty paths, and added a check for that.

So <front> needs more special handling. Manual testing is ok for <front> with this version. Let's see if this passes.

ok, lets see what this does...

1. +++ b/core/modules/views/src/Plugin/views/field/FieldPluginBase.php
   @@ -1368,6 +1368,29 @@ protected function renderAsLink($alter, $text, $tokens) {
   +    // strip_tags() and viewsTokenReplace remove <front>, so check whether it's
   +    // different to front.
   

   Shouldn't that be "different than front"

2. +++ b/core/modules/views/src/Plugin/views/field/FieldPluginBase.php
   @@ -1368,6 +1368,29 @@ protected function renderAsLink($alter, $text, $tokens) {
   +      // Use strip tags as there should never be HTML in the path.
   

   Should be strip_tags

3. +++ b/core/modules/views/src/Plugin/views/field/FieldPluginBase.php
   @@ -1368,6 +1368,29 @@ protected function renderAsLink($alter, $text, $tokens) {
   +      // However, we need to preserve special characters like " that
   +      // were removed by SafeMarkup::checkPlain().
   

   "were" still fits on that first line

4. +++ b/core/modules/views/src/Plugin/views/field/FieldPluginBase.php
   @@ -1368,6 +1368,29 @@ protected function renderAsLink($alter, $text, $tokens) {
   +      strip_tags($path);
   

   This should be $path = strip_tags($path);

5. +++ b/core/modules/views/src/Plugin/views/field/FieldPluginBase.php
   @@ -1368,6 +1368,29 @@ protected function renderAsLink($alter, $text, $tokens) {
   +      if (empty($path) && empty($alter['url'])) return $text;
   

   Not using curly braces in if statements is against coding standards.

This is also still throwing the same error as in #27 for internal paths.
So our tests aren't covering this yet.

I'm also seeing this notice now:
Warning: strlen() expects parameter 1 to be string, array given in Drupal\Component\Utility\Unicode::validateUtf8() (line 691 of /var/www/html/core/lib/Drupal/Component/Utility/Unicode.php).

That notice is happening in HEAD now also, I'm not sure it it's in scope of this ticket.

@geertvd ran into the PHP warning too and opened #2567339: PHP Warning when using link field tokens in a view

This still needs test for #27, but it should fix the issue.

Not thrilled about

      // Tokens might have resolved URL's, as is the case for tokens provided by
      // Link fields, so all internal paths will be prefixed by base_path(). For
      // proper further handling reset this to internal:/.
      if (strpos($path, base_path()) === 0) {
        $path = str_replace(base_path(), 'internal:/', $path);
      }

But it turns out that tokens provided by the link module are already prefixed with base_path, and not with internal:/, so they would get prefixed with internal:/ after that and would have base_path() added twice. Couldn't come up with a better way to work around that.

Other points:
#41.1 Apparently not, if anything it should be 'different from'
#41.2 fixed
#41.3 fixed
#41.4 fixed
#41.5 fixed

Needs work, but just want to see what the testbot has to say.

ok, so base_path() needs special love to use in unit tests.

see #2529170: Remove DrupalKernel::initializeRequestGlobals and replace base_root, base_url and base_path with a service

namespace and newline fix.

I don't really like the if/elseif/else statement in the test. Why don't we add #context and expected return in the provider?
I think that would make it more readable and we wouldn't have to duplicate that block of code 3 times.

Straight up reroll. I agree the if/elseif/else is not good will take a look at that.

Index: core/modules/views/tests/src/Unit/Plugin/field/FieldPluginBaseTest.php
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8

That shouldn't be here. Are you at drupalcon? Maybe we can have look at this together.

This bit is needed to make the tests pass. See #47

+++ b/core/modules/views/tests/src/Unit/Plugin/field/FieldPluginBaseTest.php
@@ -567,3 +607,14 @@ public function setLinkGenerator(LinkGeneratorInterface $link_generator) {
+// @todo Remove as part of https://www.drupal.org/node/2529170.
+namespace {
+  if (!function_exists('base_path')) {
+    function base_path() {
+      return '/';
+    }
+  }
+}

That seems to have disappeared in your first reroll, patch in #55 still has that bit.

Going to look at this.

Going to reroll from #55 because of the feedback in #73.

This is a reroll of #55 and I confirm that the code referred to in #73 has been reinstated.

Redid the if/ifelse/else logic in the tests and put the external tests in a different test set.

I think the test looks a lot better like that
Just one small thing on that:

+++ b/core/modules/views/tests/src/Unit/Plugin/field/FieldPluginBaseTest.php
@@ -296,6 +296,10 @@ public function providerTestRenderAsLinkWithPathAndOptions() {
 
...
+

Can we remove those blank lines.

We still have a problem with internal paths though, when I use something like "/node/1" I get the following output:

<div class="views-field views-field-field-test">
  <div class="field-content">
    <a href="/nodeinternal%3A/1">node1</a>
  </div>
</div>

Fixed blank lines.

No idea why
<a href="/nodeinternal%3A/1">node1</a>
was not getting picked up by the tests, but fixed the reason it occurred.

Added a test that fails with #77 but passes with #79 (and hopefully with this one).

Not sure if this is not critical - by checking "Rewrite as External" you basically create WSOD for your View.

This fix needs to be implemented into core. I was hitting my head against the wall this afternoon trying to figure out what I was doing wrong. Not sure how many folks use the "Rewrite as External", but if they do then they are also going to run into this issue.

ye olde reroll, #2570895: FieldPluginBase can duplicate a suffix added the namespace/base_path snippet.

New to forums. Drupal 8. PHP 5.5.
I am trying to achieve the ability to click on an image and continue to a link.

In views, "rewrite results" of an image field, ticking "output this field as a custom link", using the token replacement pattern of a link field populated with a domain (ex. http://www.drupal.org ) in the "link path" eg. {{ field_link }}.

Firstly, I found that the replacement token was pointing to the link text field instead of the link value itself.
I went into the link field and disabled the "allow link text" and thereafter the token reflected the link value (kinda).

The link created was www.mydomain.com/http://www.drupal.com .
[This was correct except that I didn't expect (or desire) a prefix of www.mydomain.com/ in the result just the link value]

I change the "link path" to http://{{ field_link }} and the link became http//www.drupal.com which is what I wanted except the : had been stripped away. Realise also that the link field was already populated with http://www.drupal.org so I expected using the "link path" in this manner would result in http://http://www.drupal.org .

It is clear that between the link functionality and view that stuff is getting parsed somewhere and causing issues.

If I click on "external server URL" under rewrite results and try any of the above scenario the link disappears completely.

I have tried to change the link field to "external links only" or "both internal and external links" under all scenarios and that does not make a difference.

I hope this is helpful or relevant in determining scenarios in this area.
I will learn how to install patches and assist with testing.

Regards,
Paul

Work around:
If I allow the link field to allow "internal links" and populate the link value with /www.drupal.org while having the "link path" as http:/{{ field_link }} it results in a link of www.drupal.org.

The end result works for me...

My solution to fix this bug. (patch)

@paul_simmons @Lóna Lore could you please use the patch in comment #84 and let us know if that fixes your problem (and if not, why not), so we can maybe move this issue forward and get it committed.

I like smaller patch from #87
that just need to be combined with test from #84

@andypost, who doesn't like a smaller patch? :-)

Here is the patch that you asked for, #87 with tests from #84.

1 test fails, namely the one where a token is given with a relative path. The way to fix this is to add base: to the path in the token but I think it's a bit much to ask users to supply paths like that.

The patch in #84 addresses that problem and also refactors the current logic that makes all path calculations stop and get thrown out and overwritten when 'external' is set (which feels a bit silly).

So yeah maybe #84 tries to do to much, but it's mostly just reshuffling to get a more logical order of doing things.

This patch does not work when placeholder is empty

Missed adding one tweak to the tests to make more pass. With this version only 1 test should fail.

Tested using patch from #93

* Improvement: Scenario: For a link field, made the link text field available again. Despite the text field now being available again, the correct token was found ie. the link value equivalent to the url field.

* No change: If I rewrite results, link path with token {{ field_link }} where field_link value = http://www.drupal.org. The result is www.mydomain.com/http://www.drupal.org. The result still contains the domain prefix.

* Change: If I rewrite results and use a "link path" of http://{{ field_link }} there is now no output whatsoever.

* No change: If I tick the external server URL box while the link path is either {{ field_link }} or http://{{ field link }} as before there is no link displayed in the output.

No change: Modifying the link field properties to "external links only" or "both internal and external links" has no impact on the scenarios above. Correspondingly, where a link is made available in the results above it will either be prefixed by http:// or a single forward slash / as it was entered in the link field value.

* No change: My workaround of setting the link field to allow internal links and then populating the field value with /www.drupal.org , while rewriting the link path to http:/{{ field_link }} still works. The link value shows as www.drupal.org and on clicking proceeds correctly.

--> Diagnostic Note
If I tick the external URL box while otherwise the workaround settings are in place, the output for the link becomes http://http/xxxa href="http://www.drupal.org"yyyhttp://www.drupal.orgxxxayyy
In the line above xxx represents < , and yyy represents >. This line was not showing correctly in this forum.
This is the first time I see any link output while having the external URL box ticked.

Thank you Lendude for your patch. The token being passed correctly is an improvement. I hope my scenarios and results make sense to you.

@paul_simmons thanks for testing that patch! Did you happen try your scenarios with the patch in #84 too? I still feel that is the way to go to get this to really work, and think some of the failing scenarios you see with #93 should be fixed by #84.

Hi Lendude,

I reversed #93 and applied #84.

!! means different behaviour to base code.
== means same as base code
!93 means different behaviour to patch #93
=93 means same as patch #93

* Improvement !! / =93: For a link field, made the link text field available again. Despite the text field now being available again, the correct token was found ie. the link value equivalent to the url field.

* == / =93: If I rewrite results, link path with token {{ field_link }} where field_link value = http://www.drupal.org. The result is www.mydomain.com/http://www.drupal.org. The result still contains the domain prefix.

* !! / =93: If I rewrite results and use a "link path" of http://{{ field_link }} there is now no output whatsoever.

* !! / =93: If I tick the external server URL box while the link path is {{ field_link }} or http://{{ field link }} there is no link displayed in the output.

* == / =93: Workaround of setting the link field to allow internal links and then populating the field value with /www.drupal.org , while rewriting the link path to http:/{{ field_link }} still works. The link value shows as www.drupal.org and on clicking proceeds correctly.

--> Diagnostic Note
=93: If I tick the external URL box while otherwise the workaround settings are in place, the output for the link becomes http://http/xxxa href="http://www.drupal.org"yyyhttp://www.drupal.orgxxxayyy
In the line above xxx represents < , and yyy represents >. (This line was not showing correctly in this forum).
This is the first time I see any link output while having the external URL box ticked.

From my testing #84 had similar results to #93.
The token being passed correctly was the big improvement.
(The external server URL tick box is not yet assisting properly with enabling the rewritten link)

Thank you again Lendude.

Regards,
Paul

I can confirm that the patch at #84 works with 8.0.1, but it doesn't with 8.0.2 (cannot be applied).

I can second #98

This is the rerolled version of #84 which I still feel is the right way to go.

Removed a yml file that got into this patch from a completely different issue, that issue has been fixed in the mean time, hence the failure to apply.

Thanks a lot for rerolling the patch, @lendude. I can confirm the patch works with 8.0.2

#100 confirmed fix for 8.0.2

#100 confirmed fix for 8.0.3 -

thank you !
pls push to master ! :)

#100 works perfectly for 8.0.3, thanks a lot, guys!

Thanks @Lendude for your work on this and many other Views patches!

It looks to me like the issue was promoted accidentally in #24 -- the beta evaluation added in that comment says the issue is normal, but the comment promoted it accidentally. The core committers and Views maintainers (alexpott, xjm, effulgentsia, tim.plunkett, and dawehner) did not that this was a major issue, so downgrading. (It's definitely a bug though so hopefully we can get code reviews of @Lendude's patch and get the fix in once it's ready.)

#100 worked for me against 8.0.3

Just wanted to chime in and say this patch indeed works as advertised.

#100 works against 8.0.4.

Hi. I`m new at D8, how to apply patch #100?

Also confirming #100 works on 8.0.4 can we get it committed?

@ToxaViking see the guide pages on patching:
https://www.drupal.org/patch/apply

Committed/pushed to 8.1.x and cherry-picked to 8.0.x. Thanks!