Problem/Motivation

We need to store facet-source specific configuration e.g. the url processor configuration.

Why we need this

By default, the facets should use f[0]=article&f[1]=admin when 2 facets are present on a page (bundle / author).
When more than one facet source is used on a page, (e.g. block views) we should be able to configure a different parameter for facets.
An example of such a url could be f[0]=article&f[1]=admin&facet[0]=blog_item. We need to configure the key for these in a setting that is linked to a facetSource; that's where it belongs.

We'd like to avoid creating an entity just for that though, that would lead into a whole other world of problems (since all facetSources come from derivers we'd need to hook in to other modules's save events, ...).

Proposed resolution

We would like to create a default configuration that is overridable per facet source.
We can use this to store global facet source settings, the config will only exist for facet sources which do not use the default configuration.

Remaining tasks

  • Introduce a new configuration for the facetSource
  • Make sure that we're using the configuration in a facet
  • Change the UI to add the ability to change this the settings. (add an edit button per facet source)

User interface changes

We should have an edit button for a facet source.

Comments

borisson_ created an issue. See original summary.

borisson_’s picture

Title: URL Processor should have settings per facet source » Add a storage for facetSource specific config
Issue summary: View changes

Updated IS.

borisson_’s picture

Issue summary: View changes

Updated IS.

borisson_’s picture

Status: Active » Needs work

I started work on this yesterday and just committed and pushed that work to a new branch: http://cgit.drupalcode.org/sandbox-mikeker-2348769/log/?h=2612090--facet..., when I get further I will continue to push to that branch until I think this is ready for review.

borisson_’s picture

Issue summary: View changes

Updated IS with current progress and added a link to github's diff view for better reviewing https://github.com/nickveenhof/facetapi/compare/8.x-1.x...2612090--facet...

borisson_’s picture

Status update:

I created a new configuration for facet sources (config/schema/facetapi.facet_source.schema.yml) and a default setting (f) for filter_key, defined in that config. I added a new configuration for per facet source that saves into that config.

I also changed the QueryStringUrlProcessor (the only current url processor) to use that config for the filter key and I made sure the unit test for that processor still passes.
I also extended the QueryStringUrlProcessorUnitTest to ensure the newly added functionality works as expected.

Those pieces of the puzzle work. The next part of this issue however doesn't work yet.

I created a src/Config/FacetSourceConfigOverride and related events modelled after LanguageConfigOverride but I still have no idea how that actually integrates with our code.

You can look at the latest interdiff on github

borisson_’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
19.58 KB

So; a config override can override every config defined in the system, this is not something we need or want to do. Looks like we went overboard a little with that idea.

Attached is a working patch.

Nick_vh’s picture

  1. +++ b/src/Controller/FacetSourceConfigController.php
    @@ -0,0 +1,42 @@
    +   *   A renderable array containing the form
    

    Missing a dot?

  2. +++ b/src/Controller/FacetSourceConfigController.php
    @@ -0,0 +1,42 @@
    +      // Return a renderable array with a short lifetime that represents the plugin not found.
    

    Comment longer than 80 chars

  3. +++ b/src/Controller/FacetSourceConfigController.php
    @@ -0,0 +1,42 @@
    +      return ['#markup' => 'Plugin not found.', '#cache' => ['max-age' => 5]];
    

    If we do not provide the cache param, does that mean it might be cached indefinitely? Should we add that here so that people understand why?

  4. +++ b/src/Form/FacetSourceConfigForm.php
    @@ -0,0 +1,79 @@
    +    $overridden_filter_key_config = $config->get('overrides.' . $facet_source_id . '.filter_key');
    

    Is there a way how we can make this "pattern" more global so that we don't have to have internal knowledge where this is referenced but rather get a certain class variable / constant that defines what the pattern is? It's "antfucking" but I don't like it that we do this concatenation in multiple places.

  5. +++ b/src/Form/FacetSourceConfigForm.php
    @@ -0,0 +1,79 @@
    +      '#markup' => $this->t('Saving this form will create a configuration override for this specific facet source. Not doing so will make sure that Facet API uses the default settings. This is an advanced feature and unless you are fully aware of why you\'re creating this configuration. With a default configuration of Facet API you can leave the default configuration.')
    

    Isn't there a convention that we can wrap this text in multiple lines? It mentions "Unless you are fully aware of why etc... but it doesn't really mention what happens unless. Please rewrite to be more clear.

  6. +++ b/src/Form/FacetSourceConfigForm.php
    @@ -0,0 +1,79 @@
    +      '#default_value' => (!is_null($overridden_filter_key_config) ? $overridden_filter_key_config : $config->get('filter_key')),
    

    Should we check if it is_null? what if it is empty but not null?

  7. +++ b/src/Form/FacetSourceConfigForm.php
    @@ -0,0 +1,79 @@
    +    $configKey = 'overrides.' . $form_state->getValue('facet_source_id') . '.';
    

    Can this form value ever be invalid as a configkey string? Any sanitization we need to add?

  8. +++ b/src/Plugin/facetapi/processor/ExcludeSpecifiedItemsProcessor.php
    @@ -56,18 +56,18 @@ class ExcludeSpecifiedItemsProcessor extends ProcessorPluginBase implements Buil
    +    $config = $processors[$this->getPluginId()];
    

    How is this related to this patch?

  9. +++ b/src/Processor/UrlProcessorPluginBase.php
    @@ -41,10 +42,26 @@ abstract class UrlProcessorPluginBase extends ProcessorPluginBase implements Url
    +
    

    I'd remove this new line

  10. +++ b/src/Processor/UrlProcessorPluginBase.php
    @@ -41,10 +42,26 @@ abstract class UrlProcessorPluginBase extends ProcessorPluginBase implements Url
    +    $override = $facetSourceConfig->get('overrides.' . $facetSourceId . '.filter_key');
    

    Another "dark magic concatenation"

