Facet's results sorting with multiple sorts.

Writing a testcase to test the expected behavior.

I'm actually not sure what the expected result for the third added test here should be. At least there's a way to test this now. That's great.

Setting to NR so the bot can take a look at the patch, but we first need to have some consensus on how to resolve this.

The test added here should be moved to the ProcessorIntegrationTest, introduced in #2680301: Expand processor test coverage..
We should also still figure out how to fix this issue.

I did what I said in #7. Moved tests.

We discussed this and I'll post a patch with some work tomorrow.

Just checking in some work. Unit tests already pass again. Everything else is still very much broken.

This fixes more of the tests, still not ready to actually review.

Saving works again, but there's a problem in the config that I can't figure out yet.
Trying to figure out how to fix that. I still have to figure out how to do multiple sorts.

Not sure about #21, I'll look at that suggestion tomorrow.
#22 looked great and I implemented that and adapted the unit tests as well.

#27 doesn't look like it'll help. So the next patch'll be based on #26.

I spent more time on this, but I can't figure out how to fix those config errors.

I changed some small things for coding standards in the patch.

I rolled a new patch after the last commits, still no idea how to resolve the config errors.

Adding to the plugin_type file

reroll.

This is the correct patch, that was a search api patch.

Needed a reroll, so let's try again.

Rerolling is not my strongest suit.

We should be back to only having config errors.

I can't figure it out, but this line is not needed, so removing that. Still the same fails.

Marking this with higher priority. It's a big config change that needs to be fixed.

This needed a rebase.

Another reroll.

Following code is another approuch, no seperate sorting plugin used here.

This should work, no tests fixxed yet, needs work. Also tests from #66 should be ported in this patch if we pick this route.

Ported test from #66, fixed unit tests as well.

I don't see the fail in testResultSorting locally.

This looks great, but since this is such a big change, I'll leave it open for a couple of days for additional feedback. I love how the last patch is less than half the size of the one in #66 and that is deletes a bunch of code as well.

Minors.

1. +++ b/src/FacetManager/DefaultFacetManager.php
   @@ -291,14 +292,37 @@ class DefaultFacetManager {
   +        // @todo For some reason getProcessorsByStage does not return the
   +        // correct configuration, thats why we use getProcessors() below.
   

   Let's investigate this in a separate issue and refer the new issue in the @todo. I think it's important to find out what's going on with that as it might affect other functionalities.

2. +++ b/src/FacetManager/DefaultFacetManager.php
   @@ -291,14 +292,37 @@ class DefaultFacetManager {
   +        $processors = $facet->getProcessors();
   

   Until fixing getProcessorsByStage(): Let's move this outside foreach () {...}. It doesn't make sense to call that method on every processor. We get the list once, before foreach.

3. +++ b/src/FacetManager/DefaultFacetManager.php
   @@ -291,14 +292,37 @@ class DefaultFacetManager {
   +          throw new InvalidProcessorException(new FormattableMarkup("The processor @processor has a build definition but doesn't implement the required BuildProcessorInterface interface", ['@processor' => $processor->getPluginDefinition()['id']]));
   

   We try to not call any function/method during throwing an exception because that can cause other unexpected errors, obscuring the main exception. Just use the string in double quotes an inject variables with {}. Probably "The processor {$processor->getPluginDefinition()['id']} has a build definition but doesn't implement the required BuildProcessorInterface interface"?

4. +++ b/src/Processor/WidgetOrderProcessorInterface.php
   @@ -10,15 +11,14 @@ interface WidgetOrderProcessorInterface extends BuildProcessorInterface {
   -  public function sortResults(array $results, $order = 'ASC');
   +  public function sortResults(Result $a, Result $b);
   

   Is this module in BC mode? Hope not, otherwise we break other modules implementing this interface.

#83:
3. Oh, I didn't realize that, we should do that in other places as well.
4. We've released 2 alpha versions and haven't promised to keep bc yet, so we're good on that front.

Apart from #83, I tested the patch and works as expected.

#83

1. Opened up a new issue: #2722267: Fix Facet::getProcessorsByStage and removed the comment.
2. Fixed.
3. Fixed for this patch, also opened up #2722271: Make sure as little code as possible is triggered when throwing an exception..

Thanks for the review @claudiu.cristea!

@borisson_, thank you.

+++ b/src/FacetManager/DefaultFacetManager.php
@@ -293,19 +293,17 @@ class DefaultFacetManager {
-        // @todo For some reason getProcessorsByStage does not return the
-        // correct configuration, thats why we use getProcessors() below.

Hm. I still believe that this comment is valuable. Why? Because after #2722267: Fix Facet::getProcessorsByStage is fixed we need to get back here and fix this (probably in a follow-up). So, I would keep the @todo comment and add also a @see https://www.drupal.org/node/2722267. In this case we need to indent the 2nd line of the @todo comment by 2 spaces.

Sure, that makes sense.

Comitted and pushed, thanks for the patches and reviews!