Facets results is sorted incorrectly then there are more then one sort processor enabled.
Results are sorted with last processor.

That's is because all processors in build stage are running by in self and sort processors rewrite previous results.
All sort processor's callbacks should be applied for result item pair. And the first value which is not equal 0 should be returned to apply sort.

Maybe we need new stage - sorting. After build stage run sorting with enabled processors.

Files: 

Comments

Evaldas Užkuras created an issue. See original summary.

borisson_’s picture

Assigned: Unassigned » borisson_

Writing a testcase to test the expected behavior.

borisson_’s picture

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.

Status: Needs review » Needs work

The last submitted patch, 3: facet_s_results_sorting-2656668-3.patch, failed testing.

The last submitted patch, 3: facet_s_results_sorting-2656668-3.patch, failed testing.

borisson_’s picture

Assigned: borisson_ » Unassigned
Issue tags: +Needs committer feedback
borisson_’s picture

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.

borisson_’s picture

Status: Needs review » Needs work

The last submitted patch, 8: facet_s_results_sorting-2656668-8.patch, failed testing.

The last submitted patch, 8: facet_s_results_sorting-2656668-8.patch, failed testing.

borisson_’s picture

Issue tags: -Needs committer feedback

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

borisson_’s picture

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

Status: Needs review » Needs work

The last submitted patch, 12: facet_s_results_sorting-2656668-12.patch, failed testing.

The last submitted patch, 12: facet_s_results_sorting-2656668-12.patch, failed testing.

borisson_’s picture

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

Status: Needs review » Needs work

The last submitted patch, 15: facet_s_results_sorting-2656668-15.patch, failed testing.

The last submitted patch, 15: facet_s_results_sorting-2656668-15.patch, failed testing.

borisson_’s picture

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.

Status: Needs review » Needs work

The last submitted patch, 18: facet_s_results_sorting-2656668-18.patch, failed testing.

The last submitted patch, 18: facet_s_results_sorting-2656668-18.patch, failed testing.

Evaldas Užkuras’s picture

FileSize
1.3 KB

Maybe we should save processors config directly to processors instead of created new dimension in array with key 'settings',
I have added a proposal, how to make it.
Using that way, we can get config's value using:

$processor->getConfiguration()['sort']
Evaldas Užkuras’s picture

I have made the proposal for applying multiple sorts.
Maybe it would be helpful.

borisson_’s picture

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.

Status: Needs review » Needs work

The last submitted patch, 23: facet_s_results_sorting-2656668-23.patch, failed testing.

The last submitted patch, 23: facet_s_results_sorting-2656668-23.patch, failed testing.

borisson_’s picture

borisson_’s picture

The last submitted patch, 26: facet_s_results_sorting-2656668-26.patch, failed testing.

The last submitted patch, 26: facet_s_results_sorting-2656668-26.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 27: with-patch-21.patch, failed testing.

The last submitted patch, 27: with-patch-21.patch, failed testing.

borisson_’s picture

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

borisson_’s picture

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.

Status: Needs review » Needs work

The last submitted patch, 33: facet_s_results_sorting-2656668-33.patch, failed testing.

The last submitted patch, 33: facet_s_results_sorting-2656668-33.patch, failed testing.

borisson_’s picture

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

Status: Needs review » Needs work

The last submitted patch, 36: facet_s_results_sorting-2656668-36.patch, failed testing.

The last submitted patch, 36: facet_s_results_sorting-2656668-36.patch, failed testing.

borisson_’s picture

Status: Needs review » Needs work

The last submitted patch, 39: facet_s_results_sorting-2656668-39.patch, failed testing.

The last submitted patch, 39: facet_s_results_sorting-2656668-39.patch, failed testing.

borisson_’s picture

Status: Needs review » Needs work

The last submitted patch, 42: follow_drupal_coding-2671288-39.patch, failed testing.

The last submitted patch, 42: follow_drupal_coding-2671288-39.patch, failed testing.

borisson_’s picture

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

Status: Needs review » Needs work

The last submitted patch, 45: facet_s_results_sorting-2656668-42.patch, failed testing.

The last submitted patch, 45: facet_s_results_sorting-2656668-42.patch, failed testing.

borisson_’s picture

Status: Needs review » Needs work

The last submitted patch, 48: facet_s_results_sorting-2656668-48.patch, failed testing.

The last submitted patch, 48: facet_s_results_sorting-2656668-48.patch, failed testing.

borisson_’s picture

Status: Needs review » Needs work

The last submitted patch, 51: facet_s_results_sorting-2656668-51.patch, failed testing.

The last submitted patch, 51: facet_s_results_sorting-2656668-51.patch, failed testing.

borisson_’s picture

We should be back to only having config errors.

Status: Needs review » Needs work

The last submitted patch, 54: facet_s_results_sorting-2656668-54.patch, failed testing.

The last submitted patch, 54: facet_s_results_sorting-2656668-54.patch, failed testing.

borisson_’s picture

Status: Needs review » Needs work

The last submitted patch, 57: facet_s_results_sorting-2656668-57.patch, failed testing.

The last submitted patch, 57: facet_s_results_sorting-2656668-57.patch, failed testing.

borisson_’s picture

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

borisson_’s picture

Priority: Normal » Major

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

StryKaizer’s picture

Issue tags: +beta blocker
borisson_’s picture

Status: Needs work » Needs review
FileSize
77.38 KB

This needed a rebase.

Status: Needs review » Needs work

The last submitted patch, 63: facet_s_results_sorting-2656668-63.patch, failed testing.

The last submitted patch, 63: facet_s_results_sorting-2656668-63.patch, failed testing.

borisson_’s picture

Status: Needs review » Needs work

The last submitted patch, 66: facet_s_results_sorting-2656668-66.patch, failed testing.

The last submitted patch, 66: facet_s_results_sorting-2656668-66.patch, failed testing.

StryKaizer’s picture

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.

Status: Needs review » Needs work

The last submitted patch, 69: facet_s_results_sorting-2656668-69.patch, failed testing.

The last submitted patch, 69: facet_s_results_sorting-2656668-69.patch, failed testing.

borisson_’s picture

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

The last submitted patch, 72: facet_s_results_sorting-2656668-72.patch, failed testing.

The last submitted patch, 72: facet_s_results_sorting-2656668-72.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 73: facet_s_results_sorting-2656668-73.patch, failed testing.

The last submitted patch, 73: facet_s_results_sorting-2656668-73.patch, failed testing.

borisson_’s picture

I don't see the fail in testResultSorting locally.

Status: Needs review » Needs work

The last submitted patch, 78: facet_s_results_sorting-2656668-78.patch, failed testing.

The last submitted patch, 78: facet_s_results_sorting-2656668-78.patch, failed testing.

StryKaizer’s picture

borisson_’s picture

Status: Needs review » Reviewed & tested by the community

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.

claudiu.cristea’s picture

Status: Reviewed & tested by the community » Needs review

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.

borisson_’s picture

#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.

claudiu.cristea’s picture

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

borisson_’s picture

claudiu.cristea’s picture

@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.

borisson_’s picture

claudiu.cristea’s picture

Status: Needs review » Reviewed & tested by the community

  • borisson_ committed 5823b2d on 8.x-1.x authored by StryKaizer
    Issue #2656668 by borisson_, StryKaizer, Evaldas Užkuras, claudiu....
borisson_’s picture

Status: Reviewed & tested by the community » Fixed

Comitted and pushed, thanks for the patches and reviews!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.