The SearchPluginManager needs to provide an alter hook for its discovery

This looks OK so far, but definitely needs tests (I am not sure I get how it would work) and a couple of minor things fixed:

a)

+function hook_search_plugin_alter(&$definitions) {
+  if (isseet($definitions['node_search'])) {
+    $definitions['node_search'] = t('Nodes');
+  }
+}

- Sample code has a typo in issseeettttt :)

b) in search.api.php

\ No newline at end of file

c) In the alter hook:

+/**
+ * Alter search plugins.
+ *

Maybe we should say something more about what this means? Is it so you can substitute in a different plugin, or change something about the existing plugins? Maybe the sample code should have examples of both?

This is an example from one of the other plugin alters:

/**
 * Allow modules to alter tip plugin definitions.
 *
 * @param array $info
 *   The array of tip plugin definitions, keyed by plugin ID.
 *
 * @see \Drupal\tour\Annotation\Tip
 */
function hook_tour_tips_info_alter(&$info) {
  // Swap out the class used for this tip plugin.
  if (isset($info['text'])) {
    $info['class'] = 'Drupal\mymodule\Plugin\tour\tip\MyCustomTipPlugin';
  }
}

That seems to me to be a more realistic example, illustrating something a module might actually want to do.

Updated issue summary.

I guess I'm still a bit unsure as to whether or not we need this. I mean, if a module offers a Search plugin, does that mean they would want to eliminate the Node, User, or some other module's search plugin from even being offered as an option? It doesn't seem necessary, given that someone can choose to define one or more search pages based on any available plugins, and that they have full control over the URL and tab name and tab order now. It just doesn't seem so necessary for module A to alter module B's offering of search plugins. ??

I'm leaning towards marking this "won't fix" because I just don't think it's necessary.

I am still trying to figure out if we need this, and I still cannot figure out a use case.

The SearchPluginManager uses an Annotation-based discovery method. I looked at what AnnotatedClassDiscovery::getDefinitions() does, and it's returning something like

array(
   'plugin_id_1' => array( ... annotation info ... ),
   'plugin_id_2' => array( ... annotation info ... ),
) 

The only information that is in the annotation headers for NodeSearch and UserSearch plugins is the ID and the title. And I think that AnnotatedClassDiscovery::getDefinitions() also adds the class. So we'd have:

$definitions = array(
  'node_search' => array(
     'id' => 'node_search',
     'title' => 'Content', // actually translated, but whatever
     'class' => '\Drupal\node\Plugin\Search\NodeSearch', // might not start with \, I'm not sure
  ),
  // additional array elements for UserSearch, etc.
)

So... I guess that the alter hook would allow a module to override either the title or the class for the 'node_search' plugin.

Changing the label seems like not a huge reason to offer an alter hook. (That is more or less the example that is in the hook body in the existing patch.) If that is the only reason, I think we should just "won't fix" this. The label is only shown to admins when they are adding search pages on the Search config page, and I don't see a strong reason to let contrib modules override this.

So the only other reason would be to let contrib modules override the class, or maybe to completely disable an existing plugin by removing it from the list.

That would theoretically allow a contrib module, such as "Better Node Search" to offer a plugin that would just take over the existing NodeSearch plugin. But again I don't see a compelling reason to let this happen. The thing is, Search plugins are now just providing admins the ability to define search pages, and having an extra one around doesn't really cause any harm. Besides which, it is quite likely that the contrib module offering "Better Node Search" also has different default configuration from what NodeSearch has, so they might not really be compatible.

So... my inclination is to say that each module can offer one or more Search plugins via annotation discovery, and that one module shouldn't be able to alter another module's ability to offer search plugins, since the admin can choose to or not to define search pages based on each type of available search plugin.

I think this issue was created back when plugins defined pages directly, before we added the ability to have one or more pages based on each plugin -- and then this would have been more necessary.

Anyway... Here is a patch that removes the @todo line that suggests that we need an alter hook.

If you disagree and think we do need an alter hook, we need to go back to the previous patch and:
a) Reroll so it would apply to current code.
b) Fix it so the sample hook function body actually makes sense and is valid PHP and would work with the structure of $definitions that we currently have
c) Document the structure of $definitions more in the alter hook (maybe with the array above?)
d) Write tests for the alter hook

And please explain why you think this is necessary.

That would theoretically allow a contrib module, such as "Better Node Search" to offer a plugin that would just take over the existing NodeSearch plugin. But again I don't see a compelling reason to let this happen.

Yes, that is the use case for this. It's why we have a *pluggable* system. We cannot anticipate the needs of contrib or custom code, but not providing an alter is equivalent to us declaring the search.module perfect for all time and that no improvements would ever need to be made.

Every other plugin manager has either an alter hook or an issue to add one.

It doesn't seem like just because all other plugins have this, that it necessarily makes a lot of sense here, for the reasons outlined above... The alter hook here would only be changing what's available on one drop-down list in the UI, where you add new search pages. It seems kind of silly to go to the trouble, even if most other plugin managers have this ability.

