Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Problem/Motivation
When editing a facet based on an aggregated field, a critical error is thrown.
The website encountered an unexpected error. Please try again later.
Error: Call to undefined method Drupal\search_api\Plugin\search_api\processor\Property\AggregatedFieldProperty::getPropertyDefinitions() in Drupal\facets\Plugin\facets\processor\BooleanItemProcessor->supportsFacet() (line 92 of modules/contrib/facets/src/Plugin/facets/processor/BooleanItemProcessor.php).
Drupal\facets\Plugin\facets\processor\BooleanItemProcessor->supportsFacet(Object) (Line: 195)
Drupal\facets\Form\FacetForm->form(Array, Object) (Line: 117)
Drupal\Core\Entity\EntityForm->buildForm(Array, Object)
call_user_func_array(Array, Array) (Line: 514)
Drupal\Core\Form\FormBuilder->retrieveForm('facets_facet_edit_form', Object) (Line: 271)
Drupal\Core\Form\FormBuilder->buildForm('facets_facet_edit_form', Object) (Line: 74)
Drupal\Core\Controller\FormController->getContentResult(Object, Object)
call_user_func_array(Array, Array) (Line: 123)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 576)
Drupal\Core\Render\Renderer->executeInRenderContext(Object, Object) (Line: 124)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->wrapControllerExecutionInRenderContext(Array, Array) (Line: 97)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}()
call_user_func_array(Object, Array) (Line: 153)
Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object, 1) (Line: 68)
Symfony\Component\HttpKernel\HttpKernel->handle(Object, 1, 1) (Line: 57)
Drupal\Core\StackMiddleware\Session->handle(Object, 1, 1) (Line: 47)
Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object, 1, 1) (Line: 99)
Drupal\page_cache\StackMiddleware\PageCache->pass(Object, 1, 1) (Line: 78)
Drupal\page_cache\StackMiddleware\PageCache->handle(Object, 1, 1) (Line: 47)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object, 1, 1) (Line: 50)
Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object, 1, 1) (Line: 23)
Stack\StackedHttpKernel->handle(Object, 1, 1) (Line: 657)
Drupal\Core\DrupalKernel->handle(Object) (Line: 19)| Comment | File | Size | Author |
|---|---|---|---|
| #14 | critical_error_thrown-2917323-14.patch | 8.13 KB | borisson_ |
Comments
Comment #2
borisson_Thanks for bringing this up, I don't we have a test with an aggregated field or we would've noticed this. Let's create a test and fix the offending code.
Comment #3
borisson_This test doesn't work yet - but it's a start.
Comment #4
borisson_Patch should work, but I don't know how to write the patch so that it passes. It currently breaks in super awesome ways.
Comment #6
archnode CreditAttribution: archnode at Signal commentedThank you for working on this! Still fails for me with patch and latest facets dev, though.
Comment #7
oxrc CreditAttribution: oxrc as a volunteer commentedThis happened to me also on custom fields added via a processor for Search API using this tutorial: https://www.drupal.org/docs/8/modules/search-api/developer-documentation...
Because of my lack of experience/knowledge with Search API and Facets modules, I wasn't sure if the Search API processors or the Facets approach should be changed, your patch makes lots of sense and has been applied so far ok, I have been able to finally see the configuration form for my widget. I'll be continuing testing it and see if all is fine (running facets 1.x-dev + Search API 1.5 here).
Comment #9
borisson_I asked Markus for help on how to write this test, he suggested adding a new index as config - and using that. So that's the plan.
Already adding credit for that helpful suggestion.
Comment #10
borisson_So markus suggested using a new index and adding that export config. I figured that was a good solution until I figured that would mean a new testmodule + view. That seems like a lot for one test.
So intead I put some more work into adding the field from the code.
I also added comments in the test to explain what I'm trying to do.
This adds a test-only (that should fail) and a normal patch (that should pass). Let's see if the testbot agrees with me.
Comment #12
drunken monkeyJoris asked me to look at this. It seems you've already got the tests passing as they should, so that's great.
However, regarding the actual fix, I have two comments:
getPropertyDefinitions(), you should just check whether the data definition is actually an instance of the interface that defines that method – i.e.,\Drupal\Core\TypedData\ComplexDataDefinitionInterface. I'm actually amazed that this doesn't cause trouble way more often – but probably that's because Field API fields are actually complex data now, so unless you index a sub-property (e.g., text field format), this bug won't be triggered by them.valuesub-property will have the desired data type. However, looping over all sub-properties for this also isn't the right way to do this – instead, you should just check the type of the main property, if available (cf.ComplexDataDefinitionInterface::getMainPropertyName()). That last bit isn't included in my attached patch, since I only thought of that just now.Comment #13
drunken monkeyComment #14
borisson_#13 copied the boolean check to all processors. This should be more specific. If all tests pass and everything still works we can commit this.
Thanks @drunken monkey!
Comment #15
oxrc CreditAttribution: oxrc as a volunteer commentedHey there,
Patch #14 applies fine for me and the problem seems solved. Nice to have @drunken_monkey looking into this one. Works for me.
Comment #16
borisson_Awesome, marking this as rtbc based on #15.
Comment #17
StryKaizerThx all!