Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
If I go to http://www.example.com/node?foo=bar, the [current-page:url] token should probably return the same.
Comment | File | Size | Author |
---|---|---|---|
#54 | 1198032-token-paginated-54.patch | 6.69 KB | jcisio |
Comments
Comment #1
ded CreditAttribution: ded commentedThis appears to still be an issue in current version. Has anyone solved this? If not I will look at a patch.
Comment #2
ded CreditAttribution: ded commentedHere is a patch which fixes this issue by adding the query parameters to the
$url_options
array before theurl()
call.Comment #4
ded CreditAttribution: ded commentedHere is a revised patch with the tests updated so that it checks the existence of the parameters.
Comment #5
ded CreditAttribution: ded commentedComment #7
Anonymous (not verified) CreditAttribution: Anonymous commentedSlight error in the url() structure of the last patch for the query. Should be sorted now.
Comment #8
coderintherye CreditAttribution: coderintherye commentedWhat 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.
Comment #9
coderintherye CreditAttribution: coderintherye commentedActually, 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.
Comment #10
m4oliveiAlso needed this, tried the patch, and it's working well as far as I can tell. Being used in a metatag token.
Comment #11
Dave ReidMy 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.
Comment #12
D2ev CreditAttribution: D2ev commentedwe had the same issue that 'Canonical' URL is ignoring the page query string. Patch #7 fixed it. +1 to RTBC
Comment #13
moxojo CreditAttribution: moxojo commentedStumbled 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!
Comment #14
DamienMcKennaMaybe there should be a separate token that includes the full query string?
Comment #15
alexmoreno CreditAttribution: alexmoreno at BBC Worldwide commented+1 with @damienMcKenna I am in a project where we need the url clean of query strings
Comment #16
NWOM CreditAttribution: NWOM commented#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:
Comment #17
NWOM CreditAttribution: NWOM commentedHere is the aformentioned patch.
Comment #18
NWOM CreditAttribution: NWOM commentedComment #19
Alex Bukach CreditAttribution: Alex Bukach commentedThis is not done in the 8.x as well.
Comment #20
Alex Bukach CreditAttribution: Alex Bukach commentedComment #22
Alex Bukach CreditAttribution: Alex Bukach commentedComment #24
Alex Bukach CreditAttribution: Alex Bukach commentedComment #25
gaurav.kapoor CreditAttribution: gaurav.kapoor at OpenSense Labs commentedThanks Alex Bukach. Last patch helped me a lot. Its fit to be RTBC if it is re rolled for latest dev branch.
Comment #26
dpagini CreditAttribution: dpagini as a volunteer commentedAttempt to re-roll against current dev branch. Adding a tests-only patch which I expect to fail, and the patch with the expected fix.
I'm not sure I know how to create an interdiff here (was going to try) since the current patch doesn't apply. Please advise if needed.
Comment #28
dpagini CreditAttribution: dpagini as a volunteer commented@gaurav.kapoor given your last comment, not sure if this should be changed to RTBC? I'll wait and let anyone chime in on this one. I took a stab at it b/c I'm having the problem and will need to pull in this patch. Thanks!
Comment #29
dpagini CreditAttribution: dpagini as a volunteer commentedHmm... I'm not sure this fully encapsulates the description. For me,
[current-page:url]
still does not contain the query string. Other similar tokens, like the ones updated in the tests ([current-page:url:relative]
,[current-page:url:absolute]
) do contain the querystring, but not simply[current-page:url]
.Comment #30
dpagini CreditAttribution: dpagini as a volunteer commentedHere's another shot... new tests file, adding the test for
[current-page:url]
and putting in code to hopefully pass the test.Comment #32
jennypanighetti CreditAttribution: jennypanighetti commentedHow does this token, or patch, work with i18n/translated views? I need a current-page:url tag with pagination query to use for the hreflang metatag. Is that supported here? For example, would the tag be [current-page:en-gb:url] ?
Comment #33
vakulrai CreditAttribution: vakulrai as a volunteer and at gai Technologies Pvt Ltd for gai Technologies Pvt Ltd commented@ dpagini I have applied the patch given at #30 and it gets applied successfully and I am able to get the query string by token.
adding image url's for reference.
thank's.
Comment #34
dpagini CreditAttribution: dpagini as a volunteer commented@jenstechs - I'm not too familiar with translation, so I know I can't speak to that, sorry!
@vakulrai - thanks for checking out the patch!
Given the feedback, would anyone mind if this goes to RTBC and hopefully get a maintainers eyes on this one?
Comment #35
cgmonroe CreditAttribution: cgmonroe as a volunteer commentedThis patch is a subset of the one as in this issue:
#1614456: [current-page:query:?] should fall back to empty string
So, it will not work correctly with a URL that has multiple query parameters: E.g. /blog?tag=security&page=4
The & will be encoded and become & which causes problems with SEO canonical links.
The encoding is happening at ~line 204 in Token.php.
Comment #36
cgmonroe CreditAttribution: cgmonroe as a volunteer commentedCurrent patch does not apply with latest dev code because of the coding standard changes. Re-rolled so that it applies now.
NOTE:
Use case for this is to generate SEO correct canonical values on paginated output. E.g. [current-page:url] should create
/blog?page=4&tag=security and not just /blog
Comment #37
dpagini CreditAttribution: dpagini as a volunteer commentedSo when I use the latest patch, and use [current-page:url] in the metatag module, I am getting the ampersand encoded as suggested in #35. I was thinking that #36 would address that, but it doesn't seem to do so for me.
Having said that... I think this all makes sense, but I was trying to really consider when I would use this. I think I first became interested in this issue b/c I wanted something like pagination in my canonical links. But I don't REALLY want the whole URL. What if someone comes in with with utm_campaign tags (or something like that)? All of a sudden Google is indexing my campaign parameters as my preferred page URL. It seems like other users are having similar thoughts... wouldn't a token like
[current-page:url:query:page&tag]
be a more useful token? Maybe there's already a way to do that...?Comment #38
Alex Bukach CreditAttribution: Alex Bukach commentedRegenerated against current dev.
Comment #39
surfgatinho CreditAttribution: surfgatinho as a volunteer commentedThis is an SEO disaster! Having the canonical page for all paginated pages pointing to the first (non querystring) page is preventing any of the following pages from being indexed.
Google have recently admitted they never even looked at the 'prev' / 'next' tags so those further pages may as well not exist.
In view of this I'd suggest this is quite should be quite an urgent fix...
Comment #40
surfgatinho CreditAttribution: surfgatinho as a volunteer commentedGiven the impact this has on SEO via the canonical tag I feel it is fairly serious.
Comment #41
robpowellI applied this patch to token 8.x-1.5 and it applied cleanly. The view page I was testing had the canonical_url set to [current-page:url:relative] and before this patch, url args would not be passed. After applying the patch, I see the args in the header,
I am marking this RTBC.
Comment #42
robpowellComment #43
ckngReroll for latest dev.
Comment #44
ckngReroll, fixed path error.
Comment #45
keopxReroll
Comment #46
ngkoutsaik CreditAttribution: ngkoutsaik at Agiledrop - Your Trusted Drupal Teammates commentedHi,
the latest patch works for me. I run it against the latest branch and the tests pass.
Comment #47
AnybodyConfirming RTBC!
Comment #48
orlando.thoenyPatch #45 works for me as well, using Drupal 9.0.2.
Comment #49
BerdirTorn on this one, it is a minor behavior change, but according to some comments here, sites with multiple pages should have their own canonical URL. Going to be a bit awkward as content moves further back on pages, as it won't be on the page that google thinks it is, but oh well, the same already happens now.
Will try to remember mentioning this in the release notes.
What about offering a token that does *not* contain the query though? Something like current-page:url:no-query? I can imagine cases where this would be a problem, then those people at least have an option to use a different token.
Comment #50
foreveryo CreditAttribution: foreveryo commentedAs @berdir points out having the no-query should also exist. I've used the patch to solve the issue in paginated views, but now I have issues on regular nodes.
It seems Google add known parameters to other nodes (like articles) and it now returns as canonical the URL with the parameter...
So, it would be great to have 2 tokens. One with parameters and another one without them. In my opinion would be better to add a new one for the URL with parameters and keep the other as it was to prevent regressions.
Comment #51
foreveryo CreditAttribution: foreveryo commentedI ended up with a custom module solution. I leave it here as it might be useful for someone else. Can be used also as a base for extending the tokens with a query attached.
Comment #52
Summit CreditAttribution: Summit commentedHi, Foreveryo, what a great name!
It would be great to have this in this module! +1!
Greetings and great christmas!
Comment #53
jcisio CreditAttribution: jcisio as a volunteer and at Axess Open Web Services for UNESCO commentedSo based on #51 and previous patches, a new one with paginated url tokens and leaving the old one intact. According to previous comments, adding query string to current tokens is a behavior change and I think it could cause more headache then what it is trying to solve. Query could be used for sorting etc. and should not change canonical url.
I'm wondering should the url-with-query version be added too.
Comment #54
jcisio CreditAttribution: jcisio as a volunteer and at Axess Open Web Services for UNESCO commentedNew patch with both versions: two new tokens.
Comment #56
jcisio CreditAttribution: jcisio as a volunteer and at Axess Open Web Services commentedSomething is wrong in the assert.
Comment #57
jcisio CreditAttribution: jcisio as a volunteer and at Axess Open Web Services commentedOk the last fail is probably due to a core bug #2968558: Canonical (and other) links with multiple query parameters have ampersand encoded. Either to fix core or to fix how token compare urls.
Comment #58
jcisio CreditAttribution: jcisio as a volunteer and at Axess Open Web Services commentedJust to test CI.
Comment #60
jcisio CreditAttribution: jcisio as a volunteer and at Axess Open Web Services commentedI'm not sure if it is a core bug. I'm trying to reproduce the bug locally but it is not successfully. I can't get the ampersand.
Comment #61
parisekThank you, working for me, but CI test still not happy. Maybe best we can is separate problematic token to another issue?
Comment #62
sashken2 CreditAttribution: sashken2 commentedI'm use patch #21 from https://www.drupal.org/project/token/issues/1614456
And it works good for me.
Comment #63
generalredneckGiven that this issue is 5 years older than my oldest child and it can watch PG-13 without supervision, I created a module that handles the scenario and a few like it.
I present Token Url Plus.
This module (as the time of this writing) provides the tokens:
[current-page:url-with-query]
The URL of the current page with query string included, if it exists. This is the functionality we were looking for here.
[current-page:url-with-query:without-some-parameters:param1,param2,…]
The URL of the current page with query string included but filtered by removing the provided query parameters. e.g. 'without-some-parameters:utm_campaign,utm_medium,utm_source' would be replaced with the current url, with the three utm parameters filtered out if they existed.
This is advanced usage for what you are looking for here
[current-page:url-with-query:with-some-parameters:param1,param2,…]
The URL of the current page with query string included but filtered to only include the provided query parameters. e.g. 'with-some-parameters:page,category_id' would return a url containing only query parameters, '?page=1&category_id=2' if they existed.
You can use this one with metatags on views to support JUST the query parameters that the view provides.
This will allow Token the module to remain unchanged and take the burden off of the decision for backward compatibility as expressed in #11.