The node link views handlers are currently using the same function for creating links as all other fields do when they use the make_link option.

This usually works fine, but if you have a replacement pattern that for some reason interferes with the link in any way, the links can break.

In my case we had a view with two terms as arguments and we added the node edit link. The node edit link has a destination GET parameter which links back to the view, and since that parameter gets url encoded, we got things like %2 in the destination variable. That got replaced with the term name in the link, which caused some problems =)

The attached patch overrides the render_as_link function in the node_link field handler and empties the tokens array. That way, no tokens can interfere with the link output. The change should be quite unintrusive as far as other functionality goes since it only happens in the node link handler.

CommentFileSizeAuthor
#60 views-rendered_links_token-1009646-60.patch5.14 KBnagy.balint
#54 views-rendered_links_token-1009646-interdiff-52.txt1.49 KBShaunDychko
#54 views-rendered_links_token-1009646-54.patch5.12 KBShaunDychko
#52 views-rendered_links_token-1009646-52.patch4.98 KBShaunDychko
#50 views-rendered_links_token-1009646-50-test-only.patch2.54 KBgeertvd
#45 views-rendered_links_token-1009646-45-tests-only.patch2.54 KBgeertvd
#45 views-rendered_links_token-1009646-44-with-tests.patch3.83 KBgeertvd
#45 views-rendered_links_token-1009646-40-with-tests.patch3.65 KBgeertvd
#44 views-rendered_links_token-1009646-44.patch1.29 KBShaunDychko
#40 interdiff_38-40.txt670 bytesheddn
#40 views-rendered_links_token-1009646-40.patch1.08 KBheddn
#38 views-rendered_links_token-1009646-38.patch1.18 KBheddn
#36 views-render_as_link_token_replacement-1009646-36.patch1.52 KBheddn
#35 Selection_010.png9.94 KBheddn
#33 views-render_as_link_token_replacement-1009646-33.patch1.67 KBheddn
#31 views-render_as_link_token_replacement-1009646-31.patch1.3 KBheddn
#29 views-render_as_link_token_replacement-1009646-29.patch0 bytesheddn
#28 views-render_as_link_token_replacement-1009646-28.patch818 bytesheddn
#25 views-prevent-token-urlencode-conflict-1009646-25.patch788 bytesFreekVR
#24 views-1009646-24.patch1.02 KBjrao
#23 views_node_links.patch1.32 KBkhaldoon_masud
#19 views_node_links.patch1.59 KBigor.ro
views-dont-use-tokens-with-node-links.patch807 bytesfabsor
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

fabsor’s picture

Status: Active » Needs review

Just setting status.

merlinofchaos’s picture

Does this break if you turn on link rewriting and actually put a token in the link?

bohz’s picture

Subscribing.
I seem to have a similar problem with an edit link with destination portion of the path like:
/edit?destination=taxonomyFtermF785

while it should read something like:
/edit?destination=taxonomy/term/785

I have applied the patch above to views_handler_field_node_link_edit.inc
and now it seems to work as expected.

Thanks a lot

ordually’s picture

Subscribing. I had this problem and applying the patch works for me.

I tried to investigate Merlin's question on tokens and find that the "Output this field as a link" options "Replacement patterns" listing shows arguments and fields in the view, but I don't see tokens listed. I tried tossing in a "[nid]" token for yuks and it didn't replace pre or post-patch.

@merlin: Thanks for the great module and incredible support!

merlinofchaos’s picture

Status: Needs review » Needs work

I'm considered that indiscriminately removing the tokens is a bad idea.

I think we should check to see if the checkbox is there, meaning the link was prepopulated. We certainly wouldn't want tokens in that case anyway. It doesn't burden the UI, and tokens will still work as expected if the link is rewritten normally.

braindrift’s picture

Subscribing

braindrift’s picture

@merlinofchaos
What sense has the option "Output this field as a link" with its token replacement for a field, which will be rendered as link anyway with its own path etc.?
IMHO this patch does exactly what it should do. Maybe it should also remove the option "Output this field as a link".

merlinofchaos’s picture

Because if I have chosen to rewrite the link, even if a default link was provided for me, I probably want tokens. I can't rewrite a link as node/[nid] without them.

braindrift’s picture

You can not rewrite a node-link. This option has no effect for this fields.

braindrift’s picture