borisson_’s picture

FileSize
15.32 KB
7.99 KB

Attached a new update.

Fixes #8.1, .2, .3, .5

.4, .10 I made it a little bit more sane.

.6 I don't think so.

.7 It's a hidden field from a machine name. So it should always be valid.

I also removed some unrelated changes.

StryKaizer’s picture

  1. +++ b/src/Form/FacetSourceConfigForm.php
    @@ -0,0 +1,79 @@
    +    $form['description'] = [
    +      '#markup' => $this->t('Saving this form will create a configuration override for this specific facet source.
    +      Unless you have to override facet source specific configuration, you can leave this to the default settings.')
    +    ];
    

    Maybe this documentation is too technical and not really needed for sitebuilders, we could add it in the code though?
    In here we can maybe add something more basic like : "Global settings for this facet source and all facets belonging to it." or just leave it out.

  2. +++ b/src/Form/FacetSourceConfigForm.php
    @@ -0,0 +1,79 @@
    +      '#description' => $this->t('They key used for filtering in the URL, defaults to f.
    +      You should change this to something else if you expect to have multiple facet sources on one page.'),
    

    Maybe as description: "The key used in the url to identify the facet source. When using multiple facet sources you should make sure each facet source has a different filter key."

One global remark.
Using the overrides system creates only 1 file for facetsource configuration when exporting config.
This *might* make this configuration more conflict-prone. Not sure if this issue is big enough to change the design though, facetsource overrides shouldnt happen often?
It would be nice if every override lived in their own configuration file though ;)

StryKaizer’s picture

Status: Needs review » Needs work
borisson_’s picture

Status: Needs work » Needs review
FileSize
15.88 KB

Attached is a reroll, this also fixes the remarks from #10.

I discussed this with @swentel at the DUGBE sprint in antwerp and we figure this configuration is probably the best we can do. The configuration will not be conflictprone, because the config entities the depends on are also defined in code.

borisson_’s picture

FileSize
15.7 KB

Uploaded the wrong patch in #12.

borisson_’s picture

Issue summary: View changes

As I mentioned in #12, I discussed this storage with @swentel at the DUGBE sprint and I'd like to elaborate on that.

Currently, we're using one storage to save all the configuration for all of the facet sources. This is a lot easier as a solution compared to creating a config entity. One thing we need to keep in mind is that we have to remove configuration from the config-object when a plugin is removed.

I'm not sure how we should do that, and if we don't, how much this'll hurt us.

StryKaizer’s picture

Let's discuss this tonight.

IMO we can not allow stale configuration at all, so we will need to find out a clean way on how to delete overridden config for non-existant facet sources.

borisson_’s picture

Status: Needs review » Needs work

We discussed this at last night's hangout and decided that we should go for a config entity anyway.

borisson_’s picture

Patch has a config entity now

borisson_’s picture

Status: Needs work » Needs review
FileSize
855 bytes
32.77 KB

Reviewed my own patch again, there looks to be sufficient test coverage and everything works as expected.

Nick_vh’s picture

  1. +++ b/src/Entity/Facet.php
    @@ -371,6 +379,40 @@ class Facet extends ConfigEntityBase implements FacetInterface {
    +    // plugin, so we create on on the fly; trough magic.
    

    typo. Should be through. Also it mentions twice "on", probably should be create it on the fly

  2. +++ b/src/Entity/Facet.php
    @@ -371,6 +379,40 @@ class Facet extends ConfigEntityBase implements FacetInterface {
    +      [
    

    does Drupal code standards allow short-hand arrays here? I am still confused what the recommendation is.

  3. +++ b/src/Form/FacetSourceEditForm.php
    @@ -0,0 +1,90 @@
    +      // plugin, so we create on on the fly; trough magic.
    

    same typo

  4. +++ b/src/Form/FacetSourceEditForm.php
    @@ -0,0 +1,90 @@
    +    // Set the module handler. This is usually handled for use, but because of
    

    More English typos. "This is handled for us". Also I do think this needs more clarifications what "usually" means and where an example can be seen.

Nick_vh’s picture

Status: Needs review » Needs work
borisson_’s picture

FileSize
37.52 KB
1.32 KB

Fixed remarks made in #19.

  1. Thanks
  2. I think the recommendation is: use it all the time. I'll post a link to be sure
  3. Was a c/p, fixed it as well.
  4. I'm not actually sure what the reason is, it doesn't work without the setModuleHandler but this is the only Form in facets (and search_api) where I see us having to do this. I removed the comment.
borisson_’s picture

Regarding #19.2: #2135291: [Policy, no patch] PHP 5.4 short array syntax coding standards for Drupal 8 mentions no final statement; as far as I can see. So that means we can use short array syntax everywhere in facets. Search API has decided against this but facets hasn't.

We could do something similar in the PHP coding standards to say "We prefer [] in PHP code for creating an array".

borisson_’s picture

Status: Needs work » Needs review

Back to NR, as the latest patch addresses all issues.

  • borisson_ committed 79dfe11 on 8.x-1.x
    Issue #2612090 by borisson_: Add a storage for facetSource specific...
borisson_’s picture

Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

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