Discovery::getDefinition() / getDefinitions() : inconsistent return values for "no result"

This is something that should have been caught in tests. Maybe more than one of the DiscoveryInterface implementations should be tested?
Otherwise, this makes perfect sense.

I'd say a unit test for AnnotatedClassDiscovery would be the most important.

YES, I want this. I had a discussion about this with eclipse.

Hey, we have tests in DiscoveryTest.php

    // Ensure that NULL is returned as the definition of a non-existing plugin.
    $this->assertIdentical($this->testPluginManager->getDefinition('non_existing'), NULL, 'NULL returned as the definition of a non-existing base plugin.');

Probably this just needs to be extended for this cases

+++ b/core/lib/Drupal/Core/Plugin/Discovery/HookDiscovery.php
@@ -37,13 +37,14 @@ class HookDiscovery implements DiscoveryInterface {
   public function getDefinitions() {
+    $definitions = array();
     foreach (module_implements($this->hook) as $module) {

This line is already in HEAD, it must have landed along with something else. Re-rolling for that, and also taking a stab at the tests. Thanks for the pointer @andypost, it seems DiscoveryTest was specifically designed for subclassing by Discovery implementations.

Here we go.

I modified DiscoveryTest (now DiscoveryTestBase) to not sub-class PluginTestBase. That was needless overhead and was implicitly testing all sorts of other plugin stuff like the plugin manager, etc. DiscoveryTestBase now acts as a base class for AnnotatedClassDiscoveryTest and StaticDiscoveryTest. HookDiscoveryTest cannot be tested for now, because these are unit tests. There might be a way to do this in the future, but that problem is completely unrelated to this patch and should not hold it up any longer.

Not providing an interdiff, because that would be identical to the hunks that add(/remove) files in the patch file.

+1 to RTBC, now we have a good coverage base for Discovery

Patch looks good. I noted one small issue:

+++ b/core/lib/Drupal/Component/Plugin/Discovery/DiscoveryInterface.phpundefined
@@ -20,7 +20,7 @@
    *
    * @return array
-   *   A plugin definition.
+   *   A plugin definition, or NULL if no definition was found for $plugin_id.
    */
   public function getDefinition($plugin_id);

This @return type hint needs to be updated as well to to 'array|null', I believe.

Whoops... Did not mean to change status.

Patch fixes hunk that depends on this issue, see #1785256: Widgets as Plugins
According coding standards fixed #12

Tentatively marking RTBC. The removed hunk makes total sense. The only thing I rolled for this patch are the tests, and those were RTBCed by @andypost in #11 and AFAICT by @Lars Toomre in #12. Since the actual code changes are trivial, I dare to be bold.

I confess I am not fully following everything going on here, but it seems to do what the issue title says on the tin, and test coverage is extensive.

Committed and pushed to 8.x. Thanks!

Shameless plug: #1809200: Consolidate discovery code in a DiscoveryBase and DiscoveryDecoratorBase

Hmm, the OP could have been solved by doing !empty, right?
This means that I can no longer += into an array with getDefinitions, because I can't trust that it is an array.

getDefinitions() is always an array, albeit perhaps an empty one.
getDefinition() is not always an array, it returns NULL if the plugin in question was not found.
Do you want to += on top of getDefinition()?

Sorry, my code was wrong, I did want getDefinitions() after all.
Glad I didn't reopen the issue :)

Thanks @tstoeckler and @yched.