OK, I was wrong. This option has only an effect, if the user has no permission to edit/delete the node. In this case the rewritten path is used.

braindrift’s picture

Is there a special reason for the argument tokens not to be placed inside square brackets?

braindrift’s picture

@merlinofchaos: What about to put the tokens for arguments also inside square brackets (e.g. %1 => [%1])?

merlinofchaos’s picture

Changing that would break the thousands or hundreds of thousands of sites that assume the existing behavior.

braindrift’s picture

Thanks for your reply.
What about an auto-repair function? This function could search for the old tokens and automaticly raplace them by the new one.
The fact is that encoded URLs with something like %2F must not be replaced.

ayalon’s picture

I have the same problem! I have some Fs in the edit links

The edit link looks like this:

http://www.test.com/node/28/edit?destination=nodeF21

Originally
http://www.test.com/node/28/edit?destination=node%2F21

%2 gets replaced.

If I understand merlinofchaos correctly I shouldn't apply the patch. But is there a solution to display the URLs correctly?

rnj786’s picture

I have a similar problem with node edit link, when using the view with exposed filter and AJAX turned ON the node edit link becomes too long and the page says Request-URI Too Large.

 node/678/edit?destination=awaiting-parts-queue%3FDate_Range%5Bmin%5D%5Bdate%5D%3D%26Date_Range%5Bmax%5D%5Bdate%5D%3D%26Store%3DNorth%2520Houston%26field_parts_status_value%3DAll%26field_awaiting_part_ro_value%3D%26form_build_id%3Dform-l3jqVKAXVGBTpBFnevCdccwl4NCuQk-OMkMEpUu5khM%26form_token%3DHJlVMvvjIm1DBCPnh-6uiI4VgHc5QbO56_4sExQyWJY%26form_id%3Dviews_exposed_form%26view_name%3Dawaiting_parts_queue%26view_display_id%3Dpage_1%26view_args%3D%26view_path%3Dawaiting-parts-queue%26view_base_path%3Dawaiting-parts-queue%26view_dom_id%3D1%26pager_element%3D0%26ajax_html_ids%5B0%5D%3Dskip-link%26ajax_html_ids%5B1%5D%3Doverlay-disable-message%26ajax_html_ids%5B2%5D%3Doverlay-profile-link%26ajax_html_ids%5B3%5D%3Doverlay-dismiss-message%26ajax_html_ids%5B4%5D%3Dtoolbar%26ajax_html_ids%5B5%5D%3Dtoolbar-home%26ajax_html_ids%5B6%5D%3Dtoolbar-user%26ajax_html_ids%5B7%5D%3Dtoolbar-menu%26ajax_html_ids%5B8%5D%3Dtoolbar-link-admin-dashboard%26ajax_html_ids%5B9%5D%3Dtoolbar-link-admin-content%26ajax_html_ids%5B10%5D%3Dtoolbar-link-admin-structure%26ajax_html_ids%5B11%5D%3Dtoolbar-link-admin-appearance%26ajax_html_ids%5B12%5D%3Dtoolbar-link-admin-people%26ajax_html_ids%5B13%5D%3Dtoolbar-link-admin-modules%26ajax_html_ids%5B14%5D%3Dtoolbar-link-admin-settings-performance_logging-memcache_clear%26ajax_html_ids%5B15%5D%3Dtoolbar-link-admin-config%26ajax_html_ids%5B16%5D%3Dtoolbar-link-admin-reports%26ajax_html_ids%5B17%5D%3Dtoolbar-link-admin-advanced_help%26ajax_html_ids%5B18%5D%3Dtoolbar-link-admin-help%26ajax_html_ids%5B19%5D%3Dedit-shortcuts%26ajax_html_ids%5B20%5D%3DtotalContainer%26ajax_html_ids%5B21%5D%3Dtop%26ajax_html_ids%5B22%5D%3Dbranding%26ajax_html_ids%5B23%5D%3Dtitle-slogan%26ajax_html_ids%5B24%5D%3Dsite-title%26ajax_html_ids%5B25%5D%3Dnavigation-primary%26ajax_html_ids%5B26%5D%3Dmenu-main-title-198%26ajax_html_ids%5B27%5D%3Dmenu-main-title-614%26ajax_html_ids%5B28%5D%3Dmenu-section-title-558%26ajax_html_ids%5B29%5D%3Dmenu-section-title-576%26ajax_html_ids%5B30%5D%3Dmenu-section-title-578%26ajax_html_ids%5B31%5D%3Dmenu-section-title-553%26ajax_html_ids%5B32%5D%3Dmenu-main-title-554%26ajax_html_ids%5B33%5D%3Dmenu-main-title-613%26ajax_html_ids%5B34%5D%3Dmenu-section-title-612%26ajax_html_ids%5B35%5D%3Dmenu-section-title-556%26ajax_html_ids%5B36%5D%3Dmenu-section-title-731%26ajax_html_ids%5B37%5D%3Dmenu-section-title-703%26ajax_html_ids%5B38%5D%3Dmenu-main-title-710%26ajax_html_ids%5B39%5D%3Dmenu-main-title-707%26ajax_html_ids%5B40%5D%3Dmenu-main-title-702%26ajax_html_ids%5B41%5D%3DpageBorder%26ajax_html_ids%5B42%5D%3Dnavigation-secondary%26ajax_html_ids%5B43%5D%3Dsecondary-menu%26ajax_html_ids%5B44%5D%3DcontentWrapper%26ajax_html_ids%5B45%5D%3Dbreadcrumb%26ajax_html_ids%5B46%5D%3DinnerContent%26ajax_html_ids%5B47%5D%3DsiteContent%26ajax_html_ids%5B48%5D%3Dpage-title%26ajax_html_ids%5B49%5D%3Dcontent%26ajax_html_ids%5B50%5D%3Dblock-system-main%26ajax_html_ids%5B51%5D%3Dviews-exposed-form-awaiting-parts-queue-page-1%26ajax_html_ids%5B52%5D%3Dedit-date-filter-min-wrapper%26ajax_html_ids%5B53%5D%3Dedit-date-filter-min%26ajax_html_ids%5B54%5D%3Dedit-date-range-min-datepicker-popup-0%26
 

How can i get rid of the Destination part ?

rnj786’s picture

I got the query string removed from the URL, that solved the problem for now but i don't know if that would cause any parallel issues.

Removed line 26
// $this->options['alter']['query'] = drupal_get_destination(); from the file
views_handler_field_node_link_edit.inc

Please point out if there is a better solution than removing the destination query string.

tbenice’s picture

hi rnj786,

it's a bit off topic here but to remove the destination you could rewrite the edit link output and set it to output a link 'node/[nid]/edit'. you'd of course have to pull in the nid field and set it to exclude display.

igor.ro’s picture

FileSize
1.59 KB

Patch work perfect but I found out the same bug with node delete link
so provide updated patch

igor.ro’s picture

Status: Needs work » Needs review
braindrift’s picture

This should not be the solution for the problem, because this would also prevent the replacement of tokens that must be replaced.
e.g in the "Link class" or the "Alt text".

IMHO the better solution would be to change the way the token replacement is performed in the views_handler_field.inc on line 1154.
old code:

    if (isset($alter['query'])) {
      // Convert the query to a string, perform token replacement, and then
      // convert back to an array form for l().
      $options['query'] = drupal_http_build_query($alter['query']);
      $options['query'] = strtr($options['query'], $tokens);
      $options['query'] = drupal_get_query_array($options['query']);
    }

new code:

    if (isset($alter['query'])) {
      // perform token replacement on each query element separately
    	foreach ($alter['query'] as $q_name => $q_value) {
	    	$options['query'][$q_name] = strtr($q_value, $tokens); 
    	}
    }
kla2t’s picture

subscribe

khaldoon_masud’s picture

Status: Needs review » Fixed
FileSize
1.32 KB

The correct patch to work for "node edit" and "node delete" link:

jrao’s picture

Version: 6.x-3.x-dev » 7.x-3.x-dev
Status: Fixed » Needs review
FileSize
1.02 KB

Here's a patch for 7.x based on #21

FreekVR’s picture

I think the best way to fix this for D6 is to decode the URL before processing tokens. This effectively means that default destination paths for node edit links for example will not get URL encoded characters (eg %2F) replaced, and tokens will still work, since %2 is not a valid URL encoding.

The attached patch is rolled against views 6.3.x

Status: Needs review » Needs work

The last submitted patch, 25: views-prevent-token-urlencode-conflict-1009646-25.patch, failed testing.

Gribnif’s picture

