Skip to content

Conversation

@godismyjudge95
Copy link
Contributor

When adding fields to an existing blueprint, the newly created fields will have a value of null. When ordering by the newly created field you'd expect those without values would be placed after those with values. This PR changes the comparison between nulls and non-nulls so that nulls are placed after non-nulls.

I realize this is a large change in that it could affect a lot, but I believe the way this functions now is not desired or expected behavior.


Example where this matters:

  • Create a new collection "Posts"
  • Create a few entries in the collection
  • Add a toggle field "Featured Post"
  • Toggle the field on an older post
  • Retrieve the entries ordering by the "Featured Post" field:
Entry::query()
  ->where('collection', 'posts')
  ->where('published', true)
  ->orderBy('featured_post', 'desc')
  ->first();
  • Notice you will always get the posts where the featured post field is set to null because they are considered a greater value

Also in favor of this change, if you create an array in PHP and sort it you get the expected orders - with null coming after non-null:

$values = [false, true, null, 0, 1, '1', 0];

sort($values);
// $values = [false, null, 0, "0", true, 1, "1"];

rsort($values);
// $values = [true, 1, "1", false, null, 0, "0"];

@godismyjudge95
Copy link
Contributor Author

The single failing test test_parser_isolation_considers_all_options_after_taxonomy was failing due to the assumption of entry order when a title is empty. Previously, when the title was empty the default (sorting title by ascending) would put the nulls would be at the end; now they are at the beginning since they are considered a lesser value

@jasonvarga
Copy link
Member

Also in favor of this change, if you create an array in PHP and sort it you get the expected orders - with null coming after non-null:

sort($values);
// $values = [false, null, 0, "0", true, 1, "1"];

This is doing falses, then nulls, then trues.
You're wanting nulls at the end, no? They're in the middle here.

The null sorting behavior should match sort() and collect()->sort(). The changes in this PR don't seem to. Also, this should probably target master since it'd be a breaking change.

However I have a simple fix that I can push separately. Toggle fields should get indexed as booleans, not nulls. Then when you query you will only be dealing with true and false.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants