Improve exception messages when plugins cannot be found

I agree with the things said here.

So yeah, as discussed on IRC, we do have valid runtimes cases where we want to be able to query a non-existing plugin_id - like "does this plugin exist ?".
That's what field_info_widget_types($widget_plugin_id) does, and it currently returns NULL - switching that to "throws an exception woudl be a non minor API change".
Also, WidgetPluginManager::getInstance() checks whether the requested plugin_id exists and switches back to the default widget if not.
For all those cases, we agreed that we don't really want to bring "throw exceptions / catch them and handle" to become a regular pattern in "normal" runtime execution.

But as sdboyer points, for other cases - notably when the retrieved definition is passed to a factory, passing NULL only causes cryptic exceptions down the line, and raising an exception at the origin of the error would make much easier debugging.

So we need a way to say how we want getDefinition() to behave - exception or NULL.
Actually Symfony's DIC has something like this this already :

interface ContainerInterface
{
    const EXCEPTION_ON_INVALID_REFERENCE = 1;
    const NULL_ON_INVALID_REFERENCE      = 2;
    const IGNORE_ON_INVALID_REFERENCE    = 3;

    public function get($id, $invalidBehavior = self::EXCEPTION_ON_INVALID_REFERENCE);
}

...
$foo = $container->get('bar', ContainerInterface::NULL_ON_INVALID_REFERENCE):
...

(also used for new Reference('bar', ContainerInterface::NULL_ON_INVALID_REFERENCE); on ContainerBuilders, so that's a pattern that can potentially be found in our Bundle::build() implementations already)

So, while I approve the approach, I wonder whether, rather than a bool whose signification is never too clear when looking at the calling code, we should go with similar constants (although I couldn't really make sense of IGNORE_ON_INVALID_REFERENCE).

Also, whether bool on constants, having to reproduce the supporting logic in all discoveries and decorators kind of begs for #1809200: Consolidate discovery code in a DiscoveryBase and DiscoveryDecoratorBase :-)

#12: discovery-exceptions-1846070-12.patch queued for re-testing.

XREF #1897762: Improve exception handling in case of NULL from getPluginClass

fix doubling of API changes section

What about something like this? Instead of using constants to alter methods' behaviors, we simply add has*() methods that allow us to fail gracefully by checking for something before we request it.

The code is untested and probably not complete, but you'll get the idea.

Then should has() be used in calling code too? We could also think about getting more info in that exception message.

Looks like this overlaps with #2178795: Allow DiscoveryInterface::getDefinition() to throw an exception for an invalid plugin ?

Parts of this are unknowingly duplicated by #2178795: Allow DiscoveryInterface::getDefinition() to throw an exception for an invalid plugin (the hasDefinition).
The other part are explicitly duplicated in #2189985: Rename UnknownPluginException to PluginNotFoundException, which is my fault...
I could close that if you'd rather continue here, but that one was quick and RTBC.

Then should has() be used in calling code too? We could also think about getting more info in that exception message.

If you really only want to check whether a plugin exists, or if you want to be able to degrade gracefully (using a fallback plugin, for instance), than calling has() is the way to go. The exception should ideally not be thrown on live sites as it should only occur when a developer makes a typo in a plugin ID, for instance.

I'll take a look at #2178795: Allow DiscoveryInterface::getDefinition() to throw an exception for an invalid plugin. This patch doesn't only touch DiscoveryInterface, though, but also deals with derivers and derivatives, for instance.

Also #1875996: Reconsider naming conventions for derivative classes will pave the way for more self-explanatory exception names.

Just noticed, probably more appropriate to make this an InvalidArgumentException. Just implement the Plugin "ExceptionInterface" instead.

patch doesn't work

as I understand it's not actual for 8.6, the structure has many changes.

We can (should) improve the wording to clarify the problem.

But the problem remains that you'll run into this error when adding a plugin to an existing module:

Plugin (my_plugin) instance class "Drupal\my_module\Plugin\ConfigFilter\MyPlugin" does not exist. In DefaultFactory.php line 97.

The discovery will find the plugin id but the class is not yet loaded (a new plugin requires a cache-clear).

There are a large number of issues in the queue describing this error message, some have a faulty namespace but in a number of cases it's due to cached plugins.

There is an issue that should solve this for plugin derivers but I couldn't find one for plugins.

Drupal plugin system has issues, it in some cases crashes the whole site when the plugin is missing. We should have some drush command to recover from site crash. Or plugin system should auto recover it self and prevent site crash.

Same problem here. I'm uploaded my site from localhost, database connects but I see this error message when I try to get into drupal admin page.

Looking for a solution. Thanks for your time.

1. +++ b/core/lib/Drupal/Component/Annotation/Plugin/Discovery/AnnotatedClassDiscovery.php
   @@ -76,11 +78,27 @@ protected function getAnnotationReader() {
   +    if (isset($plugin_definitions[$plugin_id])) {
   +      return $plugin_definitions[$plugin_id];
   ...
   +    else {
   

   else here is unnecessary, as in if, there is a return

2. +++ b/core/lib/Drupal/Component/Plugin/Discovery/DerivativeDiscoveryDecorator.php
   @@ -185,18 +210,35 @@ protected function getDerivativeFetcher($base_plugin_id, array $base_definition)
   +    else {
   +      throw new DerivativeFetcherNotFoundException('Plugin @plugin_id does not specify a derivative fetcher.', array(
   +        '@plugin_id' => $base_definition['id'],
   +      ));
   

   Similar to the above i guess

3. +++ b/core/lib/Drupal/Component/Plugin/Discovery/StaticDiscovery.php
   @@ -21,10 +23,24 @@ class StaticDiscovery implements DiscoveryInterface {
   +    else {
   +      throw new PluginNotFoundException(String::format('The plugin %plugin_id could not be found.', array(
   +        '%plugin_id' => $plugin_id,
   

   And here

4. And more similar to the above ...

applied the patch after making changes.

@narendra.rajwar27, Thank you for looking into to this issue and working to get a viable patch. The rerolls in comment #43, #44 and #45 do not have interdiffs (or a diff) so it isn't clear what changes have been made or why. You are more likely to get a review if the interdiff is provided as well as a comment describing what you have changed and why. This makes it much easier for the reviewer to understand the changes.

This issue began in 2012 and much has changed. The proposed resolution in the IS suggests adding a new PluginNotFoundException. That exception already exists, \Drupal\Component\Plugin\Exception\PluginNotFoundException, so let's have an issue summary update.

Checking my work and this isn't a bug and I didn't look at this as part of Bug Smash Initiative work, so removing tag.

The patch no longer applies against 9.2.x. Here is a re-roll without any changes.

Even after re-rolling, it appears there is another issue. I get a WSOD with the following message in watchdog:

Error: Class 'Drupal\Component\Utility\SafeMarkup' not found in Drupal\Component\Plugin\Discovery\DerivativeDiscoveryDecorator->getDefinition() (line 83 of /core/lib/Drupal/Component/Plugin/Discovery/DerivativeDiscoveryDecorator.php)

Related: #3387596: views.settings config object should not be used to cache list of display extenders