[wp-trac] [WordPress Trac] #64467: Grid view displays attachments in the wrong order when an order query var is present but not normalized

[WordPress Trac]

#64467: Grid view displays attachments in the wrong order when an order query var
is present but not normalized
-------------------------------------+-------------------------------------
 Reporter:  trivedikavit             |       Owner:  (none)
     Type:  defect (bug)             |      Status:  new
 Priority:  normal                   |   Milestone:  7.0
Component:  Media                    |     Version:
 Severity:  minor                    |  Resolution:
 Keywords:  has-screenshots          |     Focuses:  javascript,
  changes-requested                  |  administration
-------------------------------------+-------------------------------------

Comment (by trivedikavit):

 After digging further into the `Query` and `Attachments` models (thanks
 @sabernhardt for pointing those out as sources), I think there is a more
 fundamental root cause here and a cleaner fix than normalizing order at
 individual call sites.

 == What changed in my understanding ==
 The initial patch (uppercasing order inside `filters.order` /
 `comparator`, and defaulting to `DESC` unless `ASC`) is effective, but it
 treats the symptom: it fixes only the specific places where those methods
 read `this.props.get( 'order' )`.

 On closer inspection, the underlying issue is that `this.props` is not
 initialized consistently across different collection types / instantiation
 paths. As a result, `this.props.get( 'order' )` can contain different
 values depending on whether the collection is a `Query` instance or a
 plain `Attachments` instance.

 == The underlying inconsistency ==
 ==== `wp.media.model.Query.get( props )` normalizes order ====
 In `Query.get()` the code:

 * uppercases `props.order`
 * validates it against `ASC`/`DESC`
 * defaults invalid values back to the default (`DESC`)

 So any `Query` created via `Query.get()` ends up with normalized
 `props.order`.

 {{{
 // Normalize the order.
 props.order = props.order.toUpperCase();
 if ( 'DESC' !== props.order && 'ASC' !== props.order ) {
     props.order = defaults.order.toUpperCase();
 }
 }}}
 Reference:
 https://core.trac.wordpress.org/browser/trunk/src/js/media/models/query.js?rev=51632&marks=40#L254

 This is also done via `_requery` here, where if the collection is a query,
 it would create and mirror a `Query` collection:
 https://core.trac.wordpress.org/browser/trunk/src/js/media/models/attachments.js?rev=51191&marks=497,514#L443

 ==== `wp.media.model.Attachments.initialize()` does not normalize order
 ====

 In `Attachments.initialize( models, options )`, `options.props` is applied
 directly via:
 {{{
 this.props.set( _.defaults( options.props || {} ) );
 }}}
 Reference:
 https://core.trac.wordpress.org/browser/trunk/src/js/media/models/attachments.js?rev=51191&marks=497,514#L47

 If `options.props.order` is passed in as lowercase (e.g. 'desc') or
 invalid, it can remain unnormalized in `this.props`.

 Because `Query` inherits from `Attachments`, we end up with different
 `this.props` states depending on how the collection is constructed:

 * A `Query` instance created via `Query.get()` will have normalized
 `this.props.get( 'order' )`
 * An `Attachments` instance constructed with raw `options.props.order` may
 not have normalized `this.props.get( 'order' )`

 **This inconsistency becomes visible as a bug whenever downstream UI logic
 performs case-sensitive comparisons against `DESC` / `ASC` (e.g. the
 comparator branching on `'DESC' === order`), which can lead to the Grid UI
 effectively sorting in the opposite direction from what the server
 returned.**


 == How I confirmed it ==
 I added a temporary log inside the comparator to inspect the instance type
 and the order value:

 {{{
 console.log( 'comparator in', this instanceof wp.media.model.Query ?
 'Query' : 'Attachments', 'with order:', order );
 }}}

 For example, visiting `/wp-
 admin/upload.php?mode=grid&orderby=date&order=desc` yields:

 {{{
 comparator in Query with order: DESC
 comparator in Attachments with order: desc
 }}}

 == Proposed fix: normalize once at initialization ==
 Instead of normalizing in every method that consumes `props.order`,
 normalize at the point where props are first applied in
 `Attachments.initialize()` (i.e. normalize `options.props.order` before
 setting it on this.props):

 {{{
 options.props = options.props || {};

 if ( 'string' === typeof options.props.order ) {
     options.props.order = options.props.order.toUpperCase();
     if ( 'ASC' !== options.props.order && 'DESC' !== options.props.order )
 {
         options.props.order = 'DESC';
     }
 }

 this.props.set( options.props );
 }}}

 Reference (replace this line):
 https://core.trac.wordpress.org/browser/trunk/src/js/media/models/attachments.js?rev=51191&marks=497,514#L47

 == Testing ==

 Testing with the fix in place does lead to consistent normalized `order`:

 {{{
 comparator in Query with order: DESC
 comparator in Attachments with order: DESC
 }}}

 == Why this is better ==

 * '''Fixes the root cause''': `this.props` starts out normalized
 regardless of whether the collection is `Query` or `Attachments`.
 * '''More maintainable''': avoids having to normalize order everywhere
 `this.props.get( 'order' )` is referenced.
 * '''Inheritance-proof''': subclasses inheriting
 `Attachments.initialize()` get consistent normalization automatically.

 ----

 Regarding the `menuOrder`:

 This proposed change preserves existing behavior for valid `ASC`/`DESC`
 values and improves reliability for `menuOrder` (which already relies on
 `ASC` being uppercase). It only changes behavior for previously
 unnormalized inputs (e.g. `asc`, `desc`, invalid strings), aligning
 `Attachments` initialization with `Query.get()`’s existing
 normalization/defaulting.

 ----

 @adamsilverstein Let me know if this looks good.

-- 
Ticket URL: <https://core.trac.wordpress.org/ticket/64467#comment:5>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform