The configuration inspector reports an issue with the stored facet schemas.

There is an "options" key in the schema that should not be there. The key itself contains some other keys that should be higher in the yaml file hierarchy.

Actual report:
options Undefined undefined missing schema

Local Example:

uuid: ###
langcode: de
status: true
dependencies: {  }
id: user_search_city
name: 'User Search - City'
url_alias: city
field_identifier: combine
query_type_name: null
facet_source_id: 'core_views:search_users_v1:search_page'
widget: links
widget_configs:
  show_numbers: '1'
options:
  processors:
    core_views_exposed_filter_url:
      processor_id: core_views_exposed_filter_url
      weights: {  }
      settings: {  }
    count_widget_order:
      processor_id: count_widget_order
      weights: {  }
      settings:
        sort: DESC
  empty_behavior:
    behavior: none
only_visible_when_facet_source_is_visible: true
CommentFileSizeAuthor
#19 clean_up_facet_schema-2645128-18.patch26.59 KBborisson_
#19 interdiff.txt1.13 KBborisson_
#17 2645128-16-cleanup-and-2640990.patch25.79 KBChristianAdamski
#13 2645128-12-cleanup-and-2640990.patch25.26 KBChristianAdamski
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

ChristianAdamski created an issue. See original summary.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ for Acquia commented
Title: Configuration inspector reports issue in facet schema » Clean up facet schema / class.

So this issue needs to do work related to for #2638116: Clean up caching of Index class method results (especially fields) - in the search Api queue.

We want to do a couple of things to make this better:

  1. Remove [g|s]etOption(s) from the facet class / interface.
  2. Add an addProcessor / removeProcessor method to the facet class / interface and use those instead of the options to set the processors.

This is probably a good start.

ChristianAdamski’s picture

ChristianAdamski CreditAttribution: ChristianAdamski as a volunteer commented 
src/FacetInterface.php:  
public function setOption($name, $option);
public function setOptions(array $options);

src/Form/FacetForm.php:      
$facet->setOption('processors', $initial_settings);
$facet->setOption('empty_behavior', ['behavior' => 'none']);

src/Form/FacetDisplayForm.php:    
$facet->setOption('processors', $new_settings);
$facet->setOption('empty_behavior', $empty_behavior_config);

