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?

CommentFileSizeAuthor
#198 564106-more-links-198.patch10.07 KBlarowlan
#198 interdiff-803744.txt3.5 KBlarowlan
#191 views-more-link-token-replacement_564106_190.patch10.38 KBneetu morwani
#173 564106-167-171-interdiff.txt1.24 KBgnuget
#171 interdiff_167-171.txt3.14 KBilya.no
#171 views-more-link-token-replacement_564106_171.patch10.22 KBilya.no
#167 interdiff_158-167.txt18.97 KBwesleydv
#167 views-more-link-token-replacement_564106_167.patch10.38 KBwesleydv
#164 564106-164.patch9.57 KBDinesh18
#164 interdiff.txt3.7 KBDinesh18
#161 views-more-link-564106-158-codestyle-fix.patch9.36 KBdragos-dumi
#158 views-more-link-564106-158.patch9.36 KBdragos-dumi
#158 views-more-link-564106-158-interdiff.txt2.47 KBdragos-dumi
#158 views-more-link-564106-158-testonly.patch9.34 KBdragos-dumi
#154 views-more-link-564106-154.patch8.75 KBsukanya.ramakrishnan
#148 views-more-link-564106-148.patch8.59 KBpk188
#146 564106-146.patch8.61 KBdawehner
#144 Views-more-link-564106-drupal8.2-reroll.patch8.91 KBsukanya.ramakrishnan
#143 views-more-link-564106-143.patch8.55 KBsukanya.ramakrishnan
#143 interdiff.txt982 bytessukanya.ramakrishnan
#141 564106-141.patch8.31 KBjofitz
#139 interdif.txt844 bytesPavan B S
#139 more_links_pointing_to-564106-139.patch8.33 KBPavan B S
#138 interdiff-564106-136-138.txt3.84 KBshadcn
#138 more_links_pointing_to-564106-138.patch8.32 KBshadcn
#136 interdiff-564106-133-136.txt3.26 KBshadcn
#136 more_links_pointing_to-564106-136.patch7.19 KBshadcn
#133 interdiff-564106-131-133.txt508 bytesshadcn
#133 more_links_pointing_to-564106-133.patch7.56 KBshadcn
#131 interdiff-564106-129-131.txt4.7 KBshadcn
#131 more_links_pointing_to-564106-131.patch7.59 KBshadcn
#129 interdiff-564106-125-129.txt1020 bytesshadcn
#129 more_links_pointing_to-564106-129.patch4.45 KBshadcn
#123 interdiff-564106-121-123.txt1.45 KBshadcn
#123 more_links_pointing_to-564106-123.patch4.38 KBshadcn
#121 interdiff-564106-117-121.txt983 bytesshadcn
#121 more_links_pointing_to-564106-121-fail.patch2.79 KBshadcn
#120 564106.jpg88.37 KBshadcn
#117 more_links_pointing_to-564106-117-fail.patch2.29 KBshadcn
#114 multiple_query_params-564106.fix_.patch1.54 KBrealgt
#113 564106-113.fix_.patch1.13 KBBarisW
#107 564106.fix_.patch1.4 KBlarowlan
#107 interdiff.txt1.43 KBlarowlan
#104 564106-104.patch1.46 KBtherealssj
#94 views-custom-url-query-for-more-link-564106-94.patch1.19 KBrv0
#84 views-custom-url-query-for-more-link-564106-84.patch1.19 KBrv0
#79 views-custom-url-query-for-more-link-564106-79.patch3.24 KBEsculap
#77 views-custom-url-query-for-more-link-564106-77.patch3.13 KBrooby
#74 views-custom-url-query-for-more-link-564106-73.patch3.04 KBtar_inet
#73 views-custom-url-query-for-more-link-564106-73.patch0 bytestar_inet
#70 views-custom-url-query-for-more-link-564106-68.patch3.38 KBtar_inet
#68 custom-url-query-for-more-links.patch3.58 KBtar_inet
#58 564106.patch8.21 KBEclipseGc
#55 564106-More_link_to_point_to_custom_URL-55.patch8.14 KBpcambra
#50 views.564106.50.patch8.33 KBdagmar
#47 564106-47-views-link-custom-url.patch8.13 KBdagmar
#44 564106-link-custom_url.patch8.14 KBdawehner
#35 views-564106-2.x.patch8.13 KBdawehner
#34 views-564106.patch7.96 KBdagmar
#27 patch-result.jpg85.32 KBprasannah.ganeshan
#21 views-more-url_1.patch3.46 KBBWPanda
#20 views-more-url_0.patch3.55 KBdawehner
#8 views-more-url.patch3.76 KBdawehner
#3 views-more-url_564106_3.patch3.29 KBBWPanda
#2 views-more-url.patch2.52 KBdawehner
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

