Comments

ded’s picture

Version: 7.x-1.x-dev » 7.x-1.1

This appears to still be an issue in current version. Has anyone solved this? If not I will look at a patch.

ded’s picture

Version: 7.x-1.1 » 7.x-1.x-dev
Status: Active » Needs review
FileSize
534 bytes
FAILED: [[SimpleTest]]: [MySQL] 341 pass(es), 1 fail(s), and 0 exception(s). View

Here is a patch which fixes this issue by adding the query parameters to the $url_options array before the url() call.

Status: Needs review » Needs work

The last submitted patch, query_string-1198032-2.patch, failed testing.

ded’s picture

FileSize
1.33 KB
FAILED: [[SimpleTest]]: [MySQL] 341 pass(es), 1 fail(s), and 0 exception(s). View

Here is a revised patch with the tests updated so that it checks the existence of the parameters.

ded’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, query_string-1198032-3.patch, failed testing.

tedavis’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
1.3 KB
PASSED: [[SimpleTest]]: [MySQL] 347 pass(es). View

Slight error in the url() structure of the last patch for the query. Should be sorted now.

coderintherye’s picture

What are module maintainers thoughts on this?

The patch looks fine, but I could see how it could be considered a breaking change by some. I did check and if one knows the query parameters that are expected for the page then one can build the url out of existing tokens: [current-page:url]?placement_id=[current-page:query:placement_id]#information

That said, it'd be really nice if the current url included the query parameters by default or if we added a token which included them, without having to build it out like the above.

coderintherye’s picture

Status: Needs review » Reviewed & tested by the community

Actually, I take it back, the syntax of [current-page:url]?query=[current-page:query:query]#anchor does not work, because if query is blank then token just spits out the token text itself.

The patch does indeed work though, so marking this as RTBC. The question remains if it should change the functionality of the existing token or become a new token, but if we go with the form then this patch should go in as is.

m4olivei’s picture

Also needed this, tried the patch, and it's working well as far as I can tell. Being used in a metatag token.

Dave Reid’s picture

My concern about including the current query string with the token is that we should probably ignore the 'page' query string? This token is used extensively with Metatag as the default for the 'Canonical' URL which should always be the same regardless of paged content. This would cause a regression there. We'd need to have a plan to resolve that.

D2ev’s picture

we had the same issue that 'Canonical' URL is ignoring the page query string. Patch #7 fixed it. +1 to RTBC

moxojo’s picture

Stumbled across this while trying to find a fix for canonicalizing paged content. Paged content should receive it's own self referential canonical tag. Google has written about this and said that pointing all pages to the first page is an incorrect use of the canonical tag. rel="next" and rel="prev" is how pagination should be handled. If you are going to canonicalize all paginated pages to a single page then it should be a view all page, not the first page in the series.

I am looking for a solution that will preserve pagination. Bonus if it allows us to select the url parameters to keep or exclude. For instance, if we have example.com/term?page=2&foo=bar&gclid=xxxxxx I would want to preserve the canonical for "page" and possibly for "foo" while excluding gclid which is a tracking param. This would allow us to use the canonical as intended for designating the authoritative url for duplicate content. As pagination changes the content and as other filtering params can drastically change the content we need to have the ability to canonicalize to those params while excluding params that create true duplicate pages.

Cheers!

DamienMcKenna’s picture

Maybe there should be a separate token that includes the full query string?

alexmoreno’s picture

+1 with @damienMcKenna I am in a project where we need the url clean of query strings

NWOM’s picture

Status: Reviewed & tested by the community » Needs work

#7 worked great for the standard [current-page:url] token, however did not work for the [current-page:url:relative] and [current-page:url:absolute] tokens.

When I get a chance, I'll setup a new environment with a dev version of Token to provide a patch. I'm currently using the Token version that is provided by the Panopoly distribution. In the meantime, I added the following manually:

        case 'absolute':
+          $url_options['query'] = drupal_get_query_parameters();
           $replacements[$original] = url($path, $url_options);
          break;
        case 'relative':
+          $url_options['query'] = drupal_get_query_parameters();
          $replacements[$original] = url($path, array('absolute' => FALSE) + $url_options);
          break;
NWOM’s picture

NWOM’s picture

Status: Needs work » Needs review
Alex Bukach’s picture

Version: 7.x-1.x-dev » 8.x-1.x-dev
Assigned: Unassigned » Alex Bukach
Status: Needs review » Active

This is not done in the 8.x as well.

Alex Bukach’s picture

Assigned: Alex Bukach » Unassigned
Status: Active » Needs review
FileSize
617 bytes

Status: Needs review » Needs work

The last submitted patch, 20: token-query-1198032-20-D8.patch, failed testing.

Alex Bukach’s picture

Status: Needs review » Needs work

The last submitted patch, 22: token-query-1198032-22-D8.patch, failed testing.

Alex Bukach’s picture