src/Entity/Facet.php:  
public function setOption($name, $option) {
public function setOptions(array $options) {

core_views_facets/src/Plugin/facets/core_views_facets/filter_type/Combine.php:    
$facet->setOption("core_views_test", $data);

tests/src/Unit/Plugin/processor/ExcludeSpecifiedItemsProcessorTest.php:    
$facet->setOption('processors', [
$facet->setOption('processors', [
$facet->setOption('processors', [

tests/src/Unit/Plugin/processor/CountLimitProcessorTest.php:    
$facet->setOption('processors', [
$facet->setOption('processors', [
$facet->setOption('processors', [
$facet->setOption('processors', [
$facet->setOption('processors', [
borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ for Acquia commented

We have to be careful not to do this for $url->setOption calls, those are not related to what we're trying to achieve.

ChristianAdamski’s picture

ChristianAdamski CreditAttribution: ChristianAdamski as a volunteer commented

Updated list.

ChristianAdamski’s picture

ChristianAdamski CreditAttribution: ChristianAdamski as a volunteer commented

empty_behavior is stored but not defined in schema

ChristianAdamski’s picture

ChristianAdamski CreditAttribution: ChristianAdamski as a volunteer commented

There are quite a number of

  /**
   * {@inheritdoc}
   */
  public function getUrlAlias() {
    return $this->url_alias;
  }

or 

  /**
   * {@inheritdoc}
   */
  public function getOnlyVisibleWhenFacetSourceIsVisible() {
    return $this->only_visible_when_facet_source_is_visible;
  }

which are at least in some cases simply stored/called by 

$facet->set('widget_configs', $form_state->getValue('widget_configs'));
$facet->set('only_visible_when_facet_source_is_visible', $form_state->getValue(['facet_settings', 'only_visible_when_facet_source_is_visible']));

I would be in favour of removing one-line getters/setters.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ for Acquia commented

I think we should keep the one-line getters / setters and remove the $facet->set('setting', $value); versions of them. If we ever want to include some kind of validation of reactions / caching, these getters/setters will come in handy.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ for Acquia commented

Regarding #6. We should probably save the empty_behavior in a new setting as well. This used to be a plugin and it no longer is, so we can probably clean this up to be a flatter config object.

ChristianAdamski’s picture

ChristianAdamski CreditAttribution: ChristianAdamski as a volunteer commented 
src/Plugin/facets/widget/LinksWidget.php:    
$configuration = $facet->get('widget_configs');

src/Plugin/facets/widget/CheckboxWidget.php:    
$configuration = $facet->get('widget_configs');

src/Form/FacetDisplayForm.php:    
$facet->set('widget_configs', $form_state->getValue('widget_configs'));
$facet->set('only_visible_when_facet_source_is_visible', $form_state->getValue(['facet_settings', 'only_visible_when_facet_source_is_visible']));

tests/src/Unit/Plugin/widget/CheckboxWidgetTest.php:    
$facet->set('widget_configs', ['show_numbers' => 1]);

tests/src/Unit/Plugin/widget/LinksWidgetTest.php:    
$facet->set('widget_configs', ['show_numbers' => 1]);
$facet->set('widget_configs', ['show_numbers' => 1]);
$facet->set('widget_configs', ['show_numbers' => 1]);
$facet->set('widget_configs', ['show_numbers' => 0]);
$facet->set('widget_configs', ['show_numbers' => 1]);
ChristianAdamski’s picture

ChristianAdamski CreditAttribution: ChristianAdamski as a volunteer commented

I got most of this I think. Still left to do:

- clean up tests
- figure out a sane consistent way, to tell if a processor is enabled
- at some point: add a new test
- hook up the url processor changes to "my" views_facets code. I use that for testing.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ for Acquia commented

Awesome, can you post a patch so I can take a look at how you solved this?

ChristianAdamski’s picture

ChristianAdamski CreditAttribution: ChristianAdamski as a volunteer commented
Status: Active » Needs review
FileSize
2645128-12-cleanup-and-2640990.patch25.26 KB

Patch attached. It should handles what is described here and also #2640990.

It works fine here and passes tests. Doesn't say much though.

Status: Needs review » Needs work

The last submitted patch, 13: 2645128-12-cleanup-and-2640990.patch, failed testing.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ for Acquia commented

Test don't actually pass, code looks great at first glance, will have another look later today / tomorrow morning.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ for Acquia commented

Sorry for taking so long to respond. I found a couple of small remarks but this looks great. When tests are green again we can commit this.

  1. +++ b/src/Entity/Facet.php
@@ -178,6 +181,20 @@ class Facet extends ConfigEntityBase implements FacetInterface {
   /**
+   * Configuration for the processors. This is an array of arrays.
+   *
+   * @var array
+   */
+  protected $processor_configs;

    If this is really just an array of strings, the annotation should be @var string[] 

  2. +++ b/src/Entity/Facet.php
@@ -188,6 +205,11 @@ class Facet extends ConfigEntityBase implements FacetInterface {
   /**
+   * @var array;
+   */
+  protected $empty_behavior;

    Is this the plugin or the config? We should add this in as a comment as well. 

  3. +++ b/src/Entity/Facet.php
index d6f1b48..80df4df 100644
--- a/src/FacetInterface.php

--- a/src/FacetInterface.php
+++ b/src/FacetInterface.php

    All the changes in the Interface should have at least a one-line documentation as well as a description for the variable to stay within the drupal documentation standards. 

  4. +++ b/src/FacetInterface.php
@@ -286,4 +286,45 @@ interface FacetInterface extends ConfigEntityInterface {
+  /**
+   * @param array $processor
+   */
+  public function addProcessor(array $processor);

    /**
 * Adds a processor to the facet.
 *
 * @param array $processor
 *   An array of settings describing the processor.
 */

  5. +++ b/src/Form/FacetDisplayForm.php
@@ -500,16 +499,14 @@ class FacetDisplayForm extends EntityForm {
+    $facet->setWidgetConfigs($form_state->getValue('widget_configs'));
+    $facet->setOnlyVisibleWhenFacetSourceIsVisible($form_state->getValue(['facet_settings', 'only_visible_when_facet_source_is_visible']));

    Nice! This looks better.

ChristianAdamski’s picture

ChristianAdamski CreditAttribution: ChristianAdamski as a volunteer commented
Status: Needs work » Needs review
FileSize
2645128-16-cleanup-and-2640990.patch25.79 KB

1.) facet_configs:
type: sequence
label: 'Facet plugin-specific options'
sequence:
type: plugin.plugin_configuration.facets_facet_options.[%key]
label: 'Facet plugin options'

So it's acutally an array arrays. Does that indiate "array[]"?

2.) empty_behavior:
type: mapping
label: 'Empty behavior'
mapping:
behavior:
type: string
label: 'The empty behavior identifier'
text_format:
type: string
label: 'Text format'
text:
type: string
label: 'Text'

I just concluded this from the code. So I guess string[] would be correct here? Added that.

3.) Added comments at some places.

=> Tests failing. Do I have to fix that? Having issues with testing for unrelated reasons here.

Status: Needs review » Needs work

The last submitted patch, 17: 2645128-16-cleanup-and-2640990.patch, failed testing.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ for Acquia commented
Status: Needs work » Needs review
FileSize
interdiff.txt1.13 KB
clean_up_facet_schema-2645128-18.patch26.59 KB

So it's acutally an array arrays. Does that indiate "array[]"?

I don't think that's used, we can use string[][] though.

I just concluded this from the code. So I guess string[] would be correct here? Added that.

Yep that looks correct.

=> Tests failing. Do I have to fix that? Having issues with testing for unrelated reasons here.

The tests are related. I just ran a new branch test and that's green: https://www.drupal.org/pift-ci-job/135470
I attached a patch that should fix the tests.

As a sidenote, it would be awesome if you could provide an interdiff for patches.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ for Acquia commented
Status: Needs review » Fixed

Committed, thanks again for your work.

The last submitted patch, 13: 2645128-12-cleanup-and-2640990.patch, failed testing.

The last submitted patch, 17: 2645128-16-cleanup-and-2640990.patch, failed testing.

Status: Fixed » Needs work

The last submitted patch, 19: clean_up_facet_schema-2645128-18.patch, failed testing.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ for Acquia commented
Status: Needs work » Fixed

Status: Fixed » Closed (fixed)

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