merlinofchaos’s picture

That is a decently good idea.

dawehner’s picture

Status: Active » Needs review
FileSize
2.52 KB

Here 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.

BWPanda’s picture

Just changed some of the wording and edited the help text under the 'Create more link' checkbox...

Otherwise works fine!

stBorchert’s picture

Looks really good (without testing its functionality).
RTBC from me.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Perfect.

Just mark as rtbc

momper’s picture

subscribe

dawehner’s picture

Status: Reviewed & tested by the community » Needs work

But it does not work always...

For example on block views it does not display the more link

dawehner’s picture

FileSize
3.76 KB

Here comes a patch which fixes this. Its only one line which has changed.

+      $path = $path ? $path : $this->get_option('more_url');
dawehner’s picture

Status: Needs work » Needs review

.

BWPanda’s picture

Status: Needs review » Reviewed & tested by the community

Great, works for me!

drupov’s picture

Hi,

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/%"?

BWPanda’s picture

Ooh, that's a nice idea. Might have to make it a separate feature request though, unless someone can come up with an easy fix...?

momper’s picture

+1

dicreat’s picture

Good 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

merlinofchaos’s picture

Please reconcile this and http://drupal.org/node/578834#comment-2083184 into just one issue for me.

merlinofchaos’s picture

Status: Reviewed & tested by the community » Needs work

Ok, 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():

  $override_path = $this->get_option('link_url');
  if ($override_path) {
    return $override_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.

Agileware’s picture

Subscribing

merlinofchaos’s picture

Bumping because I would like to see this completed.

dawehner’s picture

Status: Needs work » Needs review
FileSize
3.55 KB

So this time its under link_display

BWPanda’s picture

Status: Needs review » Needs work
FileSize
3.46 KB

I tried testing this patch against the latest dev version (13 March) but got the following output:

$ patch -p0 < views-more-url_0_0.patch
patching file plugins/views_plugin_display.inc
Hunk #1 succeeded at 184 with fuzz 2 (offset -69 lines).
Hunk #2 FAILED at 260.
Hunk #3 succeeded at 366 with fuzz 1 (offset -18 lines).
Hunk #4 succeeded at 448 (offset -105 lines).
Hunk #5 succeeded at 954 (offset -131 lines).
Hunk #6 succeeded at 1187 (offset -97 lines).
Hunk #7 succeeded at 1550 (offset -203 lines).
1 out of 7 hunks FAILED -- saving rejects to file plugins/views_plugin_display.inc.rej

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'.

dawehner’s picture

I made a patch against 3.x, of course. I will make a 2.x too.

Agileware’s picture

FYI,

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.

dawehner’s picture

Status: Needs work » Needs review

Your patch looks fine, too

Anonymous’s picture

The 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.

dawehner’s picture

@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.

prasannah.ganeshan’s picture

FileSize
85.32 KB

Hi,

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

coolhandlukek2’s picture

Subscribing - thx

n_nelson350’s picture

Have to check and test the patch

guntherdevisch’s picture

Nice patch! Thanks! It works great. Will it be in the next Views releases?

davepoon’s picture

I vote for this, sometimes it is useful to enter a custom URL rather than the default Views page link.

locomo’s picture

subscribe

dagmar’s picture

Version: 6.x-2.x-dev » 6.x-3.x-dev

Marked 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.

dagmar’s picture

FileSize
7.96 KB

Ok, 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:

       if ($path) {
-        $path = $this->view->get_url(NULL, $path);
+        if (empty($override_path) || (strpos($override_path, '!') === FALSE && strpos($override_path, '%') === FALSE)) {
+          $path = $this->view->get_url(NULL, $path);
+        }

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

dawehner’s picture

FileSize
8.13 KB

Here is a rerole against views 2.x

iamjon’s picture

drewschmaltz’s picture

First 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!

mstrelan’s picture

@merlinofchaos -

Possibly make a note in the 'more' link checkbox that the path to link to can be set in the link display section.

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?

brenes’s picture

Yes, 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

RavenHursT’s picture

Are 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.

dawehner’s picture

Please 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.

merlinofchaos’s picture

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?

Yes, 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.

merlinofchaos’s picture

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

Ok, 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.

dawehner’s picture

Status: Needs work » Needs review
FileSize
8.14 KB

* added the radio element
* Arguments:

      if ($this->get_option('link_display') == 'link_url' && $override_path = $this->get_option('link_url')) {
        $tokens = $this->get_arguments_tokens();
        $path = strtr($override_path, $tokens);
      }

Manually testing it works fine. The tokens are used.

Not sure whether views_plugin_display::get_link_display should be changed, too.

khosman’s picture

When 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?

samuelsov’s picture

In 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 :

- if (empty($override_path) || (strpos($override_path, '!') === FALSE && strpos($override_path, '%') === FALSE)) {
+ if (empty($override_path)) {
          $path = $this->view->get_url(NULL, $path);
        }
dagmar’s picture

Rerolled

LetUsBePrecise’s picture

This is awesome idea. Hope this gets implemented soon.

dealancer’s picture

subscribing

dagmar’s picture

FileSize
8.33 KB

Rerolled

merlinofchaos’s picture

Priority: Critical » Major

I 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.

Mozzy’s picture

Is there a patch for D7?

dawehner’s picture

Not yet

stevector’s picture

Do I need a patch like this to meet the case described in #1203262: Option to exclude arguments from URL in more link. ?

pcambra’s picture

Version: 6.x-3.x-dev » 7.x-3.x-dev
FileSize
8.14 KB

Here's a version for review for D7

franz’s picture

sub

obrienmd’s picture

sub

EclipseGc’s picture

FileSize
8.21 KB

patch for current head:

EclipseGc’s picture

Status: Needs review » Reviewed & tested by the community

updated the patch and tested it. Loving this.

dawehner’s picture

Status: Reviewed & tested by the community » Fixed

Yeah. 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

merlinofchaos’s picture

Status: Fixed » Active

Ok, 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

merlinofchaos’s picture

Okay, 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:


  function get_path() {
    if ($this->has_path()) {
      return $this->get_option('path');
    }

    $display_id = $this->get_link_display();
    if ($display_id && !empty($this->view->display[$display_id]) && is_object($this->view->display[$display_id]->handler)) {
      return $this->view->display[$display_id]->handler->get_path();
    }
  }

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:

  $view->skip_arguments_in_path = TRUE;

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.

mesr01’s picture

+1

cecemel’s picture

Hi

I am using git to apply the patch. When applying, I recieve the following error:

Checking patch plugins/views_plugin_display.inc...
warning: plugins/views_plugin_display.inc has type 100755, expected 100644
error: while searching for:
      'group_by' => array('group_by'),
      'query' => array('query'),
      'use_more' => array('use_more', 'use_more_always', 'use_more_text'),
      'link_display' => array('link_display'),

      // Force these to cascade properly.
      'style_plugin' => array('style_plugin', 'style_options', 'row_plugin', 'row_options'),

error: patch failed: plugins/views_plugin_display.inc:342
error: plugins/views_plugin_display.inc: patch does not apply

As I am new to this process, could someone explain me how i should solve this?
thank you

jwilson3’s picture

In 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?

merlinofchaos’s picture

You 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.

jwilson3’s picture

Thanks. 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. ;)

tar_inet’s picture

Version: 7.x-3.x-dev » 7.x-3.3
FileSize
3.58 KB

Current 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&param2=value2. If any key is present as a filter in current view it will be override in more link.

Encarte’s picture

Version: 7.x-3.3 » 7.x-3.x-dev
Status: Active » Needs work

@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).

tar_inet’s picture

Status: Needs work » Needs review
FileSize
3.38 KB

Thanks 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

tar_inet’s picture

dawehner’s picture

Status: Needs review » Needs work
+++ b/plugins/views_plugin_display.incundefined
@@ -1769,6 +1773,12 @@ class views_plugin_display extends views_plugin {
+		$form['link_url_query'] = array(

Please use spaces instead of a tab here, this kind of looks ugly

+++ b/plugins/views_plugin_display.incundefined
@@ -1769,6 +1773,12 @@ class views_plugin_display extends views_plugin {
+          '#description' => t('Custom key/value-pairs (without any URL-encoding) to append to the link. You must add it as param1=value1&amp;param2=value2.') . $output,

can't the code take care of the decoding?

+++ b/plugins/views_plugin_display.incundefined
@@ -2511,6 +2522,26 @@ class views_plugin_display extends views_plugin {
+          $queryFields = preg_split('/[&;]/', $this->get_option('link_url_query'));
+          $link_url_query = array();
+          foreach($queryFields as $fields) {
+            $keyvalue = preg_split('/=/', $fields);
+            if(isset($keyvalue['1'])) {
+              $link_url_query[$keyvalue['0']] = $keyvalue['1'];
+            }
+            else {
+              $link_url_query[$keyvalue['0']] = null;
+            }

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.

tar_inet’s picture

Ups, 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 (&param2~&paraSEMICOMAm2). 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.

tar_inet’s picture

Status: Needs work » Needs review
FileSize
3.04 KB

I had a problem with last attachment, I send it again in this comment

dawehner’s picture

views_handler_field does something like that:

      $query = drupal_get_query_array($url['query']);
      // Remove query parameters that were assigned a query string replacement
      // token for which there is no value available.
      foreach ($query as $param => $val) {
        if ($val == '%' . $param) {
          unset($query[$param]);
        }
      }
      $options['query'] = $query;

I'm wondering whether we could extract this into a reusable method.

chernetsky’s picture

#20: views-more-url_0.patch queued for re-testing.

rooby’s picture

I 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.

Esculap’s picture

I tried #77 patch. "!1 == ... input" token isn't applied to the custom query

Esculap’s picture

merlinofchaos’s picture

I think it would be better to use drupal_parse_url() to pull the query string and other goodies from the URL and preserve them.

troybthompson’s picture

Patch #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.

zterry95’s picture

#79 patch works fine for me. Thx.

gsivaprabu’s picture

Issue summary: View changes

I 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.

rv0’s picture

A 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.

gmclelland’s picture

rjacobs’s picture

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.

That'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)?

rooby’s picture

The 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.

rv0’s picture

@rooby
The whole idea in #84 is making it so that there aren't any settings needed.
this per module maintainer request in #80:

I think it would be better to use drupal_parse_url() to pull the query string and other goodies from the URL and preserve them.

rooby’s picture

Oh yep I see now. Sorry I glossed over it a bit quick.

Ludo.R’s picture

#84 works perfectly to me! :)

Tested with 7.x-3.11

stefan.r’s picture

Status: Needs review » Reviewed & tested by the community

#84 looks great

colan’s picture

We'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.

rv0’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
1.19 KB

as per #93, same patch as in #84 to activate the test bot

rv0’s picture

Status: Needs review » Reviewed & tested by the community

as per #93, setting back to "Reviewed & tested by the community"

  • colan committed f3c64f9 on 7.x-3.x authored by dawehner
    Issue #564106 by dawehner et al.: More link to point to custom URL
    
colan’s picture

Project: Views (for Drupal 7) » Drupal core
Version: 7.x-3.x-dev » 8.1.x-dev
Component: block displays » views.module
Status: Reviewed & tested by the community » Patch (to be ported)

Thanks!

rv0’s picture

@colan
Wrong patch attribution :(

colan’s picture

Just reverted & recommitted. Sorry about that! That's what I get for trusting Dreditor.

kellyimagined’s picture

Hope this helps to answer the original question: https://www.drupal.org/node/578834#comment-10847634

dawehner’s picture

To be honest the custom more link functionality is one of the things I would let people solve with templating instead of UI code.

dawehner’s picture

Priority: Major » Normal

I don't really think that this is a major issue

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

therealssj’s picture

Patch for 8.2.x

colan’s picture

Status: Patch (to be ported) » Needs review
larowlan’s picture

Title: More link to point to custom URL » More links pointing to custom URLs don't respect entered fragments and query parameters
Version: 8.2.x-dev » 8.1.x-dev
Category: Feature request » Bug report
Status: Needs review » Needs work
+++ b/core/modules/views/src/Plugin/views/display/DisplayPluginBase.php
@@ -2102,6 +2103,24 @@ public function renderMoreLink() {
+        $parsed_url = UrlHelper::parse($url);

$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.

larowlan’s picture

Status: Needs work » Needs review
Issue tags: +Needs tests
FileSize
1.43 KB
1.4 KB

this fixes the bug

dawehner’s picture

Thank you @larowlan for posting a patch on this issue.

+++ b/core/modules/views/src/Plugin/views/display/DisplayPluginBase.php
@@ -2097,6 +2098,22 @@ public function renderMoreLink() {
+        if (!empty($options['query'])) {
+          if (!empty($url_options['query'])) {
+            $url_options['query'] = array_merge($options['query'], $url_options['query']);
+          }
+          else {
+            $url_options['query'] = $options['query'];
+          }
+        }

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?

larowlan’s picture

yeah

larowlan--

such a lazy mongrel

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

damontgomery’s picture

Queued patch in #107 against 8.2.0

dawehner’s picture

Status: Needs review » Needs work

.

BarisW’s picture

Status: Needs work » Needs review
FileSize
1.13 KB

Yes we can!

realgt’s picture

i 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&amp;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.

@@ -2097,6 +2098,22 @@ public function renderMoreLink() {
         if (!empty($this->view->exposed_raw_input)) {
           $url_options['query'] = $this->view->exposed_raw_input;
         }
+
+        $options = $url->getOptions();
+        // Preserve the query string from url.
+        if (!empty($options['query'])) {
+          if (!empty($url_options['query'])) {
+            $url_options['query'] = array_merge($options['query'], $url_options['query']);
+          }
+          else {
+            $url_options['query'] = $options['query'];
+          }
+          $url_options['query'] = array_combine(str_replace('amp;','', array_keys($url_options['query'])), array_values($url_options['query']));
+        }
+        // Add fragment if applicable.
+        if (!empty($options['fragment'])) {
+          $url_options['fragment'] = $options['fragment'];
+        }
+

probably not the best way to solve it but it worked for me, thought i'd share

Status: Needs review » Needs work

The last submitted patch, 114: multiple_query_params-564106.fix_.patch, failed testing.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

shadcn’s picture

Version: 8.3.x-dev » 8.4.x-dev
Status: Needs work » Needs review
FileSize
2.29 KB

Adding some tests here to show some use cases:

  1. node - internal without leading slash - Expected to pass.
  2. /node - internal with leading slash - Expected to fail.
  3. http://drupal.org - external - Expected to fail.
dawehner’s picture

a) 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.

Status: Needs review » Needs work

The last submitted patch, 117: more_links_pointing_to-564106-117-fail.patch, failed testing.

shadcn’s picture

FileSize
88.37 KB

a) 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.

shadcn’s picture

Added tests for query paramaters.

Status: Needs review » Needs work

The last submitted patch, 121: more_links_pointing_to-564106-121-fail.patch, failed testing.

shadcn’s picture

Let's make them green now.

Status: Needs review » Needs work

The last submitted patch, 123: more_links_pointing_to-564106-123.patch, failed testing.

shadcn’s picture

Updated the assert messages. Let's try this again.

Status: Needs review » Needs work

The last submitted patch, 125: more_links_pointing_to-564106-125.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
  1. +++ b/core/modules/views/src/Plugin/views/display/DisplayPluginBase.php
    @@ -2072,7 +2073,7 @@ public function renderMoreLink() {
    -        $url = Url::fromUserInput('/' . $path);
    +        $url = UrlHelper::isExternal($path) ? Url::fromUri($path) : Url::fromUserInput('/' . ltrim($path, '/'));
    

    Do you mind opening up an issue to maybe provide a common method on Url to do exactly that?

  2. +++ b/core/modules/views/src/Plugin/views/display/DisplayPluginBase.php
    @@ -2086,6 +2087,13 @@ public function renderMoreLink() {
    +        // Preserve the query string from url.
    +        $options = $url->getOptions();
    +        $options += ['query' => []];
    +        $url_options += ['query' => []];
    +        $url_options['query'] = array_merge($options['query'], $url_options['query']);
    +
    

    We have $url->mergeOptions() now. You could leverage that here.

shadcn’s picture

Status: Needs review » Needs work
shadcn’s picture

Status: Needs work » Needs review
FileSize
4.45 KB
1020 bytes

Added $url->mergeOptions() .

shadcn’s picture

Assigned: Unassigned » shadcn
Status: Needs review » Needs work

Actually, we could use a few more tests here.

shadcn’s picture

OK added a few more tests.

  1. Test more link without leading slash.
  2. Test more link with leading slash.
  3. Test more link with absolute url.
  4. Test more link with query parameters in the url.
  5. Test more link with fragment in the url.
  6. Test more link with arguments / token replacements.

Status: Needs review » Needs work

The last submitted patch, 131: more_links_pointing_to-564106-131.patch, failed testing.

shadcn’s picture

Status: Needs work » Needs review
FileSize
7.56 KB
508 bytes

Let's try this again.

Status: Needs review » Needs work

The last submitted patch, 133: more_links_pointing_to-564106-133.patch, failed testing.

dawehner’s picture

+++ b/core/modules/views/src/Tests/Plugin/DisplayTest.php
@@ -257,6 +257,84 @@ public function testReadMoreNoDisplay() {
+    $link = $this->xpath('//a[@href=:url]', [':url' => '/node']);
...
+    $view->display_handler->setOption('link_url', '/node');
...
+    $link = $this->xpath('//a[@href=:url]', [':url' => '/node']);

You need to take into account that you are on subdir installation on the testbot. Better use $this->assertLink

shadcn’s picture

Status: Needs work » Needs review
FileSize
7.19 KB
3.26 KB

Updated. Thanks @dawehner.

dawehner’s picture

+++ b/core/modules/views/src/Plugin/views/display/DisplayPluginBase.php
@@ -2065,14 +2066,26 @@ public function renderPager() {
+      // If the user has supplied a custom "More" link path, use that for the URL.
+      if ($this->getOption('link_display') == 'custom_url' && $path = $this->getOption('link_url')) {
+        // Replace any argument tokens.
+        if ($tokens = $this->getArgumentsTokens()) {
+          // \Drupal\views\Plugin\views\PluginBase::viewsTokenReplace will
+          // replace '&' in the $path. In order to keep '&', we first replace it
+          // with a different temporary placeholder.
+          $query_replacements = 0;
+          $path = preg_replace("/&(?!amp;)/", "##", $path, -1, $query_replacements);
+          $path = $this->viewsTokenReplace($path, $tokens);
+
+          // Replace the temporary placeholder.
+          if ($query_replacements > 0) {
+            $path = str_replace('##', '&', $path);
+          }

IMHO the better approach might be to parse the url and then run the token function on the query and the main url separate

shadcn’s picture

Updated as per #137 and added a few more tests.

Pavan B S’s picture

+++ b/core/modules/views/src/Plugin/views/display/DisplayPluginBase.php
@@ -2065,14 +2066,33 @@ public function renderPager() {
+      // If the user has supplied a custom "More" link path, use that for the URL.

Line is exceeding 80 characters.
Applying the patch please review.

Status: Needs review » Needs work

The last submitted patch, 139: more_links_pointing_to-564106-139.patch, failed testing.

jofitz’s picture

Assigned: shadcn » Unassigned
Status: Needs work » Needs review
FileSize
8.31 KB

Re-rolled.

dawehner’s picture

  1. +++ b/core/modules/views/src/Plugin/views/display/DisplayPluginBase.php
    @@ -2065,14 +2066,34 @@ public function renderPager() {
    +        // Parse the $path.
    +        $parts = UrlHelper::parse($path);
    

    ... 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.

  2. +++ b/core/modules/views/src/Plugin/views/display/DisplayPluginBase.php
    @@ -2065,14 +2066,34 @@ public function renderPager() {
    +        if ($tokens = $this->getArgumentsTokens()) {
    +          // Replace any argument tokens in path.
    +          $parts['path'] = $this->viewsTokenReplace($parts['path'], $tokens);
    

    If there a reason we have this if here? As far as I remember there are more arguments in viewsTokenReplace than just the getArgumentsTokens

  3. +++ b/core/modules/views/src/Tests/Plugin/DisplayTest.php
    @@ -257,6 +257,96 @@ public function testReadMoreNoDisplay() {
       /**
    +   * Tests the readmore with custom url.
    +   */
    +  public function testReadMoreCustomURL() {
    +    /** @var \Drupal\Core\Render\RendererInterface $renderer */
    +    $renderer = $this->container->get('renderer');
    

    Do 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 :(

sukanya.ramakrishnan’s picture

Folks,

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

sukanya.ramakrishnan’s picture

Submitting a reroll of patch in 143 for drupal 8.2

jhedstrom’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll

Patch no longer applies (I think some views tests were converted to phpunit.)

error: core/modules/views/src/Tests/Plugin/DisplayTest.php: does not exist in index
dawehner’s picture

Status: Needs work » Needs review
FileSize
8.61 KB

Just a quick reroll

Status: Needs review » Needs work

The last submitted patch, 146: 564106-146.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

pk188’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
8.59 KB

Re rolled.

Status: Needs review » Needs work

The last submitted patch, 148: views-more-link-564106-148.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

dragos-dumi’s picture

This part will strip multidimensional array (as example using a more link to a search with facets f[0], f[1] etc)

+              if (is_array($value)) {
+                $key = array_keys($value)[0];
+                $parts['query'][$name] = [$key => $this->viewsTokenReplace($value[$key], $tokens)];
+              }

other than this, it seems to be working fine. thx.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

sukanya.ramakrishnan’s picture

@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

sukanya.ramakrishnan’s picture

Status: Needs work » Needs review
sukanya.ramakrishnan’s picture

Submitting a patch after fixing failing tests and adding a comment to explain the code for which concern was raised in #150.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

pookmish’s picture

Status: Needs review » Reviewed & tested by the community

#154 works great on my side.

dragos-dumi’s picture

In 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

dragos-dumi’s picture

The last submitted patch, 158: views-more-link-564106-158-testonly.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

dragos-dumi’s picture

dagmar’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: -Needs tests

Thanks! There are 6 nested ifs in that function. There is probably a better way to implement the same logic.

Dinesh18’s picture

Assigned: Unassigned » Dinesh18
Dinesh18’s picture

Status: Needs work » Needs review
FileSize
3.7 KB
9.57 KB

Here is the updated patch and interdiff.txt

Dinesh18’s picture

Assigned: Dinesh18 » Unassigned

Status: Needs review » Needs work

The last submitted patch, 164: 564106-164.patch, failed testing. View results

wesleydv’s picture

I refactored #161 to avoid all those nested ifs.

gnuget’s picture

Status: Needs review » Needs work
Issue tags: +Novice

Thanks for the patch I gave it a review and it is almost ready just a couple nits:

  1. +  /**
    +    * Get the more url for this view.
    +    * ...
    +    */
    
    + /**
    +   * Replace the tokens in the given path.
    +   * ...
    +   */
    

    The summary lines must start with a third person singular present tense verb So it should be Replaces and Gets

  2. +    // Handle query parameters where the key in the form of array.
    +    // for example, f[0] for facets.
    

    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!

dawehner’s picture

This looks pretty good for me. Thank you for documenting the usecase of facets!

dawehner’s picture

This looks pretty good for me. Thank you for documenting the usecase of facets!

ilya.no’s picture

Attaching patch with fixes for #168 comment and also changed one line, because patch couldn't be applied.

ilya.no’s picture

Status: Needs work » Needs review
gnuget’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
1.24 KB

Yes #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!

gnuget’s picture

Issue tags: -Novice

Status: Reviewed & tested by the community » Needs work
gnuget’s picture

Status: Needs work » Reviewed & tested by the community

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

ragnarkurm’s picture

Does 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:

      if (!empty($this->view->exposed_raw_input)) {
        $url->mergeOptions(['query' => $this->view->exposed_raw_input]);
        return [
          '#type' => 'more_link',
          '#url' => $url,
          '#title' => $this->useMoreText(),
          '#view' => $this->view,
        ];
      }

My questions are:

  1. Why should a random page I'm linking to, get query parameters of a current page others than I have specified?
  2. Why should showing more link depend on current page having exposed filters? If my current view does not expose query parameters, then more link is not displayed at all, regardless of 'Always display the more link'.

(The patch does not apply to 8.5.5, probably never intended to).

gnuget’s picture

Hi ragnarkurm.

Where is that code?

What I got after applying #171 is:

     // Merge the exposed query parameters.
      if (!empty($this->view->exposed_raw_input)) {
        $url->mergeOptions(['query' => $this->view->exposed_raw_input]);
      }

      return [
        '#type' => 'more_link',
        '#url' => $url,
        '#title' => $this->useMoreText(),
        '#view' => $this->view,
      ];

David.

Status: Reviewed & tested by the community » Needs work
gnuget’s picture

Status: Needs work » Reviewed & tested by the community

Random testbot fail.

Status: Reviewed & tested by the community » Needs work
gnuget’s picture

Status: Needs work » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work
gnuget’s picture

Status: Needs work » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work
gnuget’s picture

Status: Needs work » Reviewed & tested by the community
charly71’s picture

Hi,
someone can tell if #171 patch works with Drupal 8.6.x?
Thanks

gnuget’s picture

I'm using it with Drupal 8.6.2 and it is working as expected.

Status: Reviewed & tested by the community » Needs work
neetu morwani’s picture

Reroll patch for 167th patch to make it compatible with latest code for 8.6.x branch.

caspervoogt’s picture

the 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.

gnuget’s picture

Status: Needs work » Reviewed & tested by the community

And the tests passed.

So let's switch back this to RTBC :-)

Thanks.

Status: Reviewed & tested by the community » Needs work
gnuget’s picture

why this fails so frequently?

:-(

Krzysztof Domański’s picture

Status: Needs work » Reviewed & tested by the community

Unrelated test failure. Back to RTBC.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ w/core/modules/views/src/Plugin/views/display/DisplayPluginBase.php
    @@ -2071,38 +2072,81 @@ public function renderPager() {
    +      && $url = $this->getMoreUrl()) {
    ...
    +   * @return \Drupal\Core\Url
    +   *   The more link as Url object.
    

    I'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.

  2. +++ w/core/modules/views/src/Plugin/views/display/DisplayPluginBase.php
    @@ -2071,38 +2072,81 @@ public function renderPager() {
    +      // Merge the exposed query parameters.
    +      if (!empty($this->view->exposed_raw_input)) {
    +        $url->mergeOptions(['query' => $this->view->exposed_raw_input]);
           }
    

    Should this not be part of the getMoreUrl() method?

  3. +++ w/core/modules/views/src/Plugin/views/display/DisplayPluginBase.php
    @@ -2071,38 +2072,81 @@ public function renderPager() {
    +    $options = $this->replacePathTokens($path);
    ...
    +  /**
    +   * Replace the tokens in the given path.
    +   *
    +   * @param string $path
    +   *   The internal path or external URL string to replace the tokens in.
    +   *
    +   * @return array
    +   *   The parsed given path with its tokens replaced.
    +   *
    +   * @see UrlHelper::parse()
    +   */
    +  protected function replacePathTokens($path) {
    

    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.

larowlan’s picture

Status: Needs work » Needs review
FileSize
3.5 KB
10.07 KB

addresses those changes

kim.pepper’s picture

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

PieterDC’s picture

Thanks @larowlan for your patch. It seems to work.
And thanks to everyone else involved getting to this point.

hanness’s picture

#198 works for me as well, many thanks everyone!

I'm not a coder, so only TBC, no RTBC from my side.

volkswagenchick’s picture

Issue tags: +drupalnorth2019

Tagging for DrupalNorth 2019

gnuget’s picture

Status: Needs review » Reviewed & tested by the community

I re-reviewed the patch and it looks great! (and I just ran again the tests and all passed) so I think this is RTBC

  • webchick committed e03b422 on 8.8.x
    Issue #564106 by arshadcn, dawehner, tar_inet, sukanya.ramakrishnan,...
webchick’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: +8.8.0 highlights

This 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.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.