Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
I have a few blocks that have more links that point to URL's other than View pages. I've previously used the Footer section to provide these links, but thought it might be easier if I could enter a custom URL in the Link Display section for a block...
What do you think?
Comment | File | Size | Author |
---|---|---|---|
#198 | 564106-more-links-198.patch | 10.07 KB | larowlan |
#198 | interdiff-803744.txt | 3.5 KB | larowlan |
#191 | views-more-link-token-replacement_564106_190.patch | 10.38 KB | neetu morwani |
#173 | 564106-167-171-interdiff.txt | 1.24 KB | gnuget |
#171 | interdiff_167-171.txt | 3.14 KB | ilya.no |
Comments
Comment #1
merlinofchaos CreditAttribution: merlinofchaos commentedThat is a decently good idea.
Comment #2
dawehnerHere is an initial patch.
I'm not sure whether the url should be called use_more_url or more_url. For me use_more_url and currently use_more_text does not make a lot of sense for me.
Comment #3
BWPanda CreditAttribution: BWPanda commentedJust changed some of the wording and edited the help text under the 'Create more link' checkbox...
Otherwise works fine!
Comment #4
stBorchertLooks really good (without testing its functionality).
RTBC from me.
Comment #5
dawehnerPerfect.
Just mark as rtbc
Comment #6
momper CreditAttribution: momper commentedsubscribe
Comment #7
dawehnerBut it does not work always...
For example on block views it does not display the more link
Comment #8
dawehnerHere comes a patch which fixes this. Its only one line which has changed.
Comment #9
dawehner.
Comment #10
BWPanda CreditAttribution: BWPanda commentedGreat, works for me!
Comment #11
drupov CreditAttribution: drupov commentedHi,
this is great work!
I have one more question: how is it possible to pass an argument in that Custom URL? E.g. I want to have the link contain the UID, which is passed into a panel url - should be something like "user/%"?
Comment #12
BWPanda CreditAttribution: BWPanda commentedOoh, that's a nice idea. Might have to make it a separate feature request though, unless someone can come up with an easy fix...?
Comment #13
momper CreditAttribution: momper commented+1
Comment #14
dicreat CreditAttribution: dicreat commentedGood idea.
Any solution for using a dynamic Views arguments or/and token patterns in "more links" URL?
In my View I have one block with two arguments and one page with URL "my_page/%". I need "more link" with only one argument in URL, but I can't do this, because Views always insert both arguments in URL.
Thanks for help.
UPDATE: I found proper issue #578834: More link to point to custom URL - Arguments enhanced
Comment #16
merlinofchaos CreditAttribution: merlinofchaos commentedPlease reconcile this and http://drupal.org/node/578834#comment-2083184 into just one issue for me.
Comment #17
merlinofchaos CreditAttribution: merlinofchaos commentedOk, as I look at this, specializing onto 'more_url' like this is wrong.
Instead, we should allow link_display to provide a path override. That will work for everything not just more links, which is IMO desirable. So that way, the 'link display' option would 1) always appear if a view does not have its own path, and 2) use dependencies so that you choose either linking to another display, or linking to a simple URL.
More or less I think you could add something like this to function get_path():
Then fix up the link_display UI. Possibly make a note in the 'more' link checkbox that the path to link to can be set in the link display section.
Comment #18
Agileware CreditAttribution: Agileware commentedSubscribing
Comment #19
merlinofchaos CreditAttribution: merlinofchaos commentedBumping because I would like to see this completed.
Comment #20
dawehnerSo this time its under link_display
Comment #21
BWPanda CreditAttribution: BWPanda commentedI tried testing this patch against the latest dev version (13 March) but got the following output:
It seems dereine's patch was made against a different version...
I instead edited the views_plugin_display.inc file manually, but couldn't work out how to use link_display (I couldn't find it's settings in the Views interface)... I've attached my patch.
Marking as 'Needs Work' due to the failed hunk above, but if mine works instead change it back to 'Needs Review'.
Comment #22
dawehnerI made a patch against 3.x, of course. I will make a 2.x too.
Comment #23
Agileware CreditAttribution: Agileware commentedFYI,
Here is a patch that might be of interest to people using this patch.
#715484: Target selection for read more link
I have not tested it in combination with this patch yet.
They are both patching similar areas of the same file so they might not apply cleanly over each other at this stage.
Comment #24
dawehnerYour patch looks fine, too
Comment #25
Anonymous (not verified) CreditAttribution: Anonymous commentedThe patch works, but there is a big BUT. The link display option only shows if you have 2 or more pages in your view.
The patch doesnt cover that. E.g. I have a view with a block and want a custom read more link. Not possible. Have to create two "dummy" pages routing to 'unused' in order to see the "link display" options to enter the custom url.
Thanks for the effort though, I am using it and really need it. But its a bit dumb to have to add 2 dummy pages each time.
Comment #26
dawehner@morningtime
I'm not really sure we need this. You could also use a view which points to a panel. So just on display is needed.
Comment #27
prasannah.ganeshan CreditAttribution: prasannah.ganeshan commentedHi,
I having the same problem with the views module. But I'm unable to use these patches.
First of all my problem starts at getting a custom link to the more link generated by the view. But I would be interested to know how it will work for arguments as well.
If someone had fixed this issue for a newer version of views module Please let me know.
-Prasannah
Comment #28
coolhandlukek2 CreditAttribution: coolhandlukek2 commentedSubscribing - thx
Comment #29
n_nelson350 CreditAttribution: n_nelson350 commentedHave to check and test the patch
Comment #30
guntherdevisch CreditAttribution: guntherdevisch commentedNice patch! Thanks! It works great. Will it be in the next Views releases?
Comment #31
davepoon CreditAttribution: davepoon commentedI vote for this, sometimes it is useful to enter a custom URL rather than the default Views page link.
Comment #32
locomo CreditAttribution: locomo commentedsubscribe
Comment #33
dagmarMarked this #578834: More link to point to custom URL - Arguments enhanced as a duplicate.
Lets fix this firsts in the 3.x, new features request are for 3.x.
Comment #34
dagmarOk, here is the patch including #578834: More link to point to custom URL - Arguments enhanced and request from @morningtime in #25
There is a part that needs a bit of explanation:
This check is to avoid views includes automatically the argument at the end of the link. If users are including things like mypath/!1/other_stuff this check avoid get paths more links pointing to mypath/1232/other_stuff/1232
Comment #35
dawehnerHere is a rerole against views 2.x
Comment #36
iamjon CreditAttribution: iamjon commentedUn-marked #574768: preserve block display arguments in read more link path as a duplicate.
My bad.
Comment #37
drewschmaltz CreditAttribution: drewschmaltz commentedFirst of all, I needed this exact functionality and was shocked when it inserted my argument in the more link. It was phenomenal how intuitive it worked.
But, what I'd also like to see (if possible) is an argument replacement ability within the more link text.
The reason is mostly for SEO. In my case, I show the top 5 air filters from every manufacturer I carry. Then, below each top five it says "more air filters". But, what I would like it to say is "more Toro air filters" where Toro is %1 argument.
Thanks!
Comment #38
mstrelan CreditAttribution: mstrelan commented@merlinofchaos -
Why have it in two sections? Why not just put the link display options in the more link section? Or is there more to the link display than I thought?
Comment #39
brenes CreditAttribution: brenes commentedYes, the function to add arguments to the more link text (as mentioned in #37) would be neat.
For a yearly archive I am displaying nodes of the year the current node is filed under in a sidebar block and it would make sense to add the year argument to the more link text. So that its saying something like this "more nodes of 2010"
Best Regards
Comment #40
RavenHursT CreditAttribution: RavenHursT commentedAre these patches supposed to work with the current 3.0 alpha?? I'm trying to apply them using tortoise and they aren't doing a darn thing.
Comment #41
dawehnerPlease test it with 6.x-3.x-dev. Patches are always against the development version and not an really quite old "stable" version.
Personally i would use the 6.x-3.x-dev is more stable then the 3.0-alpha3 but this is just a personal view.
Comment #42
merlinofchaos CreditAttribution: merlinofchaos commentedYes, absolutely. Summary links and other stuff go to the link display. It's not JUST for the more link, though that's the most common usage.
Comment #43
merlinofchaos CreditAttribution: merlinofchaos commentedOk, I hate to kick this back to needs work, as I think we're close.
On the "Link display" we have both a set of radio buttons AND a custom URL textfield. Which one will get used is very confusing.
Let's add another radio button: "Custom URL".
That controls the visibility of the custom URL, and controls whether or not that gets used. That is more explicit both in the UI and in the code what should get used.
The comment in #37 about using argument tokes in the more link is definitely a good idea and we should consider just adding that.
Also, "More link" and "Link display" are separated by the "Access" widget. Let's move "Access" down so that More link and Link display are together.
I think this is very close and incredibly useful, so I'm going to prioritize this one up.
Comment #44
dawehner* added the radio element
* Arguments:
Manually testing it works fine. The tokens are used.
Not sure whether views_plugin_display::get_link_display should be changed, too.
Comment #45
khosman CreditAttribution: khosman commentedWhen I attempt to use this patch (using 6.x-3.x-dev.), I get an error:
Fatal error: Call to undefined method views_plugin_display_page::render_pager() in /home/tvs...cms/sites/all/modules/views/theme/theme.inc on line 79
The "patching" seems to have gone smoothly and a spot check seems to show the patch placed correctly, but my site freaks. Thoughts?
Comment #46
samuelsov CreditAttribution: samuelsov commentedIn the last patches #35 and #44, there is no way to not use the arguments. If we don't set any argument, the defaults arguments are set automatically. I think we should not set argument if none is given so do something like :
Comment #47
dagmarRerolled
Comment #48
LetUsBePrecise CreditAttribution: LetUsBePrecise commentedThis is awesome idea. Hope this gets implemented soon.
Comment #49
dealancer CreditAttribution: dealancer commentedsubscribing
Comment #50
dagmarRerolled
Comment #51
merlinofchaos CreditAttribution: merlinofchaos commentedI have been thinking that link_display and link_url should actually be combined. The link display would be a list of displays and a 'Custom URL' option, and the custom URL option would open up a textarea. Then things that get the path and url for the display would check that and use the appropriate URL.
Comment #52
Mozzy CreditAttribution: Mozzy commentedIs there a patch for D7?
Comment #53
dawehnerNot yet
Comment #54
stevectorDo I need a patch like this to meet the case described in #1203262: Option to exclude arguments from URL in more link. ?
Comment #55
pcambraHere's a version for review for D7
Comment #56
franzsub
Comment #57
obrienmd CreditAttribution: obrienmd commentedsub
Comment #58
EclipseGc CreditAttribution: EclipseGc commentedpatch for current head:
Comment #59
EclipseGc CreditAttribution: EclipseGc commentedupdated the patch and tested it. Loving this.
Comment #60
dawehnerYeah. Wow this was a lot of work for this feature, Thanks for everyone contributed to this patch!
Commited eclipse's last patch to 7.x-3.x and the patch of dagmar to 6.x-3.x
Comment #61
merlinofchaos CreditAttribution: merlinofchaos commentedOk, this is hard coded for the more link.
That is totally wrong. You can't have a setting in 'link display' that only affects the 'more' link. Other places use the link display too and the link_url will not override there. This needs to be baked deeper in via view::get_url
Comment #62
merlinofchaos CreditAttribution: merlinofchaos commentedOkay, two things:
view::get_url() ends up calling into display_plugin::get_path(). So *that* is where link display override should take place.
This code:
When doing a piece of code that overrides link_display, doing a search on where link_display is actually used probably would've helped identify the right place to adjust the code.
Now, the second bit is that view::get_url() does argument modification automatically. But adding a simple flag to skip argument handling could make that skippable very easily.
Something like:
Then the code in get_url() could check that flag; if it's false, do what it does now. If it's true, do the token substitition instead.
Comment #63
mesr01 CreditAttribution: mesr01 commented+1
Comment #64
cecemel CreditAttribution: cecemel commentedHi
I am using git to apply the patch. When applying, I recieve the following error:
As I am new to this process, could someone explain me how i should solve this?
thank you
Comment #65
jwilson3In January 2011, Earl said this was close to a solution. Sad to see the ball was dropped, as this would be hugely useful for a pretty typical views+panels workflow, or is there any other way to get a Views content pane display to link to a totally unrelated panel page, representing the "full list" of content?
Comment #66
merlinofchaos CreditAttribution: merlinofchaos commentedYou can always use hook_panels_pane_content_alter (I think that's what its' called) and add links there. It's not ideal but it can serve the purpose.
Comment #67
jwilson3Thanks. The one downside of using hook_panels_pane_content_alter is you dont know if the view has more results or not, in order to conditionally display the more link based on the number of results. That being said, there's probably a views alter hook that I could use to inject the links into the view too. ;)
Comment #68
tar_inet CreditAttribution: tar_inet commentedCurrent custom URL is ok if you don't need to add any query as ?param1=value because drupal needs it as a different param for url function. To allow that I added a new option in "Link display" for pages of custom url.
You can add a list custom key/value-pairs as param1=value1¶m2=value2. If any key is present as a filter in current view it will be override in more link.
Comment #69
Encarte CreditAttribution: Encarte commented@tar_inet thanks for your patch. You should make it against the latest dev and then change this issue from «needs work» to «needs review» (otherwise it may just be ignored by the maintainers, who aren't enough for all the work).
Comment #70
tar_inet CreditAttribution: tar_inet commentedThanks Encarte, very good page. Here is the new patch. I can't show you any evidence it is working but I'm using it in my current project for Catch
Comment #71
tar_inet CreditAttribution: tar_inet commented#70: views-custom-url-query-for-more-link-564106-68.patch queued for re-testing.
Comment #72
dawehnerPlease use spaces instead of a tab here, this kind of looks ugly
can't the code take care of the decoding?
I think there should be similar code in views_handler_field, because you can set a query there as well, maybe you could share some of the code. Additional there are drupal api functions for that.
Comment #73
tar_inet CreditAttribution: tar_inet commentedUps, I didn't realize about these tabs... I solved it.
About html entities, I tried to do it without it and it fails so I had to change it. When I see this message without html entity it shows ¶m2 (¶m2~¶SEMICOMAm2). I prefer to not use it there but I don't know how to it, any idea?
And it's true, there is a function I didn't know: drupal_get_query_array. I changed that.
I has been looking handlers/views_handler_field.inc and it can be a good idea only for custom urls but not if you want to add custom params to current view's pages. If you have a better idea about how to do it please tell me and I change the code. An improvement can be add token to custom url or query.
Comment #74
tar_inet CreditAttribution: tar_inet commentedI had a problem with last attachment, I send it again in this comment
Comment #75
dawehnerviews_handler_field does something like that:
I'm wondering whether we could extract this into a reusable method.
Comment #76
chernetsky CreditAttribution: chernetsky commented#20: views-more-url_0.patch queued for re-testing.
Comment #77
rooby CreditAttribution: rooby commentedI haven't had a chance to address #75 but here is a reroll of #74 that applies to latest dev.
With this applied I am successfully using a more link with query params.
Comment #78
Esculap CreditAttribution: Esculap commentedI tried #77 patch. "!1 == ... input" token isn't applied to the custom query
Comment #79
Esculap CreditAttribution: Esculap commentedComment #80
merlinofchaos CreditAttribution: merlinofchaos commentedI think it would be better to use drupal_parse_url() to pull the query string and other goodies from the URL and preserve them.
Comment #81
troybthompson CreditAttribution: troybthompson commentedPatch #79 solved my problem using !1 perfectly. The only thing I see is a slight formatting issue where dependent-options class should be added to the DIV so it indents like the Custom URL field.
Comment #82
zterry95 CreditAttribution: zterry95 commented#79 patch works fine for me. Thx.
Comment #83
gsivaprabu CreditAttribution: gsivaprabu commentedI changed using #79 patch but nothing changed in my Views Block Settings.
How to fix that ? Please any one help me i am new to Drupal.
Comment #84
rv0 CreditAttribution: rv0 commentedA new patch using drupal_parse_url as merlinofchaos suggested in #80
This also preserves any url fragments (#anchor)
Patch is merging the user defined query strings with existing exposed filter query arguments.. Not sure which of them should be allowed to overwrite the other.
Comment #85
gmclelland CreditAttribution: gmclelland commentedSimilar issue #1449248: When using "Override path" and an argument with "input on pane config", the argument is appended to the overridden path
Comment #86
gmclelland CreditAttribution: gmclelland commentedSorry for the noise that issue was for ctools not views.
Comment #87
rjacobs CreditAttribution: rjacobs commentedThat's an interesting question. It looks like the current patch gives precedence to the exposed filter arguments if there is a conflict. I might think that any manually defined strings from the user should always have priority (and thus the relevant
array_merge()
call could be modified). I assume that most of the time a user would use this setting to point to a path other than the current view, in which case any query strings from exposed filters on the current page are mostly irreverent. I'm not totally sure about that though.Also, is this the only pending question? This issue has a lot of history, and I've not followed all of it, so I'm wondering where things stand. The current patch is pretty clean and I'm trying to asses if this could go back to RTBC (pending resolution on the point above)?
Comment #88
rooby CreditAttribution: rooby commentedThe patch in #84 doesn't seem like a replacement for the one in #79.
The one in #79 adds a new setting for a custom query string but the one in #84 doesn't.
Comment #89
rv0 CreditAttribution: rv0 commented@rooby
The whole idea in #84 is making it so that there aren't any settings needed.
this per module maintainer request in #80:
Comment #90
rooby CreditAttribution: rooby commentedOh yep I see now. Sorry I glossed over it a bit quick.
Comment #91
Ludo.R#84 works perfectly to me! :)
Tested with 7.x-3.11
Comment #92
stefan.r CreditAttribution: stefan.r commented#84 looks great
Comment #93
colanWe've recently switched our testing from the old qa.drupal.org to DrupalCI. Because of a bug in the new system, #2623840: Views (D7) patches not being tested, older patches must be re-uploaded. On re-uploading the patch, please set the status to "Needs Review" so that the test bot will add it to its queue.
If all tests pass, change the Status back to "Reviewed & tested by the community". We'll most likely commit the patch immediately without having to go through another round of peer review.
We apologize for the trouble, and appreciate your patience.
Comment #94
rv0 CreditAttribution: rv0 at Coworks.be commentedas per #93, same patch as in #84 to activate the test bot
Comment #95
rv0 CreditAttribution: rv0 at Coworks.be commentedas per #93, setting back to "Reviewed & tested by the community"
Comment #97
colanThanks!
Comment #98
rv0 CreditAttribution: rv0 at Coworks.be commented@colan
Wrong patch attribution :(
Comment #99
colanJust reverted & recommitted. Sorry about that! That's what I get for trusting Dreditor.
Comment #100
kellyimagined CreditAttribution: kellyimagined commentedHope this helps to answer the original question: https://www.drupal.org/node/578834#comment-10847634
Comment #101
dawehnerTo be honest the custom more link functionality is one of the things I would let people solve with templating instead of UI code.
Comment #102
dawehnerI don't really think that this is a major issue
Comment #104
therealssj CreditAttribution: therealssj commentedPatch for 8.2.x
Comment #105
colanComment #106
larowlan$url is an object here
Also this is a bug, the feature exists in core now, but it doesn't respect existing options on the Url.
Comment #107
larowlanthis fixes the bug
Comment #108
dawehnerThank you @larowlan for posting a patch on this issue.
Can't we replace these lines with
$options += ['query' => []]; $url_options += ['query' => []]; $url_options['query'] = array_merge($options['query'], $url_options['query']
and this is all?Comment #109
larowlanyeah
larowlan--
such a lazy mongrel
Comment #111
damontgomery CreditAttribution: damontgomery at Acquia commentedQueued patch in #107 against 8.2.0
Comment #112
dawehner.
Comment #113
BarisW CreditAttribution: BarisW at LimoenGroen commentedYes we can!
Comment #114
realgt CreditAttribution: realgt as a volunteer and commentedi added one line to replace escaped params when you had multiple query params in the url.
for example, this url was not working:
speeches?speaker={{raw_arguments.nid}}&field_speaker_target_id={{ arguments.nid }}
It would result in a malformed url:
speeches?speaker=3941&field_speaker_target_id=John%20Doe
instead of what it should have been:
speeches?speaker=3941&field_speaker_target_id=John%20Doe
because the second parameter was stored as 'amp;field_speaker_target_id' in the $url_options['query'] array.
probably not the best way to solve it but it worked for me, thought i'd share
Comment #117
shadcn CreditAttribution: shadcn at Chapter Three commentedAdding some tests here to show some use cases:
node
- internal without leading slash - Expected to pass./node
- internal with leading slash - Expected to fail.http://drupal.org
- external - Expected to fail.Comment #118
dawehnera) I would have never expected that external URLs are supported. It is good to have a test, but this sounds like a feature request for me
b) Regarding /node vs. node, ideally IMHO we should support both for now, with a clear UI documentation of what is allowed.
c) Note: your test coverage is missing of what the issue title says: query parameters and fragments. Let's ensure to have them in the test.
Comment #120
shadcn CreditAttribution: shadcn at Chapter Three commenteda) Hmm, the field description says you can add external url. So bug? (see image below)
b) Agreed. Related issue: #2423913: Leading slash in link fields and views has different UX.
c) I'll add tests for query parameters.
Comment #121
shadcn CreditAttribution: shadcn at Chapter Three commentedAdded tests for query paramaters.
Comment #123
shadcn CreditAttribution: shadcn at Chapter Three commentedLet's make them green now.
Comment #125
shadcn CreditAttribution: shadcn at Chapter Three commentedUpdated the assert messages. Let's try this again.
Comment #127
dawehnerDo you mind opening up an issue to maybe provide a common method on Url to do exactly that?
We have
$url->mergeOptions()
now. You could leverage that here.Comment #128
shadcn CreditAttribution: shadcn at Chapter Three commented@dawehner, #2851056: Add a Url method to create a Url from a URI or an external path.
Comment #129
shadcn CreditAttribution: shadcn at Chapter Three commentedAdded
$url->mergeOptions()
.Comment #130
shadcn CreditAttribution: shadcn at Chapter Three commentedActually, we could use a few more tests here.
Comment #131
shadcn CreditAttribution: shadcn at Chapter Three commentedOK added a few more tests.
Comment #133
shadcn CreditAttribution: shadcn at Chapter Three commentedLet's try this again.
Comment #135
dawehnerYou need to take into account that you are on subdir installation on the testbot. Better use $this->assertLink
Comment #136
shadcn CreditAttribution: shadcn at Chapter Three commentedUpdated. Thanks @dawehner.
Comment #137
dawehnerIMHO the better approach might be to parse the url and then run the token function on the query and the main url separate
Comment #138
shadcn CreditAttribution: shadcn at Chapter Three commentedUpdated as per #137 and added a few more tests.
Comment #139
Pavan B S CreditAttribution: Pavan B S at Valuebound commentedLine is exceeding 80 characters.
Applying the patch please review.
Comment #141
jofitz CreditAttribution: jofitz at ComputerMinds commentedRe-rolled.
Comment #142
dawehner... This line of documentation should tell the reader, why this is needed. For example: Parse the path, because we want to apply token replacement on the path and query string.
If there a reason we have this
if
here? As far as I remember there are more arguments in viewsTokenReplace than just the getArgumentsTokensDo you mind using
\Drupal\Tests\views\Kernel\Plugin\DisplayKernelTest
as this is a kernel test and as such run way faster? This will require though some changes below :(Comment #143
sukanya.ramakrishnan CreditAttribution: sukanya.ramakrishnan at Workday, Inc. commentedFolks,
Thanks for this awesome patch.
I am trying out this patch to build a custom URL that has a query string that can work with facets.
so my querystring is like f[0] = key:value.
The patch above doesnt work with this usecase because the line $parts['query'][$name] = $this->viewsTokenReplace($value, $tokens) in Displaypluginbase returns an empty string ($value is an array in my case).
Submitting a minor modification to the patch in 141 for my usecase.
Thanks,
Sukanya
Comment #144
sukanya.ramakrishnan CreditAttribution: sukanya.ramakrishnan at Workday, Inc. commentedSubmitting a reroll of patch in 143 for drupal 8.2
Comment #145
jhedstromPatch no longer applies (I think some views tests were converted to phpunit.)
Comment #146
dawehnerJust a quick reroll
Comment #148
pk188 CreditAttribution: pk188 at OpenSense Labs commentedRe rolled.
Comment #150
dragos-dumi CreditAttribution: dragos-dumi commentedThis part will strip multidimensional array (as example using a more link to a search with facets f[0], f[1] etc)
other than this, it seems to be working fine. thx.
Comment #152
sukanya.ramakrishnan CreditAttribution: sukanya.ramakrishnan at Workday, Inc. commented@dragos-dumi, Thanks for your comments, the part you mentioned was added for the exact use case, search with facets. It is inside a loop for the query params , so there is always one item in the item i believe. Comments appreciated.
Thanks
Sukanya
Comment #153
sukanya.ramakrishnan CreditAttribution: sukanya.ramakrishnan at Workday, Inc. commentedComment #154
sukanya.ramakrishnan CreditAttribution: sukanya.ramakrishnan at Workday, Inc. commentedSubmitting a patch after fixing failing tests and adding a comment to explain the code for which concern was raised in #150.
Comment #156
pookmish CreditAttribution: pookmish commented#154 works great on my side.
Comment #158
dragos-dumi CreditAttribution: dragos-dumi commentedIn my comment in #150 I was referring that maybe there should be another foreach on $value when is array or even a recursive function if the parameters is a more than 1 dimensional array (although would be a rare usecase)
Now it replaces token only for first [0] and stripping rest of array (let's say you have f[0] f[1] f[2])
attaching test, intrediff and patch
Comment #159
dragos-dumi CreditAttribution: dragos-dumi commentedComment #161
dragos-dumi CreditAttribution: dragos-dumi commentedComment #162
dagmarThanks! There are 6 nested ifs in that function. There is probably a better way to implement the same logic.
Comment #163
Dinesh18 CreditAttribution: Dinesh18 as a volunteer commentedComment #164
Dinesh18 CreditAttribution: Dinesh18 as a volunteer commentedHere is the updated patch and interdiff.txt
Comment #165
Dinesh18 CreditAttribution: Dinesh18 as a volunteer commentedComment #167
wesleydv CreditAttribution: wesleydv at District09 commentedI refactored #161 to avoid all those nested ifs.
Comment #168
gnugetThanks for the patch I gave it a review and it is almost ready just a couple nits:
The summary lines must start with a third person singular present tense verb So it should be
Replaces
andGets
The first letter after the period needs to be capitalized.
These changes are perfect for new contributors, so I'm going to add the tag.
Thanks again!
Comment #169
dawehnerThis looks pretty good for me. Thank you for documenting the usecase of facets!
Comment #170
dawehnerThis looks pretty good for me. Thank you for documenting the usecase of facets!
Comment #171
ilya.no CreditAttribution: ilya.no as a volunteer and at Skilld for Skilld commentedAttaching patch with fixes for #168 comment and also changed one line, because patch couldn't be applied.
Comment #172
ilya.no CreditAttribution: ilya.no as a volunteer and at Skilld for Skilld commentedComment #173
gnugetYes #167 needed a reroll because #2958429: Properly deprecate SafeMarkup::format() was commited.
All looks good, the only thing that looks weird is the interdiff it displays several more changes than expected so I made it again and this time it displays the expected changes.
(Interdiff attached)
Thanks!
Comment #174
gnugetComment #176
gnugetComment #178
ragnarkurm CreditAttribution: ragnarkurm as a volunteer commentedDoes not work for me.
What I need is to create more like:
mypage?x=y
But I get no more link, although I have enabled " Always display the more link".
Talking about #171.
What I see in code is:
My questions are:
(The patch does not apply to 8.5.5, probably never intended to).
Comment #179
gnugetHi ragnarkurm.
Where is that code?
What I got after applying #171 is:
David.
Comment #181
gnugetRandom testbot fail.
Comment #183
gnugetComment #185
gnugetComment #187
gnugetComment #188
charly71 CreditAttribution: charly71 commentedHi,
someone can tell if #171 patch works with Drupal 8.6.x?
Thanks
Comment #189
gnugetI'm using it with Drupal 8.6.2 and it is working as expected.
Comment #191
neetu morwani CreditAttribution: neetu morwani as a volunteer and commentedReroll patch for 167th patch to make it compatible with latest code for 8.6.x branch.
Comment #192
caspervoogt CreditAttribution: caspervoogt at Plethora commentedthe patch from #191 worked for me on the latest 8.6 branch. There does not appear to be a way to translate the More Link path, but that may be a separate issue.
Comment #193
gnugetAnd the tests passed.
So let's switch back this to RTBC :-)
Thanks.
Comment #195
gnugetwhy this fails so frequently?
:-(
Comment #196
Krzysztof DomańskiUnrelated test failure. Back to RTBC.
Comment #197
alexpottI'm not sure that assignment in a super complex
if
like this is the way to go. Also do we have test coverage when this doesn't return a URL? We've not documented that there's a chance this won't return a Url object.Should this not be part of the getMoreUrl() method?
I think we should keep new methods being added to the base class to a minimum. Let's make replacePathTokens() part of getMoreUrl() and only move it into it's own protected method when we have more than one use-case.
Comment #198
larowlanaddresses those changes
Comment #199
kim.pepperComment #201
PieterDCThanks @larowlan for your patch. It seems to work.
And thanks to everyone else involved getting to this point.
Comment #202
hanness#198 works for me as well, many thanks everyone!
I'm not a coder, so only TBC, no RTBC from my side.
Comment #203
volkswagenchickTagging for DrupalNorth 2019
Comment #204
gnugetI re-reviewed the patch and it looks great! (and I just ran again the tests and all passed) so I think this is RTBC
Comment #206
webchickThis looks like a really nice clean-up, with an extensive set of test coverage, fixing a ten year old issue. :O
Committed and pushed to 8.8.x, along with a few minor English fixes. Thanks!
I might propose this for "highlights" too just given how many people are subscribed here and how long this issue is.