Hi all,

Testing another issue I had to set up a Facet for a Search API view, a REST export display. Being my first time I saved with the default values, and I got a white page with an error. Later I realized that I had to select the "Array with raw results" option for the Widget, but it wasn't evident to me that this was needed.

Would it be possible to add maybe some description to the widget selection field? I don't know if it's possible to catch the Exception and fail more gracefully.

The Exception shown and the trace is the following one:

Symfony\Component\Serializer\Exception\UnexpectedValueException: Could not normalize object of type Drupal\Core\Url, no supporting normalizer found. in Symfony\Component\Serializer\Serializer->normalize() (line 149 of /var/www/drupal8/vendor/symfony/serializer/Serializer.php).
Symfony\Component\Serializer\Serializer->normalize(Array, 'json', Array) (Line: 138)
Symfony\Component\Serializer\Serializer->normalize(Array, 'json', Array) (Line: 138)
Symfony\Component\Serializer\Serializer->normalize(Array, 'json', Array) (Line: 138)
Symfony\Component\Serializer\Serializer->normalize(Array, 'json', Array) (Line: 138)
Symfony\Component\Serializer\Serializer->normalize(Array, 'json', Array) (Line: 138)
Symfony\Component\Serializer\Serializer->normalize(Array, 'json', Array) (Line: 101)
Symfony\Component\Serializer\Serializer->serialize(Array, 'json', Array) (Line: 94)
Drupal\rest_facets\Plugin\views\style\FacetsSerializer->render() (Line: 390)
Drupal\rest\Plugin\views\display\RestExport->Drupal\rest\Plugin\views\display\{closure}() (Line: 574)
Drupal\Core\Render\Renderer->executeInRenderContext(Object, Object) (Line: 391)
Drupal\rest\Plugin\views\display\RestExport->render() (Line: 1520)
Drupal\views\ViewExecutable->render() (Line: 381)
Drupal\rest\Plugin\views\display\RestExport->execute() (Line: 1617)
Drupal\views\ViewExecutable->executeDisplay('rest_export_1', Array) (Line: 78)
Drupal\views\Element\View::preRenderViewElement(Array)
call_user_func(Array, Array) (Line: 376)
Drupal\Core\Render\Renderer->doRender(Array, 1) (Line: 195)
Drupal\Core\Render\Renderer->render(Array, 1) (Line: 139)
Drupal\Core\Render\Renderer->Drupal\Core\Render\{closure}() (Line: 574)
Drupal\Core\Render\Renderer->executeInRenderContext(Object, Object) (Line: 140)
Drupal\Core\Render\Renderer->renderRoot(Array) (Line: 364)
Drupal\rest\Plugin\views\display\RestExport::buildResponse('shapes', 'rest_export_1', Array) (Line: 52)
Drupal\views\Routing\ViewPageController->handle('shapes', 'rest_export_1', Object)
call_user_func_array(Array, Array) (Line: 123)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 574)
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: 144)
Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object, 1) (Line: 64)
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: 652)
Drupal\Core\DrupalKernel->handle(Object) (Line: 19)
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jsbalsera created an issue. See original summary.

borisson_’s picture

We could try to catch that exception and throw a new one instead. Not sure if that'd be better though.

Adding a description to the widget selection field is a good idea though, we just have to figure out the best way to do that. Probably something like this. We could add this in the original save of the entity ($entity->isNew())

if ($facet->getFacetSource->getDisplay() instanceof \Drupal\search_api\Plugin\search_api\display\ViewsRest) { 
drupal_set_message('Please select a raw widget'); $facet->setWidget('array');
}
jsbalsera’s picture

Assigned: Unassigned » jsbalsera
Issue tags: +drupaldevdays

Ok, I will take a look into it.

Thank you!

jsbalsera’s picture

Hi again, @borisson_

I've been trying to add a code similar to your exmaple inside the form validation (FacetForm::validateForm), but I discovered that getDisplay is a protected method of the SearchApiDisplay. I wasn't able to find another way to obtain the same information, and after talking with @marthinal, we are not sure if maybe we should just change getDisplay to public?

borisson_’s picture

As long as it gets added to \Drupal\facets\FacetSource\SearchApiFacetSourceInterface I don't think this is a bad idea.

jsbalsera’s picture

