As discussed with borisson_, we want all getters / setters to be explicit, so they can be overridden if necessary. The first part was done in https://www.drupal.org/node/2645128 This issue is for the actual setOption and getOption functions.

Comments

ChristianAdamski created an issue. See original summary.

ChristianAdamski’s picture

ChristianAdamski’s picture

Status: Active » Needs review
borisson_’s picture

After a quick look at the patch, I found this:

  1. +++ b/src/Entity/Facet.php
    @@ -306,7 +304,7 @@ class Facet extends ConfigEntityBase implements FacetInterface {
    -      $processor_settings = $this->getOption('processors', []);
    +      $processor_settings =       $processors_settings = !empty($this->processor_configs) ? $this->processor_configs : [];
    

    There's 2 $processor_settings here.

I'll take a look in more detail later.

ChristianAdamski’s picture

The last submitted patch, 2: 2656226-2-remove-option-setter.patch, failed testing.

The last submitted patch, 2: 2656226-2-remove-option-setter.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 5: 2656226-5-remove-option-setter.patch, failed testing.

The last submitted patch, 5: 2656226-5-remove-option-setter.patch, failed testing.

ChristianAdamski’s picture

Schema update missing.

Status: Needs review » Needs work

The last submitted patch, 10: 2656226-10-remove-option-setter.patch, failed testing.

The last submitted patch, 10: 2656226-10-remove-option-setter.patch, failed testing.

ChristianAdamski’s picture

Title: Remove set|getOption from Facet Entity et al. » Remove set|getOption from Facet Entity et al. and fix schema
Status: Needs work » Needs review
FileSize
9.16 KB

All of the above + url_processor_handler schema and widget_configs property change from string to array by default.

Status: Needs review » Needs work

The last submitted patch, 13: 2656226-13-remove-option-setter-_-fix-schema.patch, failed testing.

The last submitted patch, 13: 2656226-13-remove-option-setter-_-fix-schema.patch, failed testing.

borisson_’s picture

  1. +++ b/config/schema/facets.processor.schema.yml
    @@ -40,3 +40,11 @@ plugin.plugin_configuration.facets_processor.count_limit:
    +# There are no settings intended. So probably always an empty array.
    

    There are no settings intended for this, this will probably always be an empty array but not having a schema available means the facet config is not correct. This is important because the tests use strict config validation.

  2. +++ b/src/Entity/Facet.php
    @@ -306,7 +304,7 @@ class Facet extends ConfigEntityBase implements FacetInterface {
    -      $processor_settings = $this->getOption('processors', []);
    +      $processor_settings = !empty($this->processor_configs) ? $this->processor_configs : [];
    

    Looks great.

  3. +++ b/src/FacetInterface.php
    @@ -230,6 +183,14 @@ interface FacetInterface extends ConfigEntityInterface {
    +   * Set the query operator.
    

    From the docs: https://www.drupal.org/coding-standards/docs

    Summary lines must start with a third person singular present tense verb, and they must explain what the function does. Example: "Calculates the maximum weight for a list."

    This should be Sets the query operator.

    This is a rule that I tend to forget myself as well ;)

Otherwise this patch is looking great, hopefully the testbot agrees.

ChristianAdamski’s picture

Testbot does not agree. But at least he is a lot more verbose about it. Now I don't know why "show_numbers" is there...

ChristianAdamski’s picture

Additionally fixed:
- Minor issue in LinksWidget assuming $configuration['show_numbers'] to be set.
- camelCase vs snake_case in facet_source entity and schema

ChristianAdamski’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 18: 2656226-18-remove-option-setter.patch, failed testing.

The last submitted patch, 18: 2656226-18-remove-option-setter.patch, failed testing.

borisson_’s picture

borisson_’s picture

Status: Needs review » Fixed

Tests are green, happy with the code. Thanks for the patches!

Status: Fixed » Closed (fixed)

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