Introducing the current search/summary block

What the difference between this and #2669556: Port Current Search Block functionality? I'll do a more in depth review tomorrow morning at the frontendunited sprint.

I like the code and the approach that you took to do this, I still don't see the difference between this and the current search block. I did see some things that I'd change. I'll ping StryKaizer to see if he has anything to say about this.

If you agree that this is essentially the same thing as the current search block, I'd love to take approach 2 and keep this in facets as a sub-module.

I did have some nitpicks.

1. +++ b/facets_source_summary/config/install/facets_source_summary.schema.yml
   @@ -0,0 +1,33 @@
   +    empty_message:
   +      type: string
   +      label: 'Message if there are no results'
   

   I see you used the empty_message as a string here, I think it'd be a good idea to use the same approach for the facet entity. However, we use empty_behavior there, we should probably use the same for both entities.

   Not sure which one we should use though. Any preference?

2. +++ b/facets_source_summary/src/Plugin/Block/SourceSummaryBlock.php
   @@ -0,0 +1,172 @@
   +  /**
   +   * The associated facets_source_summary entity.
   +   *
   +   * @var \Drupal\facets_source_summary\SourceSummary\SourceSummaryInterface
   +   */
   +  protected $entity;
   

   Can we move this up, so this is above the methods in the class?

3. +++ b/facets_source_summary/src/Plugin/Block/SourceSummaryBlock.php
   @@ -0,0 +1,172 @@
   +    // There are no results and count is not supported or we don't want to show
   +    // the total count.
   +    else {
   +      return [];
   +    }
   

   I'd love it if we can put that return as early as possible so we can avoid the else here.

4. +++ b/src/FacetManager/DefaultFacetManager.php
   @@ -166,11 +166,11 @@ class DefaultFacetManager {
   -  protected function getFacetsByFacetSourceId($facetsource_id) {
   +  public function getFacetsByFacetSourceId($facetsource_id) {
   

   This change will clash with a recent change in #2722783: Deleting a facet source leads to a fatal error on trying to edit facets associated with that facet source. Just letting you know.

5. +++ b/src/Plugin/facets/facet_source/SearchApiBaseFacetSource.php
   @@ -9,6 +9,7 @@ use Drupal\facets\FacetInterface;
    use Drupal\search_api\FacetsQueryTypeMappingInterface;
   +use Drupal\views\Views;
    use Symfony\Component\DependencyInjection\ContainerInterface;
   

   Did we need this change?

I discussed this a little with StryKaizer already, we did see that this is the same as the current search module. Closed that issue in favor of this one.

Hi, thanks for starting to work on this. This is actually the "Current Search Blocks" functionality as was a submodule in facet_api d7. We had some discussion already concerning this so I'll list a few things about what we had in mind here.

We want all features which facet_api d7 offered to be ported, and make it pluggable for new features.
See #2669556: Port Current Search Block functionality and #1375674: How do I use the new Current Search Blocks module?

Some missing features in current patch are:

- Allow to show full_text search (currentsearch-item plugin, see below)

- Allow block display setting following these rules
-- Display only when keywords are entered.
-- Display on empty searches where no keywords are entered.
-- Display only when one or more facet items are active.
-- Display when either keywords are entered one or more facet items are active.

- Create a new plugintype which allows creation of new currentsearch-items and ship with at least 2 default ones, being "Custom Text" and "Active items", see facet_api d7

- Show currentsearch items in facet overview page grouped by facetsource, under or above the facets.

- I prefer renaming the submodule to "Current search blocks" (what d7 used) instead of "Facets Source Summary" to not confuse people. (EDIT: lets discuss this, summary on the other hand is a good name ;-) )

- Allow creation of multiple summary blocks per facet source, this is needed if people want different summaries depending on e.g. the display setting, so lets use a config entity for current_search, similar to a facet, instead of 1 default block.

- Allow deselection on facet-items in the current_search block, e.g. MyFacet (x)

I understand that moving to config entities instead of 1 default block per facet is a big change, but we really need to cover as much use-cases here.

@claudiu.cristea
No problem at all, thanks for starting on this! I'll start working on the current search functionality and will let you know when I need a code review ;-)

Back to NW based on #13. Starting on this now.

So, I tried to work on this yesterday / today. It looks like this isn't as easy as I thought and I don't really understand everything very well either. So attached patch is WIP but not planning to do more work on this today.

@claudiu.cristea, I started from scratch again, so no interdiff is available. I figure I probably won't do any work on this @ dev days anymore. The patch also doesn't do anything yet (nothing really works except saving the config entity).

Rerolling

There were some more basic failures in there. This now allows creation and deletion of those facet summary blocks. Will try to figure out now what else we need to copy from the work that was done earlier to make this work.

As discussed in milan, Nick will merge in all functionality from #12 into the last patch.

Progress. Able to store the config entities already. Next up is the derived block & checkup of the functionality. Review is already welcome but will need some more work to get it back to the functional state of #12

Blocks are being derived. I have to stop now but the work remaining is to create a build processor to add the counts. What also needs to happen is to make it pluggable so that we can add different types of current search items as mentioned by @strykaizer. Right no wit has the "old" facets logic in the patch but that is not being saved yet.

Tried patch in #26which threw following error:
Fatal error: Call to a member function loadMultiple() on null in /path/to/facets/facets_summary/src/Plugin/Block/FacetsSummaryBlockDeriver.php on line 60

@Nick_vh: am I missing something?

Also small note, at this moment current_search is a submodule but is used in facetlistbuilder anyway, which throws an error when not enabled. We should fix that too in this patch.

There seems to be a typo in #26:

class property:

  /**
   * The entity storage used for facets summaries.
   *
   * @var \Drupal\Core\Entity\EntityStorageInterface $facetsSummaryStorage
   */
  protected $facetsSummaryStorage;

in create:

    $deriver->facetStorage = $container->get('entity_type.manager')->getStorage('facets_summary');

but in getDerivativeDefinitions:

      $all_facets_summaries = $this->facetsSummaryStorage->loadMultiple();

The patch no longer applies.

No more failures at least. Now we're ready to move on

The blocks show up again but now the facets don't want to appear. Debugging slowly but feel free to take a look already at the general concepts here.

The following patch is in a working state and has 3 processors on top of the actual current search facets:

• Hide when not rendered processor
• Show text when there are no facets
• Show a brief summary per facet
• Show counts next to the facet items
• links to remove the active facets, using the same behavior as facets

What it doesn't do:
No support for ordering

I'm quite happy with how this turned out after DrupalCon Dublin. Please take a look and try it out. I'm sure there are still bugs in here and cleanup todo.
Also I tried to install it from scratch and use it, so hopefully this also works for someone else.

Before I continue, I'd like some review and someone actually testing this.

Some of my thoughts of what still needs to happen:

We need the capability to style the results in the same way as regular facets. Since the facet results are an array of Results this is actually quite easy to do. Same for the sorting of them, and this way we could reuse email existing processors and potentially even the widgets in facets so that we do not have to duplicate effort. However, I would like the current functionality to go in first as experimental because it is in a working phase and architecturally I don't think it prevents any of this to happen. I started to duplicate the sort processors initially but it felt wrong.

Obviously the test failures need to be resolved as well :-)

Added a count processor but the count coming back from the query doesn't seem to be accurate. Could be a Search API bug.

@Nick_vh #2809753: getFacets in search_api_db returns wrong resultset might be the bug you're seeing.

This should fix the testbot. At least they pass locally... Let's see. Also, the count should be fixed, but I'm really surprised I needed a global variable for that. This is weird, but apparently that's how core does it.

Woot, passing. Can someone do the code review? There are no tests yet so I'd like to discuss a strategy of getting this in. Most of it is a separate module so what if we get that in and classify it as experimental? Perhaps a small integration test? I'm also good in leaving this here in the queue but I won't have time to write the tests for it unfortunately :(

I'll do a review before tomorrow's hangout for this. We could do this as an experimental module - that sounds like a good idea. Not sure if that's hard do for us.

Congrats on passing tests!

Happy with the direction here.

Some nitpicks:

1. +++ b/facets_summary/src/Entity/FacetsSummary.php
   @@ -0,0 +1,293 @@
   +  protected $facet_source_id;
   ...
   +  protected $facet_source_instance;
   

   I think these should be camel-cased to pass coding standards.

2. +++ b/facets_summary/src/Entity/FacetsSummary.php
   @@ -0,0 +1,293 @@
   +    if (!$this->facet_source_instance && $this->facet_source_id) {
   

   What happens when no id is set? it will return null?

3. +++ b/facets_summary/src/Entity/FacetsSummary.php
   @@ -0,0 +1,293 @@
   +  public function getProcessorConfigs() {
   +    return !empty($this->processor_configs) ? $this->processor_configs : [];
   +  }
   

   Can we default this to an empty array in the definition instead?

4. +++ b/facets_summary/src/Entity/FacetsSummary.php
   @@ -0,0 +1,293 @@
   +    // Get a list of all processors for given stage.
   +    foreach ($processors as $name => $processor) {
   +      if ($processor->supportsStage($stage)) {
   +        if (!empty($processor_settings[$name]['weights'][$stage])) {
   +          $processor_weights[$name] = $processor_settings[$name]['weights'][$stage];
   +        }
   +        else {
   +          $processor_weights[$name] = $processor->getDefaultWeight($stage);
   +        }
   +      }
   +    }
   +
   +    // Sort requested processors by weight.
   +    asort($processor_weights);
   +
   +    $return_processors = array();
   +    foreach ($processor_weights as $name => $weight) {
   +      $return_processors[$name] = $processors[$name];
   +    }
   +    return $return_processors;
   

   I know this is how the DefaultFacetManager does this - but since then we've learned so much. Would it be better to change this with an array_filter call to reduce complexity?

5. +++ b/facets_summary/src/FacetsSummaryInterface.php
   @@ -0,0 +1,139 @@
   +   * @param array $facets
   

   /s/array/array[]/ to be compatible with the declaration in getFacets?

6. +++ b/facets_summary/src/FacetsSummaryInterface.php
   @@ -0,0 +1,139 @@
   +   *   An associative array keyed by facet id and having arrays as values with
   +   *   the next structure:
   +   *   - facet_id: (string) The facet entity id.
   +   *   - show_count: (bool) If the source count will be displayed in the block.
   +   *   - prefix: (string) Prefix of facet group.
   +   *   - suffix: (string) Suffix of facet group.
   +   *   - separator: (string) Separator for facet items.
   

   Let's just point to getFacets instead, this doesn't really add anything to be in there twice and we can only forget to updated one of them if this changes.

7. +++ b/facets_summary/src/FacetsSummaryManager/DefaultFacetsSummaryManager.php
   @@ -0,0 +1,176 @@
   + * The facet manager.
   

   Facet summary manager?

8. +++ b/facets_summary/src/FacetsSummaryManager/DefaultFacetsSummaryManager.php
   @@ -0,0 +1,176 @@
   + * The manager is responsible for interactions with the Search backend, such as
   + * altering the query, it is also responsible for executing and building the
   + * facet. It is also responsible for running the processors.
   

   I think this is a c/p remnant, should it be updated?

9. +++ b/facets_summary/src/FacetsSummaryManager/DefaultFacetsSummaryManager.php
   @@ -0,0 +1,176 @@
   +class DefaultFacetsSummaryManager {
   ...
   +  public function build(FacetsSummaryInterface $facets_summary) {
   

   I noticed that building here happens after sorting, we do that the other way around in the defaultFacetManager, any reason?

10. +++ b/facets_summary/src/FacetsSummaryManager/DefaultFacetsSummaryManager.php
    @@ -0,0 +1,176 @@
    +   * Builds a facet and returns it as a renderable array.
    ...
    +   * This method delegates to the relevant plugins to render a facet, it calls
    +   * out to a widget plugin to do the actual rendering when results are found.
    +   * When no results are found it calls out to the correct empty result plugin
    +   * to build a render array.
    ...
    +   * Before doing any rendering, the processors that implement the
    +   * BuildProcessorInterface enabled on this facet will run.
    ...
    +   *   The facet we should build.
    ...
    +   *   Facet render arrays.
    ...
    +   * @throws \Drupal\facets\Exception\InvalidProcessorException
    +   *   Throws an exception when an invalid processor is linked to the facet.
    

    Same here.

11. +++ b/facets_summary/src/Form/FacetsSummaryForm.php
    @@ -0,0 +1,420 @@
    +        $form['weights'][$stage]['order'][$processor_id]['#attributes']['class'][] = 'search-api-processor-weight--' . Html::cleanCssIdentifier($processor_id);
    ...
    +              'search-api-processor-weight-' . Html::cleanCssIdentifier($stage),
    

    I think this shouldn't be search-api

12. +++ b/facets_summary/src/Form/FacetsSummaryForm.php
    @@ -0,0 +1,420 @@
    +    drupal_set_message(t('Facets Summary %name has been updated.', ['%name' => $facets_summary->getName()]));
    

    /s/t(/$this-t(

13. +++ b/facets_summary/src/Form/FacetsSummarySettingsForm.php
    @@ -0,0 +1,272 @@
    +      list(, $view_id, $display) = explode(':', $facet_source_id);
    

    We've refactored this for the other plugins to be ":","__" as to only have one colon in the plugin id, for consistency with drupal core. Let's change that here as well.

We should only add tests for the facet-changes for now, other tests can be added as a followup.
getCount is the only actual change - needs thorough testing.
For the experimental module; we should handle this the same way as core handles experimental modules. (package: Facets (Experimental))

As mentioned, I ran this trough phpcs this is the result.

This looks great already, thanks all for your work!!

A few nitpicks
- Since #2798483: Move submodules into a modules folder is in, lets move this into the /modules folder
- We probably need processors on this. e.g. when term id's are indexed and transformed into labels using processors in facets, term id's are shown when using "Show a summary of all selected facets".
- Lets set the "empty text behaviour" default disabled. Most usecases will not show anything when there are no filters imo.
- We can probably do with a few less twig templates. Maybe it would also be nice to have the facet machine name (or url identifier) as a class available to group results visualy?
- At this moment there seems to be no support for non-facet queries. D7 also supports search string. Would be nice to have this too.
- Feature request: It would be nice if there's also an option to remove all active filters

Back to needs work per #62 and #59

When enabling the patch and adding a new facet summary, the following warnings occur on 8.2.1:

Warning: array_keys() expects parameter 1 to be array, null given in Drupal\facets_summary\Form\FacetsSummaryForm->form() (line 142 of modules/contrib/facets/facets_summary/src/Form/FacetsSummaryForm.php).

Warning: in_array() expects parameter 2 to be array, null given in Drupal\facets_summary\Form\FacetsSummaryForm->form() (line 145 of modules/contrib/facets/facets_summary/src/Form/FacetsSummaryForm.php).

Also, when displaying the summary block, all facets disappear.

Anyone tested this with search_api_solr yet?

Update: in the UI when unselecting a facet under "ENABLED FACETS" then saving, the facet reappears selected in the list.

When I create a block in the interface, I receive a white screen with the following error message.
Any idea where this could come from?

The website encountered an unexpected error. Please try again later.
Error: Class 'Drupal\facets\Form\SubFormState' not found in Drupal\facets_summary\Form\FacetsSummaryForm->form() (line 228 of modules/contrib/facets/facets_summary/src/Form/FacetsSummaryForm.php).
Drupal\facets_summary\Form\FacetsSummaryForm->form(Array, Object) (Line: 115)
Drupal\Core\Entity\EntityForm->buildForm(Array, Object)
call_user_func_array(Array, Array) (Line: 514)
Drupal\Core\Form\FormBuilder->retrieveForm('facets_summary_edit_form', Object) (Line: 271)
Drupal\Core\Form\FormBuilder->buildForm('facets_summary_edit_form', Object) (Line: 74)
Drupal\Core\Controller\FormController->getContentResult(Object, Object)
call_user_func_array(Array, Array) (Line: 123)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 574)
Drupal\Core\Render\Renderer->executeInRenderContext(Object, Object) (Line: 124)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->wrapControllerExecutionInRenderContext(Array, Array) (Line: 97)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}()
call_user_func_array(Object, Array) (Line: 144)
Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object, 1) (Line: 64)
Symfony\Component\HttpKernel\HttpKernel->handle(Object, 1, 1) (Line: 57)
Drupal\Core\StackMiddleware\Session->handle(Object, 1, 1) (Line: 47)
Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object, 1, 1) (Line: 99)
Drupal\page_cache\StackMiddleware\PageCache->pass(Object, 1, 1) (Line: 78)
Drupal\page_cache\StackMiddleware\PageCache->handle(Object, 1, 1) (Line: 47)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object, 1, 1) (Line: 50)
Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object, 1, 1) (Line: 23)
Stack\StackedHttpKernel->handle(Object, 1, 1) (Line: 652)
Drupal\Core\DrupalKernel->handle(Object) (Line: 19)

I committed #2813707: Use core's subform state instead of our own. over the weekend - looks like the changes made there don't agree with this patch. This will need a refactor to use core's implementation of subformstate instead of our own old one.

I don't think that I can help with refactore the module as I'm not deep enough in module development. Is there a way one could help you with this problem?

I haven't tested the patch, but this should do the refactoring required. (The branch for this issue was 25 commits ago so the rebase might've been a long shot - since the patch size looks similar I have faith git did a good job)

Fail was unrelated.

Rebased on the latest changes.

Moved to subfolder, as discussed.

Committed this as experimental for now as we discussed.

Have a problem with the summary block, created a new issue for it - does anybody else have that problem? Error when creating a summary block

This works fine. Why it still added as Experimental?