- Most Discovery::getDefinition($plugin_id) implementations chose to return array() when no definition for $plugin_id could be found.
That's the case for HookDiscovery, AnnotatedClassDiscovery, CacheDecorator.
StaticDiscovery returns NULL instead.

- Then, implementations that rely on the result of a decorated discovery to act on the results (PluginManagerBase, DerivativeDiscoveryDecorator) do something like :

<?php
public function getDefinition($plugin_id) {
 
$definition = $wrapped_discovery->getDefinition($plugin_id);
  if (isset(
$definition)) {
   
// Do something.
 
}
  return
$definition;
}
?>

If $plugin_id does not exist, and $wrapped_discovery->getDefinition($plugin_id) returns array(), the isset() check is true, and the definition gets massaged.

--> Consequence :
- PluginManagerBase::getDefinition('whatever') always returns a non-empty definition, containing PluginManagerBase::$defaults

Files: 
CommentFileSizeAuthor
#14 1778942-getDef-14.patch13.97 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 42,320 pass(es).
[ View ]
#14 1778942-interdiff-10vs14.txt1.52 KBandypost
#10 plugins-fix-getDefinition-1778942-10.patch13.8 KBtstoeckler
PASSED: [[SimpleTest]]: [MySQL] 42,143 pass(es).
[ View ]
#5 plugins-fix-getDefinition-1778942-5.patch3.04 KByched
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch plugins-fix-getDefinition-1778942-5.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#3 plugins-fix-getDefinition-1778942-3.patch1.85 KByched
PASSED: [[SimpleTest]]: [MySQL] 41,252 pass(es).
[ View ]
#1 plugins-fix-getDefinition-1778942-1.patch2.23 KByched
FAILED: [[SimpleTest]]: [MySQL] 41,245 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

Comments

yched’s picture

StatusFileSize
new2.23 KB
FAILED: [[SimpleTest]]: [MySQL] 41,245 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

I guess the choice of returning array() instead of NULL on a non-existent $plugin_id could be debated, however, sonce that's what most implementations do currently, I kept this.

Attached patch :
- makes StaticDiscovery return array() instead of NULL on non-existent $plugin_id
- fixes PluginManagerBase and DerivativeDiscoveryDecorator to not massage empty definitions.

yched’s picture

Status:Active» Needs review
yched’s picture

Status:Needs work» Needs review
Issue tags:-Needs tests
StatusFileSize
new1.85 KB
PASSED: [[SimpleTest]]: [MySQL] 41,252 pass(es).
[ View ]

Oh wait, right now DiscoveryTest::testDiscoveryInterface() actually explicitly tests :
$this->assertIdentical($this->testPluginManager->getDefinition('non_existing'), NULL)
This worked so far because TestPluginManager uses StaticDiscovery, which is the only one to have the correct behavior.

Then, switching the fix to
- makes getDefinition() return NULL instead of array() on non-existent $plugin_id
impacts HookDiscovery, AnnotatedClassDiscovery, CacheDecorator

tim.plunkett’s picture

Status:Needs review» Needs work
Issue tags:+Needs tests

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.

yched’s picture

Title:Discovery::getDefinition() implementations inconsistent on non-existent plugin_id» Discovery::getDefinition() / getDefinitions() : inconsistent return values for "no result"
Status:Needs review» Needs work
Issue tags:+Needs tests
StatusFileSize
new3.04 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch plugins-fix-getDefinition-1778942-5.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Actually, it's not only getDefinition().
There are also inconsistencies around getDefinitions() - when no definitions exist, most discoveries return an empty array, but HookDiscovery returns NULL.

Attached patch fixes that too :
- getDefinition() returns a definition or NULL.
- getDefinitions() returns the array of definitions, empty array if no definitions were found.
- Updated phpdocs in DiscoveryInterface to make the contract clearer.

This being said, and re: #4 - True, should be detected by tests, but I'm not sure what's the best way to test this. Do we need to write a full test suite on each discovery + decorator to get the fixes in ?

tim.plunkett’s picture

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

aspilicious’s picture

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

andypost’s picture

Hey, we have tests in DiscoveryTest.php

<?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

tstoeckler’s picture

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

tstoeckler’s picture

Status:Needs work» Needs review
StatusFileSize
new13.8 KB
PASSED: [[SimpleTest]]: [MySQL] 42,143 pass(es).
[ View ]

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.

andypost’s picture

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

Lars Toomre’s picture

Status:Needs review» Needs work

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.

Lars Toomre’s picture

Status:Needs work» Needs review

Whoops... Did not mean to change status.

andypost’s picture

StatusFileSize
new1.52 KB
new13.97 KB
PASSED: [[SimpleTest]]: [MySQL] 42,320 pass(es).
[ View ]

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

tstoeckler’s picture

Status:Needs review» Reviewed & tested by the community

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.

webchick’s picture

Status:Reviewed & tested by the community» Fixed

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!

tstoeckler’s picture

tim.plunkett’s picture

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.

tstoeckler’s picture

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()?

yched’s picture

getDefinitions() will always return an array.
getDefinition() returns NULL when no definition is found for the requested $plugin_id. This seems semantically more correct, there is no corresponding definition, not "the definition is an empty array" - that's what we do throughout core for somilar cases, I'd say.

You'd want to += the result of getDefinition whether the definition exists or not ? Seems odd, what's the use case ?

Besides, if definitions were classed objects (which they totally could have been) instead of D7-style arrays, null would definitely be the correct value.

tim.plunkett’s picture

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

Thanks @tstoeckler and @yched.

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

Anonymous’s picture

Issue summary:View changes

Typo