This is a follow-on to #228510: Exposed Sorts. The exposed sorts do not properly default; all sorts appear. It can appear to work right, because the first sort in the list is going to appear in the sort gadget, but if that value has a lot of the same data (as is often the case when using devel generated data and dates) the secondary sorting will be wrong and provide inconsistent results when you hit apply.

CommentFileSizeAuthor
#27 exposed_sorts_do_not_properly_default-666870-27.patch733 bytesa.ross
#26 666870-26.patch553 bytesjody lynn
#16 666870-views-exposed_sorts-2.patch1.31 KBkenorb
#14 views_plugin_exposed_form.inc-666870.patch1.08 KBkenorb
#6 666870-6.views-exposed-sorts.patch1.64 KBdww
#1 666870-views-exposed_sorts.patch941 bytesdawehner

Comments

dawehner’s picture

dawehner
Primary language German
commented
Status: Active » Needs review
StatusFileSize
new 666870-views-exposed_sorts.patch941 bytes

i got it

dww’s picture

dww
we/he/they
commented
Status: Needs review » Reviewed & tested by the community

Somehow I didn't find this while searching, and I just submitted a duplicate of this #800752: Exposed sort order dropdown doesn't reflect the active default order *doh*!

I just tried it and this patch definitely fixes the bug I was talking about. I can't say I grok exposed sorts yet, but a quick visual skim of the patch looks exactly like what I proposed at #800752. Small patch that I can confirm solves the bug I saw. I'll let the Views3 crew actually commit, but this looks RTBC to me.

Thanks!
-Derek

p.s. Shouldn't we add an "exposed sorts" component?

dawehner’s picture

dawehner
Primary language German
commented

I would like to have a exposed form component, there might be issues with other stuff, like exposed "items per page".

Would it be possible to rename a component?

PS:
Only earl commits to drupal-6--3

dww’s picture

dww
we/he/they
commented

Wrong place for this discussion, but...

"Would it be possible to rename a component?"

#140989: Renaming components is unsafe -- still not there yet... :(

dww’s picture

dww
we/he/they
commented
Assigned: Unassigned » dww
Status: Reviewed & tested by the community » Needs work

Drat, I'm sorry to report that this patch appears to break the exposed sort order UI. :( While it defaults to the right thing with the patch, if you change the order in the UI and submit, the changed order is ignored and the query ends up always sorting DESC.

The combo of these two bugs is actively nailing me right now, so I'm going to debug a little more closely, stay tuned.

dww’s picture

dww
we/he/they
commented
Status: Needs work » Needs review
StatusFileSize
new 666870-6.views-exposed-sorts.patch1.64 KB

Heh. Yeah, this was missing a critical piece. We were simply ignoring the exposed form value unless it was in (asc, desc), but the rest of the code was using ASC and DESC.

This seems like a fragile aspect of exposed sorts, since it varies across the views codebase whether we use asc vs. ASC.

The attached patch actually works in the case of the exposed sorts I'm dealing with, which is what I need to get working.

However, it'd probably be worth a more far-reaching review and plan around this problem, since I fear similar bugs will emerge in the future if we don't do something systematic.

rburgundy’s picture

rburgundy commented

subscribe

Bilmar’s picture

Bilmar commented
Status: Needs review » Reviewed & tested by the community

Applied the patch and it fixed the issue with Exposed Sort Order not properly defaulting to Asc or Desc.
I came across this issue when testing #657148: Add support for Exposed Sort for BEF module.

merlinofchaos’s picture

merlinofchaos commented
Status: Reviewed & tested by the community » Fixed

Applied to both -dev branches.

I agree that we should consider finding something more systematic to prevent the case issue, but that is kind of a difficult problem. This fix will have to do for now.

Status: Fixed » Closed (fixed)

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

scotjam’s picture

scotjam commented
Assigned: dww » Unassigned
Category: bug » support
Status: Closed (fixed) » Active

Unfortunately, I can't get this working.

I have two different sort criteria, both exposed for the user to select. Each sort criteria has a different default sort order. But, when the user selects a sort criteria the sort order is not updated to its pre-set default.

I'm I wrong in assuming that this fix should update the exposed-drop-down-selection-sort-order to reflect the default sort order of the sort criteria? I could have got the wrong end of the stick!

Hope someone can help!
cheers
scotjam

scotjam’s picture

scotjam commented

Found how to get around it...

unset($form['sort_order']);

placed in a hook_form_alter.

That way, the sort_order doesn't get picked up, instead the default sort_order does.

Hope this helps
scotjam

kenorb’s picture

kenorb commented 
        '#default_value' => $first_sort->options['order'],

Why we copying default order from the first sort?
What if we have more than one?

Marked as duplicate:
#1000160: Order of second exposed sort is copied from the first one when manually applying sort_by=title into URI

kenorb’s picture

kenorb commented
Category: support » bug
Status: Active » Needs review
StatusFileSize
new views_plugin_exposed_form.inc-666870.patch1.08 KB

Patch in attachment.

