I think we initially talked about this, but somehow it got lost, or maybe we/you decided against it after all.
The thing is that there are several other modules, like Search API Saved Searches and Search API Autocomplete, which need a mechanism to find out about the different searches/search pages that exist for a given index (or in general). So I think it would be greatly benefitial for these modules to move some of this search page discovery mechanism from Facets into the Search API.

My plan here would be to have a "Search page" (or a better name, if someone comes up with one) plugin in the Search API, with a few search-specific methods like getIndex(), getPath(), getSearchId(), getResults() (with the option to execute the search if the results aren't there (yet)) and even isRenderedInCurrentRequest().

An implementation with a deriver to produce plugins for every Views page would of course be in the Search API, and you'd just have to change the code to have only a single class for all Search API facet sources, using that Search API plugin, and having a deriver that creates one facet source for every search page.

The large advantage for everyone would be larger adoption, since this would then live directly in the Search API, and other modules would be even more inclined to implement the plugin for their search pages. (I'd also be thinking about a supportsFeature() mechanism, like for backend plugins – that way, there would be a clear way of implementing support for things like Autocomplete, which needs specific search-specific support. However, it doesn't seem to me like your code actually needs that – just the generic information cited above, as far as I can see.)

Would you be willing to make such a change?
I think it would be quite easy, just the normal boilerplate code to write for a new plugin type, but other than that pretty clean. However, it would of course again be a disruption in compatibility, so it would be a change that should happen sooner rather than later.


(Also, completely unrelatedly, I think this project needs a few more issue queue "Components" – "Search API integration", "Core Search integration", and maybe others. Having not just the default four will surely be helpful in the long term, when the module and its usage grow.)

Files: 

Comments

drunken monkey created an issue. See original summary.

StryKaizer’s picture

This sounds great as search api sorts can also benefit from this.
We decided on not implementing seach api sorts, but since this part of the code is/was living in facets atm, it did make sense thinking about it.

+1 on moving the parts which make sense to search api.

Nick_vh’s picture

+1 - same reasoning as StryKaizer. Would love to see that in Search API so other modules can benefit from that. However, we should be careful not to brake current mechanisms in Facet API so the easiest way to do this is to keep the facet source in Facet API but make generic functions in Search API that are then called by the Facet Source. This might not be that easy and potentially becomes very complex.

We could also move over the complete facet source so that Search API can do this refactoring without breaking Facet API all the time. As long as there is a facet source available, Facet API is happy. So I suppose this is a 2 step issue. Step 1: Move the Facet Source, Step 2: Break out the Facet Source to generic functions and classes so that other modules can also use those without breaking Facet API.

borisson_’s picture

In moving over the plugin-type facetSource to search_api, we're going to be requiring modules that work with core search (core_search_facets) or views (core_views #2630896: Generic views support) to also rely on search_api.
I don't think this is a good idea to introduce that dependency.

So I think it's a -1 from me, unless I don't fully understand.

borisson_’s picture

drunken monkey’s picture

In moving over the plugin-type facetSource to search_api, we're going to be requiring modules that work with core search (core_search_facets) or views (core_views #2630896: Generic views support) to also rely on search_api.
I don't think this is a good idea to introduce that dependency.

So I think it's a -1 from me, unless I don't fully understand.

No, you misunderstood there. We don't want to move the plugin type, we want a new plugin type in the Search API that does more or less the same – provide information for a single search "page" (or whatever) and allow configuration of that. The facet source plugin type would stay in the Facets module, but the Search API implementation for it would just be a single class, with derivatives for all of the Search API "search page" plugins, that redirect all page-specific methods to the corresponding "search page" plugin.

And yes, Nick, the move would of course need to be coordinated. However, I still don't think that we need to move the facets source plugin to the Search API, nor that we should – seems that would make it harder, in turn, to make changes to the facet source plugin type afterwards. Also, I think there was a problem with the interdependence of the facet source and widget plugins?

borisson_’s picture

I know I understood, @Nick_vh and @StryKaizer explained why this is a good idea. I think we should do this. Do you think we can do this before this week's round of alpha releases?

StryKaizer’s picture

Project: Facets » Search API
Component: Code » Framework

Moving this to Search API, as the first action is in this module.

StryKaizer’s picture

I used the name "Display" instead of Search Page, as Views uses "Display" for something which relates the most to what we are implementing here.

Feel free to discuss and/or change that name ;)

