Problem/Motivation

When using this module we are getting a TypeError after the plugin redirects.

TypeError: Drupal\facets\FacetManager\DefaultFacetManager::sortFacetResults(): Argument #2 ($results) must be of type array, null given, called in /app/web/modules/contrib/facets/src/FacetManager/DefaultFacetManager.php on line 350 in Drupal\facets\FacetManager\DefaultFacetManager->sortFacetResults() (line 510 of /app/web/modules/contrib/facets/src/FacetManager/DefaultFacetManager.php)

ActiveByDefaultProcessor breaks the BuildProcessorInterface as it does not return a value and execution continues. This results in $results becoming NULL as the DefaultFacetManager builds the facet.

Steps to reproduce

  1. Enable the module and configure a facet to redirect to the first item
  2. Ensure the facet has at least 1 sort plugin configured
  3. Navigate to the search result
  4. Observe you have been redirect
  5. Observe the error is in the logs

Proposed resolution

Drupal discourages terminating the execution mid-request. We should return the results as is defined by the interface.

While investigating this I found that the redirect is not performed exactly as documented in https://www.drupal.org/node/2023537

  1. Ensure the session is saved and kernel events are triggered as per "Redirecting when not in context of a controller" under https://www.drupal.org/node/2023537
  2. return results

Remaining tasks

  • Patch
  • Review
  • Commit

User interface changes

n/a

API changes

n/a

Data model changes

n/a

CommentFileSizeAuthor
#5 facet_active_by_default-terminate-execution-after-redirect-3366611-3.patch3.3 KBericgsmith
#3 interdiff_2_3.txt442 bytesericgsmith
#3 entity_usage_validate-incorrect-field-ref-3355512-3.patch606 bytesericgsmith
#2 facet_active_by_default-terminate-execution-after-redirect-3366611-2.patch3.32 KBericgsmith

Comments

ericgsmith created an issue. See original summary.

ericgsmith’s picture

ericgsmith commented
Assigned: ericgsmith » Unassigned
Issue summary: View changes
Status: Active » Needs review
StatusFileSize
new facet_active_by_default-terminate-execution-after-redirect-3366611-2.patch3.32 KB
ericgsmith’s picture

ericgsmith commented
Issue summary: View changes
StatusFileSize
new entity_usage_validate-incorrect-field-ref-3355512-3.patch606 bytes
new interdiff_2_3.txt442 bytes

*EDIT* Apoliges this is the wrong patch attached here

After conversation with a colleague I've removed an earlier implementation containing exit in favour of returning the results.

ericgsmith’s picture

ericgsmith commented
Title: TypeError as execution continues after plugin redirects » TypeError as an array is not returned after redirecting
ericgsmith’s picture

ericgsmith commented
StatusFileSize
new facet_active_by_default-terminate-execution-after-redirect-3366611-3.patch3.3 KB

Sorry - wrong patch attached earlier

xurizaemon’s picture

xurizaemon
he/him
Location Ōtepoti, Aotearoa 🏝
commented

Thanks Eric. It feels like sending a RedirectResponse really ought to trigger termination of the current request, to me.

(EDIT: By which I mean ... I think the proposed change looks correct, but I feel like it should be unnecessary to introduce and then terminate the kernel request as I feel like that would be sensible default behaviour for core when sending a RedirectResponse already.)

I don't see that, but wanted to check in case we'd missed something there.

ericgsmith’s picture

ericgsmith commented

Hi Chris,

Sorry I missed this message when I was on leave.

but I feel like it should be unnecessary to introduce and then terminate the kernel request as I feel like that would be sensible default behaviour for core when sending a RedirectResponse already

You are correct - if core was doing the send we wouldn't need to do this - the issue is that the plugin is sending the response so that plugin needs to ensure it is terminated. If this was redirecting somewhere that core expected (e.g middleware, a controller or event subscriber) it wouldn't be needed.

The main different here is that its not good to be sending your own responses - core should be doing that. But at the same time its relying on information this isn't available until the query to solr has run and the response is being built.

Another option would be to add a kernel event listener to the event dispatcher - this is what core does in the drupal goto plugin https://git.drupalcode.org/project/drupal/-/blob/11.x/core/lib/Drupal/Co...

This would let the request complete and be handle by Drupal - less code / logic for the module - but it not what the (admittedly dated) CR says about redirect outside the context of a controller - https://www.drupal.org/node/2023537

  • azinck committed 5e041e06 on 1.0.x authored by ericgsmith 
    Issue #3366611 by ericgsmith: TypeError as an array is not returned...
azinck’s picture

azinck commented
Status: Needs review » Fixed

Thanks for the fix!

Status: Fixed » Closed (fixed)

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