Problem described here:
#1000160: Order of second exposed sort is copied from the first one when manually applying sort_by=title into URI

merlinofchaos’s picture

merlinofchaos commented
Status: Needs review » Needs work 
+      $input_order = $form_state['input']['sort_order'] ? $form_state['input']['sort_order'] : $this->view->sort[$form_state['input']['sort_by']];

This looks like it's going to produce an inconsistent variable. i.e, if $form_state['input']['sort_order'] exists, it will be a string. If it does not exist, it will be a sort handler.

-        '#default_value' => $first_sort->options['order'],
+        '#default_value' => $input_order ? $input_order->options['order'] : $first_sort->options['order'],

That means if $input_order existed in the input, then $input_order->options['order'] is going to be broken because it's a string, not a class.

Plus, we now have a $first_sort even though we didn't really need one.

Ultimately, if $form_state['input']['sort_order'] is actually set, our default_value doesn't really matter a whit because it'll be overridden by the input, so there is no point in checking that. What we care about is if sort_by is set and exists, in which case we can go ahead and use its value as our default.

  if (isset($form_state['input']['sort_by']) && isset($this->view->sort[$form_state['input']['sort_by']]) {
    $default_sort_order = $this->view->sort[$form_state['input']['sort_by']]->options['order'];
  }
  else {
    $first_sort = reset($this->view->sort);
    $default_sort_order = $first_sort->options['order'];
  }

And then '#default_value' is just $default_sort_order.

Does that do what you want?

kenorb’s picture

kenorb commented
Status: Needs work » Needs review
StatusFileSize
new 666870-views-exposed_sorts-2.patch1.31 KB

Yes, that's correct. Tested and it does work in the same way.

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs review » Needs work

Sorry.

It would be cool to fix the codestyle first


+      } else {

It should be have an extra line.

vistree’s picture

vistree commented

Hi, I am not sure if my problem is the same as described here.
I added several exposed sorts. The first is descending. If I open the view, sorting is as aspected.
Now, I have a second exposed sort. This time it is ascending.
Looking at the view, both columns are sorted correctly.
But, if I change the sortfild in the dropdown (header of view) to the second value, the sorting is wrong. Instead of sorting the column asc (as set in the exposed filter), it is sorted descending (as the first column).

Can I fix this by applying the above patch?

dawehner’s picture

dawehner
Primary language German
commented

Just try out the patch.

dawehner’s picture

dawehner
Primary language German
commented

Based on #1061014: Exposed sort ignores sorting set for each field #18 requests a new feature which will not be fixed with this patch.

dawehner’s picture

dawehner
Primary language German
commented

Perhaps you should ignore me and just try out the patch :)

vistree’s picture

vistree commented

Hi, thank you for your reply.
I applied the patch from #16, but there is no diffence afterwards. Do I have to modify the patch somehow? Is there something in #17 I have to apply?

Drupi’s picture

Drupi commented
Version: 6.x-3.x-dev » 7.x-3.x-dev

The same bug seems to be in Drupal 7, too.
Can anyone provide a patch similar to patch #16 for the current 7.x-3.x-dev version, that addresses the problem? That would be great!

dawehner’s picture

dawehner
Primary language German
commented
Version: 7.x-3.x-dev » 6.x-3.x-dev

This pach works for 7.x, too

Drupi’s picture

Drupi commented

Thanks for your quick response, dereine. Unfortunately, I'm having the same problem as vistree and the patch doesn't work here either.

Do you have any ideas?

jody lynn’s picture

jody lynn
she/her
Primary language English
commented
Version: 6.x-3.x-dev » 7.x-3.x-dev
Status: Needs work » Needs review
StatusFileSize
new 666870-26.patch553 bytes

The patch in #16 looks to be already applied. However it fixed the default sort_by but not the default sort_order.

Currently if you have multiple exposed sorts it's impossible to have a default sort. I would expect it to default to the first sort since that is what it shows in the UI.

This patch adds the first sort as the default sort.

a.ross’s picture

a.ross commented
StatusFileSize
new exposed_sorts_do_not_properly_default-666870-27.patch733 bytes

I'm not sure if I grok #26, but I'm also having an issue with the default sort order of exposed filters. The default sort order is incorrect when the exposed filter is below a regular filter (like "node: sticky"). It in fact uses the sort order for the first filter, even if that isn't an exposed filter.

Media Crumb’s picture

Media Crumb commented

Do patch 26 and 27 fix this issue?

ryan.ryan’s picture

ryan.ryan commented
Issue summary: View changes

Thanks Jody, exactly what I needed. :) #26 is what I needed.

dobe’s picture

dobe commented
Status: Needs review » Closed (duplicate)
Parent issue: » #2037469: Exposed Sort By and Sort Order view pane settings not retained

This patch is relevant but seems to be kinda old. I re-rolled a patch at https://www.drupal.org/node/2037469#comment-11023513

So I am going to mark as duplicate and hopefully we can get the other one pushed.

Thanks to everyone who has contributed to this issue. I have requested that all involved here get credit for the other issues queue.