Add media order normalization add tests for https://github.com/WordPress/wordpress-develop/pull/10680 - #10682
Conversation
Add order normalization in the Attachments model initialize method to ensure the 'order' property is always 'ASC' or 'DESC' (defaulting to 'DESC' for invalid values). This provides consistent behavior across all attachment collections. Part of fixing Media Library Grid view ordering when order query var is not normalized. See #64467.
Since Query inherits from Attachments, and Attachments now normalizes the order property in its initialize method, remove the duplicate normalization code from Query.get() to avoid redundancy and ensure consistent behavior. Part of fixing Media Library Grid view ordering when order query var is not normalized. See #64467.
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Core Committers: Use this line as a base for the props when committing in SVN: To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Test using WordPress PlaygroundThe changes in this pull request can previewed and tested using a WordPress Playground instance. WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser. Some things to be aware of
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
mukeshpanchal27
left a comment
There was a problem hiding this comment.
Thanks for the PR. Left some suggestion.
| this.props.on( 'change:query', this._changeQuery, this ); | ||
|
|
||
| this.props.set( _.defaults( options.props || {} ) ); | ||
| options.props = _.defaults( options.props || {} ); |
There was a problem hiding this comment.
_.defaults() only adds properties from the source object if they don't exist in the target. Since options.props || {} creates a fresh object. Direct assignment is cleaner.
There was a problem hiding this comment.
Note: this code was from #10680.
Is your suggestion to leave this line change off?
There was a problem hiding this comment.
Thanks @mukeshpanchal27 - switched to the direct assignment.
Co-authored-by: Mukesh Panchal <mukeshpanchal27@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR adds QUnit test coverage for Media Library attachment order normalization and consolidates normalization behavior by moving it into wp.media.model.Attachments (with wp.media.model.Query inheriting it), aligning with the fix described in referenced PR #10680.
Changes:
- Normalize
options.props.orderinsrc/js/media/models/attachments.jsand remove duplicate normalization fromsrc/js/media/models/query.js. - Add a new QUnit suite to validate normalization behavior across
AttachmentsandQuery. - Register the new QUnit test file in the QUnit runner HTML.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| tests/qunit/wp-includes/js/media/test-media-models.js | Adds QUnit tests covering order normalization and inheritance behavior. |
| tests/qunit/index.html | Includes the new media model test script in the QUnit test runner. |
| src/js/media/models/query.js | Removes now-redundant order normalization in Query.get() (expects inheritance from Attachments). |
| src/js/media/models/attachments.js | Adds centralized order normalization during Attachments.initialize(). |
Suppressed comments (7)
tests/qunit/wp-includes/js/media/test-media-models.js:119
- Like the
nullcase, explicitly passingorder: undefinedshould be treated as invalid and coerced to'DESC'to avoid truthy/non-string values influencing sort direction unexpectedly.
QUnit.test( 'Attachments should not process undefined order value', function( assert ) {
var collection = new wp.media.model.Attachments( [], {
props: {
order: undefined
}
tests/qunit/wp-includes/js/media/test-media-models.js:130
- Numeric
ordervalues should be treated as invalid and defaulted to'DESC'; otherwise they can be interpreted as ascending due to non-'DESC'truthy values.
QUnit.test( 'Attachments should not process numeric order value', function( assert ) {
var collection = new wp.media.model.Attachments( [], {
props: {
order: 123
}
tests/qunit/wp-includes/js/media/test-media-models.js:141
- Boolean
ordervalues should be coerced to'DESC'(invalid input). Preservingtruein particular would be treated as ascending by the comparator, which is not intended.
QUnit.test( 'Attachments should not process boolean true order value', function( assert ) {
var collection = new wp.media.model.Attachments( [], {
props: {
order: true
}
tests/qunit/wp-includes/js/media/test-media-models.js:152
- Boolean
ordervalues should be coerced to'DESC'(invalid input). Keepingfalseas-is is ambiguous and differs from how invalid values are handled elsewhere (e.g., WP_Query::parse_order()).
QUnit.test( 'Attachments should not process boolean false order value', function( assert ) {
var collection = new wp.media.model.Attachments( [], {
props: {
order: false
}
tests/qunit/wp-includes/js/media/test-media-models.js:164
- Object
ordervalues should be treated as invalid and coerced to'DESC'; otherwise any truthy object value will be interpreted as ascending by the comparator.
QUnit.test( 'Attachments should not process object order value', function( assert ) {
var orderObj = { value: 'ASC' };
var collection = new wp.media.model.Attachments( [], {
props: {
order: orderObj
}
tests/qunit/wp-includes/js/media/test-media-models.js:176
- Array
ordervalues should be treated as invalid and coerced to'DESC'; preserving an array would be truthy and therefore interpreted as ascending by the comparator.
QUnit.test( 'Attachments should not process array order value', function( assert ) {
var orderArray = ['ASC', 'DESC'];
var collection = new wp.media.model.Attachments( [], {
props: {
order: orderArray
}
tests/qunit/wp-includes/js/media/test-media-models.js:216
- Same as the previous Query test: avoid
props.query = true(it triggers Attachments mirroring), and pass anargsobject to better reflect how Query instances are constructed in production.
QUnit.test( 'Query should default invalid order to "DESC"', function( assert ) {
var query = new wp.media.model.Query( [], {
props: {
order: 'random',
query: true
}
});
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The `typeof` guard let truthy non-string order values through unnormalized. `Attachments.comparator()` and Query's order filter both test for the literal 'ASC'/'DESC', so `order: true` or `order: 123` sorted ascending instead of falling back to DESC - the same class of bug this change set out to fix. Coerce with `String()` and normalize whenever order is set, leaving null and undefined alone so the existing comparator fallback still applies. Also drops the `_.defaults()` call, which returns its argument unchanged when given no sources, and restores `var` since nothing else in src/js/media uses block scoping. The Query tests set `props.query`, which fires `_requery()` and a real server request; they now pass `args` to match what `Query.get()` builds.
|
Pushed updates for the feedback here, thanks @mukeshpanchal27. The one that mattered was the Copilot catch on non-string values. The Also in this push:
|
|
Thanks for testing @shail-mehta! I also verified this works correctly locally. |


Add: tests for #10680
Commit message
Trac ticket: https://core.trac.wordpress.org/ticket/64467