Status: Needs work » Needs review

The patch in #24 also corrects the problem in #2004960: rewrite destination parameter when using node delete links.. I think it's RTBC for 7.x.

heddn’s picture

This should do the trick. Re-encoding another time causes issues.

heddn’s picture

Title: Node link and node edit link should not use replacement patterns » Rendered links token replacement URL encoding is broken
FileSize
0 bytes

Status: Needs review » Needs work

The last submitted patch, 29: views-render_as_link_token_replacement-1009646-29.patch, failed testing.

heddn’s picture

azinck’s picture

Status: Needs review » Needs work

Seems to me that #31 won't work. The token replacement *must* be done on the individual values in the query array before it's rendered as an encoded query string. Consider what would happen in #31 if there's a parameter value that contains an ampersand? That ampersand would be decoded, then split on in drupal_get_query_array().

heddn’s picture

  • I built a simple test view of nodes where the %2 is a contextual filter. %2 is important because many URL replacements use %2 + a number. %26 = an ampersand, %20 = a space, etc.
  • On this view there is also an exposed title filter.
  • The title of the node I'm visiting has 'notice & news' in the name.
  • When I filter the node by its title, the url looks like: node-test/5?title=notice+%26+news.
  • The second position is 5 and is used as a contextual filter.
  • The edit link: /node/5/edit?destination=node-test/5%3Ftitle%3Dnotice%20%26%20news
  • delete link: /node/5/delete?destination=node-test/5%3Ftitle%3Dnotice%20%26%20news

So far, so good. I edit the node and save it. The URL now looks like: node-test/5?title=notice%20&news=
The ampersand lost its encoding and '&n' isn't a valid character so the contents of the title filter now is 'notice '. Everything from the & and after is lost.

OK, I got my notes written. Now to study things and figure out what is going on and if we can salvage everything after the ampersand. I think, in general, the patch here improves things measurably. But we might be able to get better. The only thing not functional is the loss some of the contents of the exposed filter.

Status: Needs review » Needs work

The last submitted patch, 33: views-render_as_link_token_replacement-1009646-33.patch, failed testing.

heddn’s picture

Issue summary: View changes
FileSize
9.94 KB

Screen capture of test view:

heddn’s picture

heddn’s picture

Status: Needs work » Needs review
heddn’s picture

azinck’s picture

