Give widgets the ability to require settings

Removing tag, was discussed in the facetapi hangout of 09/11.

If there are processors that'll break a widget, we could add this.
Otherwise, we could add "not recommended".

Changing category to reflect that this is a feature.

I started working on this, because I figured we were going to need this to make sure only 1 active item was shown for the dropdown widget, turns out that limiting to only 1 active item is something that's not a processor. So leaving my work here but not doing any more work on this for now. This works but needs extra tests before it can be committed and an alter hook in the methods.

That sounds great. Don't know if already exist or not or right place but there should also be some way to define if a plugin is compatible to the the field/data type (may be by annotation). I am porting range slider (https://github.com/joshirohit100/facet_slider) and seems like it should only be available for number or date/timestamp field/datatype

@borisson_ and I discussed this, and we agreed that for the moment we only see a use case for requiring certain processors or settings to be active.
I created a wip patch that provides the basics for this functionality.

it lacks the functionality to require the processors and settings when changing the widget. This should be done with some ajax (just as the widget specific settings).

This functionality is needed for the slider widget, as the 'Make sure only one result can be shown.' widget setting should always be enabled.

Setting to needs review for the testbot. Also changing priority now that we have a use-case.

I made a typo, added new patch an interdiff

I'd love to also get tests for this functionality, I can work on those if needed. Back to NW for tests.

This only applied with git apply -C 2 mark_certain_processors-2611198-13.patch. So new patch attached.

cs fixes.

NR, for tests.

Start of integration test, but nothing happens really. Not sure if I did it right.

Back to NW for the additional tests.

Test's red now. @mollux, am I doing it right?

Rebased.

Updated IS to say what we're actually working on here.

The new test is green locally. So that's great. Let's see how the bots feel

It took me ~5 months but the tests here are green again! I feel this is sufficient for now. Since this is an API break - I'd like to get this in before tagging the first beta.

1. +++ b/src/Form/FacetForm.php
   @@ -137,6 +137,7 @@ class FacetForm extends EntityForm {
   +    $widget = $facet->getWidgetInstance();
   

   Should this get a phpstorm codehint?

2. +++ b/src/Form/FacetForm.php
   @@ -218,7 +219,7 @@ class FacetForm extends EntityForm {
   +          '#default_value' => $processor->isLocked() || $widget->isPropertyRequired($processor_id, 'processors') || !empty($enabled_processors[$processor_id]),
   

   This code gets quite long, should we break it out?

3. +++ b/src/Widget/WidgetPluginBase.php
   @@ -212,4 +212,33 @@ abstract class WidgetPluginBase extends PluginBase implements WidgetPluginInterf
   +  public function isPropertyRequired($name, $type) {
   

   When using api functions it is preferable they follow the chain. Eg, first define the type and then the variable to check.

   isPropertyRequired('processor', 'processor-to-check') instead of the reverse.

4. +++ b/src/Form/FacetForm.php
   @@ -313,14 +314,16 @@ class FacetForm extends EntityForm {
   +      '#default_value' => ($widget->isPropertyRequired('show_only_one_result', 'settings') ?: $facet->getShowOnlyOneResult()),
   

   Are the ( and ) needed here? Doesn't add much for readability.

5. +++ b/src/Form/FacetForm.php
   @@ -313,14 +314,16 @@ class FacetForm extends EntityForm {
   +      '#disabled' => ($widget->isPropertyRequired('show_only_one_result', 'settings') ?: 0),
   

   Are the ( and ) needed here? Doesn't add much for readability.

6. +++ b/src/Widget/WidgetPluginBase.php
   @@ -212,4 +212,33 @@ abstract class WidgetPluginBase extends PluginBase implements WidgetPluginInterf
   +      return $this->requiredFacetProperties()[$type][$name];
   

   Does this actually return true or a value? Careful to pass unexpected values. Certainly because this is a plugin type that developers can create themselves.

@Nick_vh

1. No, phpstorm understands the @return typehint.
2. Done!
3. Not sure I agree here, I think it makes sense like this as well.
4. done
5. done
6. Should return a boolean. That's how it's documented, I don't think we should defend against improper api usage here.

Reroll attached

Discussed this with nick, we should use a value object instead of an array here.

Discussed this with @mollux, we decided to trash requiredFacetProperties

Updated IS.

Fixes the test.

Adds extra docs.

Committed, thanks!