Exposed sorts, that also exist as a default sort for a view, never apply via the exposed form

+++ b/tests/search_api_test_views/config/install/views.view.search_api_test_view.yml
@@ -229,6 +229,30 @@ display:
+          expose:
...
+          expose:

You are missing the 'identifier' key here.

Could you please provide a screenshot of your Views sort configuration, so I have an easier time reproducing the issue?

Nevermind, I see the problem now. Very good analysis, thanks!

After thinking about it, that code in \Drupal\search_api\Query\Query::sort() just seems wrong, or at least counter-intuitive. If I add a sort for a field, and later another one for the same field, the second should not delete the first, but on the contrary be ignored. At least, that's how it works in SQL queries, and also for multiple sorts on different fields: sorts specified later only take effect for those results that are considered "equal" regarding all previous sorts. Which means a second sort on the same field will never have any effect.
The attached patch should fix this. Let's see whether it also fixes the (first) test fail.

Luckily, it seems we never documented the old, broken behavior anywhere, so changing this now isn't exactly an API break. Still, we should probably create a change record for it if we decide to go with this solution.

Also, since you already have the sorts set up, could you maybe check the (first) @todo comment in \Drupal\search_api\Plugin\views\sort\SearchApiSort::query()? (I guess the second one can be deleted with this issue – sorry, should have done that in my patch but realized too late.) I.e., do we still need that weird if block or can we just make the method a simpler one-liner? Could you test whether it works the same (or maybe even better) without that block?
Of course, if we can get those tests to work, that would be even better, since we could then check with them whether or not this block is needed.

+++ b/tests/src/Functional/ViewsTest.php
@@ -438,6 +453,32 @@ class ViewsTest extends SearchApiBrowserTestBase {
+    if (isset($expected_results)) {
+      foreach ($expected_results as $idx => $id) {

I think we can slightly improve this.

if ($expected_results !== NULL) {
  return;
}

foreach (...)...

That's the only remark I have, but because it reduces complexity, I'm setting this back to needs work for it.

Looks great.

Thanks a lot, great job!
Thanks also for your review, Joris!

However, I still have a few nitpicks:

• Test method should be further down, now it's between the existing first test method and its helper methods.
• Making $expected_results optional makes no sense for checkResultsOrder(). Likewise $arguments, if we then never use it.
• checkResultsOrder() should also clearly document it uses a different view, and it should check that really only those results are returned (i.e., also check the count).
• My instinct would also be to include those two measly method calls into the existing testView() instead of adding a new test method just for them. This new test method isn't really needed at all, and unnecessarily increases test run time. However, I already know most others don't share my opinion there, so it's also fine to leave it as a separate method, I guess.

Please see/review the attached revision! (Unfortunately, the interdiff is a bit chaotic due to the moved method. However, I don't think I changed anything in that method, so just ignore that part.)

Was easier to just go trough the patch again. Looks good.

Is the change record for this one ready? Can we have a look at that one so we publish that at the same time as pushing the commit? (That's the way core does this, not sure if we'd want to also use that standard but I guess it's a good idea)

I was planning to write and publish it right when committing the issue, but letting you review it first of course also makes sense, sure: here is my suggestion. Any remarks?

Writing this I realized, though, that subsequent calls could, in some cases, have some meaning: if some code would later fo and remove the (first) sort again from the query, for any reason. However, that wasn't possible before either, and to fix it we'd have to change the (documented) return value of \Drupal\search_api\Query\QueryInterface::getSorts(), so this is a no-go anyways.

That looks super solid! Great job.

Good to hear, thanks again!
Committed and published the CR.
Thanks again also to acbramley for the initial patch!