+++ b/handlers/views_handler_field.inc
@@ -1338,11 +1338,13 @@ function render_as_link($alter, $text, $tokens) {
+      $options['query'] = drupal_get_query_array(drupal_http_build_query($alter['query']));

Is there a reason we need to send this through this round trip? Why not just set $options['query'] = $alter['query']?

heddn’s picture

re #39: I don't recall if the reason is simply because that is how it was done before or if there was a larger reason. Let's see how the tests fair. There seems to be decent test coverage of this part of the field handler.

azinck’s picture

Status: Needs review » Reviewed & tested by the community

#40 looks good and is working for me.

NWOM’s picture

#40 worked for me as well. Thank you for everyone's work on this!

cha0s’s picture

Just wanted to drop another affirmative for #40 :)

ShaunDychko’s picture

Status: Reviewed & tested by the community » Needs work
FileSize
1.29 KB

#40 didn't quite work for the use case where a destination query parameter in $alter['query'] already contains a URL encoded entity.og_handler_field_og_membership_link_edit::render_link from Organic Groups does this by using drupal_get_destination() here:

    if (!empty($this->options['destination'])) {
      $this->options['alter']['query'] = drupal_get_destination();
    }

When using an exposed filter with a text field containing a space, the URL will be like group/node/1237/admin/people?uid=The+Name&state=All but drupal_get_destination() returns group/node/1237/admin/people?uid=The%20Name&state=1. Notice the plus replaced by %20.

The following patch returns to the idea of #31 by decoding the $alter['query'] array, but makes the decoding recursive since the query parameter can possibly contain nested arrays (see drupal_http_build_query() which also accounts for nested arrays).

The critique against #31 in #32 was "Consider what would happen in #31 if there's a parameter value that contains an ampersand?" While the url decoding doesn't modify any literal '&' characters, it would change ;amp into '&', which will cause problems since drupal_get_query_array() will make erroneous splits on the new literal '&' characters introduced by decoding.

Sorry I don't have the answer about what to do here. #40 fails if $alter['query'] has something that's already url encoded, whereas the patch here fails if a query parameter contains an ampersand. Setting to "needs work" :P

geertvd’s picture

Added some test coverage for this issue and applied it to both the patch in #40 and #44.
All of these tests should fail for now.

The last submitted patch, 45: views-rendered_links_token-1009646-40-with-tests.patch, failed testing.

The last submitted patch, 45: views-rendered_links_token-1009646-44-with-tests.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 45: views-rendered_links_token-1009646-45-tests-only.patch, failed testing.

geertvd’s picture

+++ b/tests/handlers/views_handler_field_url.test
@@ -56,5 +56,64 @@ class ViewsHandlerFieldUrlTest extends ViewsSqlTest {
+    $field_destination['name']['alter']['query'] = $destination;

This should be $name_field instead of $field_destination

geertvd’s picture

Status: Needs work » Needs review
FileSize
2.54 KB

Test only patch where I fixed that typo.

Status: Needs review » Needs work

The last submitted patch, 50: views-rendered_links_token-1009646-50-test-only.patch, failed testing.

ShaunDychko’s picture

Status: Needs work » Needs review
FileSize
4.98 KB

Thanks for the tests @geertvd! Here's another go. It works for any url encoded characters! The only limitation I can see is that it won't replace token that are in the keys of the $alter['query'] array. Would there ever be tokens in the keys? I didn't think so, but if that's important, then maybe this patch could be extended for that requirement. This patch includes the tests from #50.

The approach for this patch is to use regex to apply token replacement only on strings not containing a url encoded character, as matched by /(%[0-9a-fA-F]{2})/.

If a value contains a urlecoded string, that string is decoded since this array is later passed to l(), the documentation for which says that query parameters should not be encoded. From https://api.drupal.org/api/drupal/includes%21common.inc/function/url/7.x

'query': An array of query key/value-pairs (without any URL-encoding) to append to the URL.

.

Passing url encoded characters to l() results in the percent sign being encoded as %25. For example "John%20Smith" becomes "John%2520Smith".

Status: Needs review » Needs work

The last submitted patch, 52: views-rendered_links_token-1009646-52.patch, failed testing.

ShaunDychko’s picture

Adjusted the test to pass a decoded query parameter to l().

jomarocas’s picture

I try the patch #54 and try and dont working, i have a views with simple url to edit, with certainly filters, apply and execute the filters, later go to edit, but nothing no works

jomarocas’s picture

Priority: Normal » Major
Status: Needs review » Needs work

i Consider this is major and need work, node edit link no working while your apply filters using or not ajax

Richard15’s picture

+1

Chris Matthews’s picture

The two year old patch in #54 to views_handler_field.inc and views_handler_field_url.test does not apply to the latest views 7.x-3.x-dev.

Checking patch handlers/views_handler_field.inc...
error: while searching for:
  var $aliases = array();

  /**
   * The field value prior to any rewriting.
   *
   * @var mixed

error: patch failed: handlers/views_handler_field.inc:47
error: handlers/views_handler_field.inc: patch does not apply
Checking patch tests/handlers/views_handler_field_url.test...
error: while searching for:
    $this->executeView($view);

    $this->assertEqual(l('John', 'John'), $view->field['name']->advanced_render($view->result[0]));
  }
}

error: patch failed: tests/handlers/views_handler_field_url.test:56
error: tests/handlers/views_handler_field_url.test: patch does not apply
Chris Matthews’s picture

The two year old patch in #54 to views_handler_field.inc and views_handler_field_url.test does not apply to the latest views 7.x-3.x-dev.

Checking patch handlers/views_handler_field.inc...
error: while searching for:
  var $aliases = array();

  /**
   * The field value prior to any rewriting.
   *
   * @var mixed

error: patch failed: handlers/views_handler_field.inc:47
error: handlers/views_handler_field.inc: patch does not apply
Checking patch tests/handlers/views_handler_field_url.test...
error: while searching for:
    $this->executeView($view);

    $this->assertEqual(l('John', 'John'), $view->field['name']->advanced_render($view->result[0]));
  }
}

error: patch failed: tests/handlers/views_handler_field_url.test:56
error: tests/handlers/views_handler_field_url.test: patch does not apply
nagy.balint’s picture

nagy.balint’s picture

Issue tags: -Needs reroll