Also, do we need a search_api_display_info_alter hook for this? Cant think of a usecase yet for that hook though.

borisson_’s picture

Status: Needs review » Needs work

I like the display name, that has my vote. Overall this looks great, I found a couple of smaller things.

@drunken monkey: is there any way we can write tests for this?

  1. +++ b/src/Display/DisplayDeriverBase.php
    @@ -0,0 +1,102 @@
    +namespace Drupal\search_api\Display;
    +
    +
    

    Should be only one blank line.

  2. +++ b/src/Display/DisplayDeriverBase.php
    @@ -0,0 +1,102 @@
    +  public function setEntityTypeManager(EntityTypeManager $entity_type_manager) {
    +    $this->entityTypeManager = $entity_type_manager;
    +    return $this;
    

    Can we typehint on the interface instead of the class here?

  3. +++ b/src/Display/DisplayInterface.php
    @@ -0,0 +1,55 @@
    +interface DisplayInterface extends PluginInspectionInterface, DerivativeInspectionInterface {
    

    I'm not sure if the interface should extend the DerivativeInspectionInterface. I think the base class should implement that interface instead.

  4. +++ b/src/Display/DisplayInterface.php
    @@ -0,0 +1,55 @@
    +   * Retrieves the index used by this display.
    

    /s/Retrieves/Returns/ for more consistency in the methods.

  5. +++ b/src/Display/DisplayInterface.php
    @@ -0,0 +1,55 @@
    +   * Returns the path used for this Display.
    

    /s/Display/display/

  6. +++ b/src/Display/DisplayPluginBase.php
    @@ -0,0 +1,60 @@
    + * - description: The human-readable description of the display class, translated.
    

    This exceeds 80 cols.

  7. +++ b/src/Display/DisplayPluginManager.php
    @@ -0,0 +1,40 @@
    +/**
    + * Manages data type plugins.
    

    Manages display plugins

  8. +++ b/src/Display/DisplayPluginManager.php
    @@ -0,0 +1,40 @@
    +  /**
    +   * Constructs a DisplayPluginManager object.
    +   *
    +   * @param \Traversable $namespaces
    +   *   An object that implements \Traversable which contains the root paths
    +   *   keyed by the corresponding namespace to look for plugin implementations.
    +   * @param \Drupal\Core\Cache\CacheBackendInterface $cache_backend
    +   *   Cache backend instance to use.
    +   * @param \Drupal\Core\Extension\ModuleHandlerInterface $module_handler
    +   *   The module handler.
    +   */
    +  public function __construct(\Traversable $namespaces, CacheBackendInterface $cache_backend, ModuleHandlerInterface $module_handler) {
    

    I think this can be changed to {@inheritdoc}

drunken monkey’s picture

Issue summary: View changes

Great job, thanks a lot! Looks quite good already.
Also, of course, thanks to Joris for the thorough review, as usual!

@drunken monkey: is there any way we can write tests for this?

Sure. E.g., we could add a check to ViewsTest to see if the corresponding display plugin for the view is available and has the correct information (label, description, path).

I'm not sure if the interface should extend the DerivativeInspectionInterface. I think the base class should implement that interface instead.

I disagree, we do the same for all the other plugin types as well. Doing it the other way, you'd need additional instanceof checks wherever you need to use such a method – and I don't think they're doing any harm just being there when they're not needed.

Also, I spotted a few other minor issues with the patch:

  1. +++ b/search_api.plugin_type.yml
    @@ -17,3 +17,8 @@ search_api_tracker:
    +
    +search_api_display:
    +  label: Search API display
    +  plugin_manager_service_id: plugin.manager.search_api.display
    +  plugin_definition_decorator_class: \Drupal\plugin\PluginDefinition\ArrayPluginDefinitionDecorator
    diff --git a/search_api.services.yml b/search_api.services.yml
    
    diff --git a/search_api.services.yml b/search_api.services.yml
    index 6c1207a..9a42b1c 100644
    
    index 6c1207a..9a42b1c 100644
    --- a/search_api.services.yml
    
    --- a/search_api.services.yml
    +++ b/search_api.services.yml
    
    +++ b/search_api.services.yml
    +++ b/search_api.services.yml
    @@ -26,6 +26,10 @@ services:
    
    @@ -26,6 +26,10 @@ services:
         class: Drupal\search_api\Tracker\TrackerPluginManager
         parent: default_plugin_manager
     
    +  plugin.manager.search_api.display:
    +    class: Drupal\search_api\Display\DisplayPluginManager
    +    parent: default_plugin_manager
    

    In both files, I think we currently have alphabetic ordering, which we should preserve here.

  2. +++ b/src/Display/DisplayDeriverBase.php
    @@ -0,0 +1,102 @@
    +    /** @var \Drupal\Core\Entity\EntityTypeManager $entity_type_manager */
    +    $entity_type_manager = $container->get('entity_type.manager');
    +    $deriver->setEntityTypeManager($entity_type_manager);
    

    I think the type hint isn't necessary (any more?) with PhpStorm and the Symfony plugin. Since that's the de-facto standard, I'd rather leave that out now.

  3. +++ b/src/Display/DisplayDeriverBase.php
    @@ -0,0 +1,102 @@
    +    return $this->entityTypeManager ?: \Drupal::service('entity_type.manager');
    

    I think there's ->entityTypeManager(), too, instead of using service().

  4. +++ b/src/Display/DisplayInterface.php
    @@ -0,0 +1,55 @@
    +  /**
    +   * Returns the label for use on the administration pages.
    +   *
    +   * @return string
    +   *   The administration label.
    +   */
    +  public function label();
    

    Why isn't there a getDescription() method as well? (Btw, ugly that we use label() instead of getLabel(), but I guess we're stuck with that now.)
    Or, if we don't want descriptions for displays, they should be removed from the annotation.

  5. +++ b/src/Plugin/search_api/display/ViewsPageDisplayDeriver.php
    @@ -0,0 +1,71 @@
    +      return [];
    

    Please use array() instead of []. Same in the rest of the code.
    It probably was the wrong decision back then, but I'd say the time to adapt the new style in this module is over now.

  6. +++ b/src/Plugin/search_api/display/ViewsPageDisplayDeriver.php
    @@ -0,0 +1,71 @@
    +              $machine_name = $view->id() . PluginBase::DERIVATIVE_SEPARATOR . $name;
    +
    +              $plugin_derivatives[$machine_name] = [
    +                  'id' => $base_plugin_id . PluginBase::DERIVATIVE_SEPARATOR . $machine_name,
    

    Is it really legal to have two colons in the plugin ID? It seems like this might cause issues in some cases. (Also see #2565045-6: Document valid plugin ID format.)

    On the other hand, come to think of it, you might need to have those in the Facet API then as well, right? If you want derivatives for all Search API displays, but they might already be derivatives themselves.
    Might need a clever solution there.

    For here, though, maybe just use a double underscore or something like that instead?

  7. +++ b/src/Plugin/search_api/display/ViewsPageDisplayDeriver.php
    @@ -0,0 +1,71 @@
    +                  'label' => $this->t('View name: %view_name. Display: %display_title', ['%view_name' => $view->label(), '%display_title' => $display_info['display_title']]),
    +                  'description' => $this->t('Provides a display.'),
    

    How about just 'View %view, display %display' instead for the label?
    Also, is there something better we can use for the description? At the very least, I'd change this to something like 'Represents the page display %display of view %view.' (i.e., a more verbose version of the label). But I think views also have descriptions, so maybe use that (if available) and just add the display name at the end?

But, as said, in general great work!

drunken monkey’s picture

Title: Move part of the Search API Views facet source to a new plugin type in the Search API? » Add a "display" plugin type to list all known search pages

Changing the title – the Facet API changes should then probably live in a new follow-up issue.

StryKaizer’s picture

Remark 6 from #11 still needs to be addressed.

StryKaizer’s picture

Using patch in #13 for search_api sorts to use displays instead of index for configuration.

Should we add getDisplay() to the QueryInterface, so modules (e.g. search_api_sorts) can detect which display fired the query?
Or is there another way to do this?

borisson_’s picture

Would it be enough to do $plugin->getPluginDefinition()['view_display'] instead @StryKaizer?

StryKaizer’s picture

@borisson_ The reason why I need a way to identify the Display which fired the Query, is because "Search api sorts" needs to do the correct query alter using hook_search_api_query_alter.

At this moment I can only do alters based on the index, not specificly target the Display.

drunken monkey’s picture

Status: Needs review » Needs work

Thanks a lot for your continued work on this!

  1. +++ b/src/Display/DisplayInterface.php
    @@ -0,0 +1,55 @@
    +  /**
    +   * Returns the path used for this display.
    +   *
    +   * @return string
    +   *   The path of the display.
    +   */
    +  public function getPath();
    

    Wouldn't it be better to return a Url object here? Just having a path, without even the possibility of GET parameters, seems a bit restrictive.

  2. +++ b/src/Display/DisplayPluginBase.php
    @@ -0,0 +1,60 @@
    + * - description: Human-readable description of the display class, translated.
    

    Displays don't seem to have a description at the moment.

  3. +++ b/src/Display/DisplayPluginBase.php
    @@ -0,0 +1,60 @@
    +abstract class DisplayPluginBase extends IndexPluginBase implements DisplayInterface {
    

    You can't use IndexPluginBase here, as that's always configurable. You'll need to include the index handling manually.

    Also, I guess in the case of displays, the index isn't passed through the plugin configuration, but determined by the plugin itself?

    For the default getIndex() implementation, maybe just check whether there is an 'index' key in the definition, instead? (Should then probably also be in the annotation/documentation.)

  4. +++ b/src/Display/DisplayPluginManager.php
    @@ -0,0 +1,70 @@
    +    if (!isset($this->DataTypes)) {
    +      $this->displays = array();
    

    Copy/paste error, it seems.

Would it be enough to do $plugin->getPluginDefinition()['view_display'] instead @StryKaizer?

I think he means Search API display, not the Views display.

And yes, @StryKaizer, I guess that would make a lot of sense. The question is, how to set it? Just a setter, or as a special option?
With an option, we wouldn't really need the method, just let people access the option directly, instead. A bit worse DX-wise, but on the other hand we can't (or, at least, don't) add separate methods for all known options.
What would you say?

In any case, though, being able to determine which display triggered a search query is of course important and sensible functionality.

borisson_’s picture

Attached patch fixes #17.1, .2 and .4. I haven't changed .3 yet.

drunken monkey’s picture

Status: Needs review » Needs work

Thanks, good work!
So only #11-6 and #17-3 need to be addressed now. (The former is pretty tricky, though. I've asked in #2565045-8: Document valid plugin ID format, maybe someone can help.)

drunken monkey’s picture

How about this? The solution for #11-6 here is pretty simple – just use a double underscore instead of a colon. Since the view and display IDs are already in the plugin definition, we don't really need the plugin ID to be parseable.
The only downside is that this might, in rare situations, lead to conflicts (i.e., when two displays on two different views have the same combined machine name, which means that one view ID and one display ID (of the other view) would have to contain double underscores, too – and even that would only be a problem in very specific circumstances) – but I think the chance is minimal enough to live with it. Otherwise, unless we want to risk the potential issues with two colons in a plugin ID (and I'd guess that would be considerably riskier), we'd probably have to keep a table where we store the "sanitized" plugin IDs, to make sure we always create the plugin ID for a certain display.

In any case, I'd say you can use the exact same mechanism on the Facets side, too – just take our display plugin IDs, replace any plugin derivative seperators (i.e., colons) with double underscores, and you should be fine.

Status: Needs review » Needs work

The last submitted patch, 20: 2648260-20--display_plugin.patch, failed testing.

The last submitted patch, 20: 2648260-20--display_plugin.patch, failed testing.

drunken monkey’s picture

borisson_’s picture

I think this is looking great, but I'd love to get sign-off from @StryKaizer

borisson_’s picture

I noticed that we're only supporting page displays for now. We recently started supporting blocks in facets as well - see http://cgit.drupalcode.org/facets/commit/?id=196a64e284b760e8d68b248fd67...

We can do that as a follow-up though.

I also found a couple of nitpicks that can be updated.

StryKaizer’s picture

Added the display to the query object using setOption

borisson_’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

Now that @StryKaizer confirmed that this works sufficiently, I'd say this is RTBC.

StryKaizer’s picture

Status: Reviewed & tested by the community » Needs work

Looks like isRenderedInCurrentRequest is not yet working.

StryKaizer’s picture

Added isRenderedInCurrentRequest for ViewsPageDisplay

borisson_’s picture

Status: Needs review » Reviewed & tested by the community

Looks great, no complaints here.

StryKaizer’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
1.15 KB
16.68 KB

Added getDescription to the interface and baseclass.

StryKaizer’s picture

StryKaizer’s picture

Not sure how I missed this. Now returning the correct Search api display plugin when using views (instead of the views display, which is irrelevant).

I also added a todo, since we should allow other modules to alter the views query to support new views display modes.
Anyone any idea how? A hook_search_api_views_displays_info() hook or any better ideas?

drunken monkey’s picture

Thanks, great job!
Still, a few more changes (it also needed a re-roll). Please see if you're OK with them.

Also, as a suggestion, should we maybe also just include the base path in the plugin definition? Then, I think, the plugin base class could also have default implementations for those two methods (and ViewsPageDisplay would be empty – but nothing wrong with that, really).

borisson_’s picture

Status: Needs review » Needs work
+++ b/src/Display/DisplayPluginBase.php
@@ -19,7 +18,8 @@
- *   label = @Translation("My display")
+ *   label = @Translation("My display"),
+ *   label = @Translation("My display"),

This doesn't look right

StryKaizer’s picture

Fixed current path check, so it works with language prefix too
Tried to inject the current path service with DI and failed miserably, so feel free to fix that.

Also removed double label from #35

borisson_’s picture

Status: Needs work » Needs review

Let's see how the bot feels.

borisson_’s picture

Status: Needs review » Reviewed & tested by the community

I think I can change this back to RTBC. I'm really happy with the current patch and this patch is required to make search_api_sorts work. We can't release an alpha there, before there's another alpha release for search_api that includes this patch.

drunken monkey’s picture

The second label was probably meant to become a description and I somehow lost track between duplicating the line and actually changing it.

Fixed current path check, so it works with language prefix too
Tried to inject the current path service with DI and failed miserably, so feel free to fix that.

Thanks, and did that (hopefully).

Please verify path detection still works correctly (since we don't have tests for that yet).

Status: Needs review » Needs work

The last submitted patch, 39: 2648260-39--display_plugin.patch, failed testing.

The last submitted patch, 39: 2648260-39--display_plugin.patch, failed testing.

drunken monkey’s picture

Status: Needs review » Needs work

The last submitted patch, 42: 2648260-42--display_plugin.patch, failed testing.

The last submitted patch, 42: 2648260-42--display_plugin.patch, failed testing.

drunken monkey’s picture

borisson_’s picture

Views …

:-)

  1. +++ b/src/Display/DisplayPluginBase.php
    @@ -30,6 +34,47 @@
    +   * @var CurrentPathStack|null
    

    This should be the FQDN

  2. +++ b/src/Display/DisplayPluginBase.php
    @@ -30,6 +34,47 @@
    +   * @return CurrentPathStack
    +   *   The current path service.
    

    Same here.

Other changes look good.

drunken monkey’s picture

Thanks for spotting that. So this should be RTBC?

This should be the FQDN

You probably mean "FQN". ;)

borisson_’s picture

I think this is rtbc, yeah. Not sure if we want to have @StryKaizer verify that this all works for search api sorts before committing this?

This should be the FQDN

You probably mean "FQN". ;)

I did, yes.

StryKaizer’s picture

Status: Needs review » Needs work
+++ b/src/Display/DisplayPluginBase.php
@@ -0,0 +1,121 @@
+      return $current_path == $plugin_definition['path'];

Should be:
return $current_path == '/' . $plugin_definition['path'];

drunken monkey’s picture

Oops, thanks a lot for catching that!
Then, if we always want to prepend a slash anyways, let's just prepend it right in the definition.

StryKaizer’s picture

Status: Needs review » Reviewed & tested by the community

Looking good!
Setting rtbc

borisson_’s picture

RTBC++

drunken monkey’s picture

Status: Reviewed & tested by the community » Fixed

Excellent. Committed.
Thanks again, everyone!

Status: Fixed » Closed (fixed)

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