Status: Active » Needs review
FileSize
2.35 KB

I have attached a patch for this, but maybe some of the texts could be improved. I added the getDisplay public method to the FacetSourcePluginInterface (this is the one that SearchApiDisplay implements, if I'm not mistaken), but I'm not sure about the comment here. Are all the classes extending this going to be a Views Display?

Additionally, I would be happy to provide some test, but I'm not completely sure about the right type, and if I should just add one test to one of the existing classes or...

Thank you.

borisson_’s picture

FacetSourcePluginInterface is not the right interface, it's supposed to be SearchApiFacetSourceInterface. SearchApiDisplay doesn't implement that but it should (we can change that in a followup or in this issue, if you'd like an extra credit I don't mind another issue ;)).

The reason it's supposed to be on that interface is because we core search facets won't be able to return the search api information.

An integration test should be provided, it can be appended as a new method in the \Drupal\Tests\rest_facets\Functional\RestIntegrationTest, I'd probably make a new method called: "testWidgetSelection" that creates a new facet trough the UI (plenty of examples to be found in the tests), and does assertText("message"); and checks if the facet now has the expected widget.

  1. +++ b/src/FacetSource/FacetSourcePluginInterface.php
    @@ -98,4 +98,10 @@ interface FacetSourcePluginInterface extends PluginFormInterface, DependentPlugi
    +  /**
    +   * Returns the display used by the Facet Source.
    +   *
    +   * @return mixed
    +   */
    

    Returns the search api facet source.

    Search Api facet sources have are linked to a display plugin, we return that one here.

    @return \Drupal\search_api\Display\DisplayInterface

  2. +++ b/src/Form/FacetForm.php
    @@ -558,6 +558,8 @@ class FacetForm extends EntityForm {
    +
    +
         // Iterate over all processors that have a form and are enabled.
    

    These newlines should be removed again.

  3. +++ b/src/Form/FacetForm.php
    @@ -575,6 +577,14 @@ class FacetForm extends EntityForm {
    +    // Only the raw widgets work with Rest sources. If the user selects another
    +    // the system will raise an Exception, so we make a validation here.
    

    Let's rephrase this a little bit:

    "Only widgets that return an array can work with rest facet sources, so if the user has selected another widget, we should point them to their misconfiguration."

    I think that's better? If you disagree I don't mind.

Thanks for all you've done on this issue so far; great work!

borisson_’s picture

Status: Needs review » Needs work

NW per #7.

jsbalsera’s picture

Status: Needs work » Needs review
FileSize
4.5 KB
5.29 KB

Ok, there goes a new try, adding the suggestions in #7 and a test.

And thank to you, @borisson_! It's great to get such a good feedback and so fast :-)

borisson_’s picture

Status: Needs review » Reviewed & tested by the community

Looks great!

  • borisson_ committed 9e162f1 on 8.x-1.x authored by jsbalsera
    Issue #2862982 by jsbalsera, borisson_: Selecting the wrong widget in a...
borisson_’s picture

Status: Reviewed & tested by the community » Fixed

Committed, thanks

ChristianAdamski’s picture

Priority: Normal » Major
Status: Fixed » Needs work

Before calling getDisplay() at line 585 in FacetForm, there is no check whatsoever if the facet source actually offers that function. There is no check for facet source being type SearchApiFacetSourceInterface. So at least for us, facets cannot be saved anymore.

ChristianAdamski’s picture

Status: Needs work » Needs review
FileSize
1.57 KB

Maybe this fixes it.

There are some more non-interface function calls throughout the file, but none critical to us.

borisson_’s picture

This looks good, if the tests come back green we can commit this. Thanks!

Status: Needs review » Needs work

The last submitted patch, 14: 2862982-14-do-not-call-non-interface-functions.patch, failed testing.

borisson_’s picture

14:03:12 Warning: failed to get default registry endpoint from daemon (Cannot connect to the Docker daemon. Is the docker daemon running on this host?). Using system default: https://index.docker.io/v1/

This not our fault, testbot should pick this up again automatically.

ChristianAdamski’s picture

I did create the patch from our project repository, not a clean facets git clone. It does look fine though.

borisson_’s picture

Status: Needs work » Fixed

Committed and pushed.

Status: Fixed » Closed (fixed)

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