Add a "display" plugin type to list all known search pages

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?

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!

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

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.

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.)

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.

That one was on you, Joris! :P

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).

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).

Views …

Then maybe like this?

Thanks for spotting that. So this should be RTBC?

This should be the FQDN

You probably mean "FQN". ;)

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.

Excellent. Committed.
Thanks again, everyone!