Refactor widget plugins by adding interface, base class, schema

Refactored a lot. Probably the intediff doesn't help too much. I'm expecting a lot of test failures so, yes, this is just a workin' draft, a start.

I'm not sure that is Novice.

The patch, sorry.

This patch needs some love. I'm afraid I don't have time to work on it next period.

I updated the IS to reflect the changes.

BTW: I think the whole facet config structure should be revised before BETA. It's a little bit messy.

Last test fixing tentative. Please continue to fix them.

So, test failures were significantly decreased in #10. Can somebody take over?

This should fix all unit tests, the integration test is fixed in a way that I'm unhappy with, Hoping to figure out how to fix that better later today.

Wow, nice!

Few notes.

1. +++ b/config/schema/facets.widgets.schema.yml
   @@ -0,0 +1,33 @@
   +facet.widget.config.dropdown:
   ...
   +facet.widget.config.links:
   ...
   +facet.widget.config.checkbox:
   

   Let's add a comment to each to explain the widgets they are referring to.

2. +++ b/src/Plugin/facets/widget/DropdownWidget.php
   @@ -29,7 +29,8 @@ class DropdownWidget extends LinksWidget {
   -    ] + parent::defaultConfiguration();
   +      'show_numbers' => FALSE,
   +    ];
   

   No. The parent always contains 'show_numbers'. That is common to all widgets.