Or ... possibly it would also alter existing search pages? I guess maybe it would. So if I had already configured a page using the 'node_search' plugin, and I then enabled a module that used this alter hook to replace the default NodeSearch class with a different class for the plugin identified as 'node_search', would my existing configured NodeSearch page then use the new class? In that case, this seems dangerous -- the config might not be compatible... We should put a note to that effect in the alter hook documentation if we do this.

Anyway... If we're going this route, because we apparently have to or it's an undocumented Drupal standard to do so, we need to fix up the original patch. I've updated issue summary with the list of what needs to be done, and hidden my simpler patch that just removed the @todo.

Incidentally, I do not think this is really the same as other plugins that might really need alter hooks.

The list of plugins is only visible to admins, and since any module can provide plugins, any module can give admins the ability to define search pages based on their plugin. And if a given plugin has no pages defined, it has no other effect on the site besides showing up in that one drop-down list as one that can be added as a search page.

So I don't think that not having an alter hook is really saying "NodeSearch and UserSearch are wonderful", all it is saying is "NodeSearch and UserSearch are available for you to use as search page plugins, but feel free to add more too". What, really, does letting modules alter this list buy us, that's worth the time of writing docs and tests to add this ability?

In general you could indeed replace the search plugin manager service and provide an alter hook if you really need one.
Given that the search module does not seem to be the most used API (maybe I am pesimistic here)
and you can't think of a reason it seems fine for me to not provide an alter hook. More important: All you can override in the plugin definition
is the used class, id and title. This is much less than other plugins provide.

I rerolled the patch in comment #1. going to move it to 'needs review' to see what the test bot makes of it

The API had changed, fixed the call.

RE #12... As I have asked multiple times on this issue already...

Can you please explain to me why a module would even need to replace a plugin class? The plugin class is not used directly on the site, unless an admin configures a page using that plugin. So if module A provides a plugin, and module B wants to provide a similar plugin, I don't see any reason for module B to replace module A's plugin with its own -- it can just provide the similar plugin, and the admin can use module B's plugin to create a search page.

So I don't get the need for this.

If we do need it... the patch in #16 does need some work:
- The documentation is not following our standards (@param descriptions indentation, etc.)
- The example body cannot be right
- Needs more documentation on the structure that the hook would get in $definitions

Oh and it would definitely also need a test.

As far as I know, this is the only plugin manager without an alter hook.
Plugins are supposed to be swappable. That's one of the basic tenets of the plugin system.

Additionally, this does *not* need dedicated tests.
If you look at the patch, you see we're calling an existing method on the parent class, the same as every other manager. This method and its underlying mechanism have existing test coverage, and we don't need to provide per-plugin type tests for all 2 dozen in core.

I don't like alters on plugins but its how we've decided to use them. Makes things consistent.

hey @tim.plunkett - i #19 you added a comment to add an alter hook. i was going to try to do that (still a novice), I looked through half a dozen other classes that extend DefaultPluginManager to try to get a sense of what is supposed to happen, but I dont see anything. Could you give me a pointer?

Also, shouldnt something like that be handled in the base class, anyway?

The most recent patch removes the comment. It matches the other managers.
The base class cannot handle this because we must pick the alter name ourselves. The alterInfo() method handles the rest.

Is it possible we could get some more information into the hook documentation:

+ * @param array $definitions
+ *   The array of search plugin definitions, keyed by plugin ID.

If I wanted to use this hook, pwolanin says the main reason would be to swap out the plugin class. The documentation and the example do not tell me how to do this. I'd have to ... I don't know even where I would look to figure out what "search plugin definitions" are. There is no @see reference or information here, and the example just shows me how to swap out the title of the plugin, which is not all that useful,

The data structure is self documenting and this is a core way that plugins work. I don't think we should have to document this in every location, that defeats the reason to use a standard system.

Um. Self-documenting... really? Where would I find information about it? What's the key for the plugin class, and what are the other keys in the information array?

Honestly I have no idea where I would look to find this information. There is no link to the "standard" way of doing things that I can see, and as someone new to the plugin system who just wants to provide a search plugin to override the Core plugin (which I've been told is why we need this hook), I do not think it would be obvious or self-documenting.

@see \Drupal\search\Annotation\SearchPlugin
That has the keys that are available, and its parent explains the rest, like 'class' and 'provider'.

That was copied 1:1 from core:

$ grep -nr "The array of .* plugin definitions, keyed by plugin ID." *
modules/system/system.api.php:521: *   The array of local action plugin definitions, keyed by plugin ID.
modules/system/system.api.php:533: *   The array of local tasks plugin definitions, keyed by plugin ID.
modules/tour/tour.api.php:28: *   The array of tip plugin definitions, keyed by plugin ID.

Please, if you'd like to open a generic plugin issue, go ahead. But stop stonewalling this issue.

Sorry. I was not trying to be difficult, just trying to understand what someone would need to do. Thanks for the information.

As requested, filed separate issue to address the standard documentation problem.

is there anything else i can do to help with this issue? testing or something ...?

@AndyThornton this issue is just waiting for a committer, it is otherwise ready to go. Thanks!

19: search-2087965-19.patch queued for re-testing.

#2281905: Stop caching plugin discovery/info hooks by language

Easy re-roll.

Committed/pushed to 8.x, thanks!