Skip to:
Content

BuddyPress.org

Opened 8 months ago

Closed 8 months ago

Last modified 7 months ago

#8623 closed defect (bug) (fixed)

Profile Fields "Custom" Sort Order not working since 10.0

Reported by: niftythree's profile niftythree Owned by: imath's profile imath
Milestone: 10.1.0 Priority: normal
Severity: normal Version: 10.0.0
Component: Core Keywords: has-patch commit
Cc:

Description

Hello,

Since updating to BuddyPress 10.0, the "Custom" sort order for all profile fields is no longer working.

On the backend when editing a profile field, the options will be listed in the chosen order. However, on the front end (and on the backend, when not editing) the options always appear to be sorted in ascending order, even though "Ascending" is not selected. Changing the "sort order" to "Descending" works as it should, but "Custom" doesn't work; the options will always be displayed in ascending order.

Any chance this could be addressed soon, please? πŸ™‚

Thanks.

Attachments (2)

8623.patch​ (939 bytes) - added by oztaser 8 months ago.
8623.2.patch​ (1.5 KB) - added by imath 8 months ago.

Download all attachments as: .zip

Change History (16)

#1 @niftythree
8 months ago

Should have mentioned that we're using the legacy template.

Thanks.

#2 @oztaser
8 months ago

  • Keywords has-patch added
  • Milestone changed from Awaiting Review to 10.1.0

Hi @niftythree,

I was able to reproduce this problem. I think we can simply add a condition to check if order is custom.

@oztaser
8 months ago

#3 @espellcaste
8 months ago

This regression was introduced by me at ​https://buddypress.trac.wordpress.org/changeset/13147 I was actually not aware that you could have custom sort options there. However, the sorting defaults to ASC, even if they are custom, so I’m not sure what custom means in that scenario.

This ticket was mentioned in ​Slack in #buddypress by imath. ​View the logs.


8 months ago

#5 @espellcaste
8 months ago

@oztaser I uploaded another patch to revert the regression. Your patch looked good, but it could still add issues in case someone else used a different custom order_by slug.

Last edited 8 months ago by espellcaste (previous) (diff)

#6 @oztaser
8 months ago

Hi @espellcaste,

You are totally right and your patch looks great to me. Thanks for the improvement.

@imath
8 months ago

#7 @imath
8 months ago

  • Keywords commit added

Sorry @oztaser I was so tired yesterday I haven't noticed you submitted a patch 😞.

@espellcaste your patch doesn't fix the issue, because bp_esc_sql_order() returns "ASC" or "DESC", so it's always one of these into your statement and never "custom" 😬.

Many thanks to you both for your work on it, it helped me understand what was going wrong, I was looking at fields order, although it was options order...

If no objections, let's commit this in 10.0 branch and trunk.

Last edited 8 months ago by imath (previous) (diff)

#8 @oztaser
8 months ago

Thanks @imath, that's what I tried to explain in Slack thread and fix in my patch. I don't know how I've reviewed 8623.diff and how I missed the main point 🀦. We are lucky to have you! Thanks again.

#9 @espellcaste
8 months ago

@imath

Both patches (actually all patches provided here) accomplsh the same thing, just differently: it defaults to ASC for custom options.

My patch does not remove the custom key or skip it. It defaults to ASC just like yours do. If you check $this->order_by before or after sanitizing it makes no difference.

#10 @imath
8 months ago

@espellcaste

Sorry to insist but when I’ve tested yours the first condition was always true with the custom order. So it was always sorted by name and not by order_option.

'ASC' === bp_esc_sql_order( β€˜custom’ )

is the same as β€˜ASC’ === Β΄ASC’

😊

#11 @espellcaste
8 months ago

@imath Thank you for the insistance. You are most certainly correct. Upon a closer review, my patch is the one that does not work.

#12 @imath
8 months ago

In 13230:

Bring back custom order to xProfile field options sorting

r13147 introduced a regression about this type of xProfile field options order. To make sure we preserve it, we need to check $this->order_by value before sanitizing it otherwise the potential custom value is sanitized to ASC and the SQL sort part is never set to use the option_order db field.

Props oztaser, espellcaste, niftythree

See #8623 (branch 10.0)

#13 @imath
8 months ago

  • Owner set to imath
  • Resolution set to fixed
  • Status changed from new to closed

In 13231:

Bring back custom order to xProfile field options sorting

r13147 introduced a regression about this type of xProfile field options order. To make sure we preserve it, we need to check $this->order_by value before sanitizing it otherwise the potential custom value is sanitized to ASC and the SQL sort part is never set to use the option_order db field.

Props oztaser, espellcaste, niftythree

Fixes #8623 (trunk)

This ticket was mentioned in ​Slack in #buddypress by imath. ​View the logs.


7 months ago

Note: See TracTickets for help on using tickets.