3. +++ b/tests/src/Unit/Plugin/widget/CheckboxWidgetTest.php
   @@ -49,7 +49,7 @@ class CheckboxWidgetTest extends UnitTestCase {
   +    $this->widget = new CheckboxWidget(['show_numbers' => TRUE], 'checkbox', []);
   
   +++ b/tests/src/Unit/Plugin/widget/DropdownWidgetTest.php
   @@ -49,7 +51,12 @@ class DropdownWidgetTest extends UnitTestCase {
   +    $this->widget = new DropdownWidget(['show_numbers' => TRUE], 'dropdown', []);
   
   +++ b/tests/src/Unit/Plugin/widget/LinksWidgetTest.php
   @@ -49,7 +49,7 @@ class LinksWidgetTest extends UnitTestCase {
   +    $this->widget = new LinksWidget(['show_numbers' => TRUE], 'links', []);
   

   probably we want to make __construct() pass only $configuration as signature. But that as optional so we can write something like:

   $widget = new LinksWidget();
   $widget->setConfiguration([...]);
   

   Probably \Drupal\facets\Widget\WidgetPluginBase::__construct can look as:

     /**
      * Constructs a plugin object.
      *
      * @param array $configuration
      *   (optional) An optional configuration to be passed to the plugin. If
      *   missed, the plugin is initialized with its default plugin configuration.
      */
     public function __construct(array $configuration = []) {
       $plugin_id = $this->getPluginId();
       $plugin_definition = $this->getPluginDefinition();
       parent::__construct($configuration, $plugin_id, $plugin_definition);
       $this->setConfiguration($configuration);
     }
   

   The we can call the constructor without parameters. Or only with configuration as 1st parameter.

#15

1. Sure, added those.
2. Because this extended the linkswidget, it also added 'soft_limit', that's not needed for dropdown. So I moved the needed code from the linkswidget to the base class
3. This looks like it makes sense, I'll do that a followup to this patch.

No sorry, Now there are a lot of unrelated changes now in #17. We need to go back to #14 and apply #15, #16.

On 2. you're not right dropdown is extending the base clsss only so he won't get soft_limit

@claudiu.cristea I reverted the changes in #17 and also reverted the fix I applied for the integration test in #14.

About #17.2, this is the definition of the dropdown widget of current HEAD: class DropdownWidget extends LinksWidget {}, it's the same in the current patch.

I reapplied the changes in #17.1 and fixed #17.3.

I expect at least one fail, in the WidgetIntegrationTest.

Working a little bit on it :)

Let's try again

@marthinal, thank you.

Sorry @borisson_, you're absolutely right. I was wrong about class hierarchy. But I feel that the correct inheritance should be:

WidgetPluginBase
- LinksWidget
-- CheckboxWidget
- DropdownWidget

Please correct me if I'm wrong.

What I did.

• Restructured widget class hierarchy
• I simplified all the widgets by extending the base class
• Renamed widget 'id' to 'type' (it's more like in EntityDisplayBase style)

I didn't run any tests, so failures are possible.

Sorry @borisson_, you're absolutely right. I was wrong about class hierarchy. But I feel that the correct inheritance should be:

WidgetPluginBase
- LinksWidget
-- CheckboxWidget
- DropdownWidget

Please correct me if I'm wrong.

No worries, That is the correct inheritance, that looks like it makes more sense.

What I did.

Restructured widget class hierarchy
I simplified all the widgets by extending the base class
Renamed widget 'id' to 'type' (it's more like in EntityDisplayBase style)

That looks great, I agree with these changes.

I didn't run any tests, so failures are possible.

Do you think you have time to look at the remaining test failures or do you want me to have a go at them?

Patch doesn't apply anymore either.

Patch applies again now. Test still fail though, looking at those now.

This should fix at least some of the failures.

This should fix the remaining fail in the Integrationtest, I'll have a look at the last unit test fail after lunch.

Attached patch should be green.

This now needs another thorough review before we can commit this.

Reroll

(Three way merge required for Entity and manual merge for Dropdown Widget - there was translation added to the defaults, I've added this back, not sure if that's correct yet).

1. +++ b/src/Entity/Facet.php
   @@ -40,7 +40,6 @@ use Drupal\facets\FacetInterface;
   - *     "widget_configs",
   

   add widget_config to the FacetInterface here.

2. +++ b/src/FacetInterface.php
   @@ -10,25 +10,37 @@ use Drupal\Core\Config\Entity\ConfigEntityInterface;
   +   *   An associative array with the next structure:
   

   "with the following structure"

3. +++ b/src/FacetInterface.php
   @@ -10,25 +10,37 @@ use Drupal\Core\Config\Entity\ConfigEntityInterface;
   +   *   - config: (array) The widget configuration.
   

   isn't it called "configuration" instead of config. And I think you mentioned it is optional?

4. +++ b/src/FacetListBuilder.php
   @@ -100,7 +100,7 @@ class FacetListBuilder extends ConfigEntityListBuilder {
   +            '#suffix' => '<div>' . $entity->getFieldAlias() . ' - ' . $entity->getWidget()['type'] . '</div>',
   

   This will break if the getWidget() function doesn't return an object. We should verify if that value exists before referring to the type. Unless we're 100% sure the getWidget cannot fail? :)

   Also rename to $facet, we're using $facet everywhere else don't we?

5. +++ b/src/Form/FacetForm.php
   @@ -105,29 +104,32 @@ class FacetForm extends EntityForm {
   +    $widget_plugin_id = $form_state->getValue('widget') ?: $facet->getWidget()['type'];
   

   Same concern here. Don't we need to check the getWidget object actually exists first and fail appropriately?

6. +++ b/src/Plugin/facets/widget/LinksWidget.php
   @@ -19,174 +15,41 @@ use Drupal\facets\Widget\WidgetInterface;
   +    return ['soft_limit' => 0] + parent::defaultConfiguration();
   

   Is the configuration that is allowed documented somewhere?

7. +++ b/src/Plugin/facets/widget/LinksWidget.php
   @@ -19,174 +15,41 @@ use Drupal\facets\Widget\WidgetInterface;
   +      '#default_value' => $this->getConfiguration()['soft_limit'],
   

   same concern here.

8. +++ b/tests/src/Unit/Plugin/widget/DropdownWidgetTest.php
   @@ -49,7 +51,12 @@ class DropdownWidgetTest extends UnitTestCase {
   +    $container = $this->prophesize(ContainerInterface::class);
   

   Can we add some comments to explain what this is doing?

Other than that, this change looks great. Awesome that we no longer have to extend a linkswidget to create a dropdownwidget ;-)

Fixed broken tests,

regarding #46:
.1, we store everything in the widget now, the config as well, so we don't have to remove that.
.2 Fixed
.3 It's called config in the .schema.yml, configuration is not optional, it's always available but can change per widget.
.4 a facet without a widget is broken anyway, let's not code for that case because everything will stop working anyway. I did the rename to $facet.
.5 see .4
.6, .7 See .3, all configuration can be allowed, as long as it's provided with a schema.
.8 Sure, added a comment.

If the bots and @Nick_vh approve, we can commit this today.

Go testbot, go!

Go commit, Go!

Committed.

testFacets() still seems to have a reference to "widget_configs":

    // Create a facet, enable 'show numbers'.
    $this->createFacet('Owl', 'owl');
    $edit = ['widget' => 'links', 'widget_configs[show_numbers]' => '1'];
    $this->drupalPostForm('admin/config/search/facets/owl/edit', $edit, $this->t('Save'));

Had some trouble with this patch after re-exporting. See https://www.drupal.org/node/2755447.

Please ignore my last comment, I still had some old files locally.