PluginException should provide a getPluginId() method

This is kinda of novice task.

Base PluginException class doesn't define a specific constructor parameter for the pluginId. New instance creation are passed the pluginId via the $code parameter (from Exception base class).

On the other end, PluginNotFoundException and InvalidPluginDefinitionException define a $pluginId constructor parameter which is redundant with $code.

There is less instance creation of those two so it would be more straightforward to drop the specific $pluginId argument and use $code everywhere in an aim to unify those classes (getPluginId() would return $code).

On the other hand, is it the real purpose of the $code argument. Shouldn't we add a $pluginId constructor parameter to the PluginException class and fix all instance creation (and what value give to $code in that case?)

On the other hand, is it the real purpose of the $code argument. Shouldn't we add a $pluginId constructor parameter to the PluginException class and fix all instance creation (and what value give to $code in that case?)

Yeah indeed we have to expand the exception class or provide one for cases where the plugin ID is known. It is always a better DX if you provide more context.

This is a first attempt.

A getPluginId() method already existed in InvalidPluginDefinitionException so I moved it to PluginException alongside the protected var $pluginId.

Next step (if tests don't already broke at this step) : Add a pluginId parameter to PluginException constructor and re-factor every instantiation of this class and its subclasses.

(put to need reviews to trigger the bot but I don't think it's finished yet)

Testbot is happy. Deeper refactoring can take place.

I added a plugin_id argument in PluginException and re-factored every instantiation I could find.

I'm work on review for a patch.

The patch looks OK for me.

I verified if all execution of PluginException hierarchy constructors:
- InvalidDeriverException
- InvalidPluginDefinitionException
- PluginNotFoundException

For all cases, constructor is updated.

My local phpunit execution complains about incomplete tests, but there are out of scope of this issue. It's related to issue #2299795, so it's different story. I'm still waiting for output from testbot.

I got failing tests, I'm investigating that.

I found source of problem. One of PluginNotFound exceptions has one argument too much and php complained about type of arguments. I fixed that, and attached patch should hopefully fix the issue.

I will make this "needs work" when it comes back from test bot.

+++ b/core/lib/Drupal/Core/Menu/MenuTreeStorage.php
@@ -1153,7 +1153,8 @@ protected function ensureTableExists() {
+      // TODO why are we firing a PluginException here ?

This should be @todo per documentation standards: https://www.drupal.org/coding-standards/docs#todo

Usually we make a follow-up issue about this.

Can we find a follow-up issue for this that already exists or create a new one.

I'll mark this needs work right now anyway.

There's already an issue so I'm removing the TODO :
#2302805: Have a more specific exception for menu links than generic pluginException (maybe two)

Latest changes (patch #18) removes single line of comment (TODO line). I don't see other changes, so it's still look ok for me.

1. +++ b/core/lib/Drupal/Component/Plugin/Exception/InvalidPluginDefinitionException.php
   @@ -13,13 +13,6 @@
      /**
   ...
       * Constructs a InvalidPluginDefinitionException.
       *
       * @param string $plugin_id
   
   @@ -28,18 +21,7 @@ class InvalidPluginDefinitionException extends PluginException {
       * @see \Exception for the remaining parameters.
       */
      public function __construct($plugin_id, $message = '', $code = 0, \Exception $previous = NULL) {
   ...
   +    parent::__construct($plugin_id, $message, $code, $previous);
      }
   

   Replace the docblock with:

     /**
      * {@inheritdoc}
      */
   

   What happens when the message is empty? Shouldn't we provide a default generic message?

       if (empty($message)) {
         $message = sprinf("Invalid plugin '%s' exception.', $plugin_id);
       }
       parent::__construct($message, $code, $previous);
   

2. +++ b/core/lib/Drupal/Component/Plugin/Exception/PluginException.php
   @@ -5,9 +5,44 @@
   +use Exception;
   

   Empty line between 'namespace' and 'use'.

3. +++ b/core/lib/Drupal/Component/Plugin/Exception/PluginException.php
   @@ -5,9 +5,44 @@
   +class PluginException extends \Exception implements ExceptionInterface {
   

   Just Exception, no name spacing while it has been already declared in 'use'.

4. +++ b/core/lib/Drupal/Component/Plugin/Exception/PluginException.php
   @@ -5,9 +5,44 @@
   +   * Creates a new instance of PluginException
   

   Empty line between description and @param bloc. Missed dot at the end.

5. +++ b/core/lib/Drupal/Component/Plugin/Exception/PluginException.php
   @@ -5,9 +5,44 @@
   +   *   the id of the plugin which relates to this exception
   

   Should start with upercase ("The...").

6. +++ b/core/lib/Drupal/Component/Plugin/Exception/PluginException.php
   @@ -5,9 +5,44 @@
   +   *   [optional] The Exception message to throw.
   ...
   +   *   [optional] The previous exception used for the exception chaining. Since 5.3.0
   

   "Optional" should be enclosed in normal/round parentheses. Also there's a end dot missed. When a parameter is optional you should also provide the the default value at the end of description. Example: (optional) The Exception code. Defaults tp 0.

7. +++ b/core/lib/Drupal/Component/Plugin/Exception/PluginException.php
   @@ -5,9 +5,44 @@
   +   */
   

   At the end of the docbloc ass also a reference to \Exception:

      *
      * @see \Exception
      */
   

8. +++ b/core/lib/Drupal/Component/Plugin/Exception/PluginException.php
   @@ -5,9 +5,44 @@
   +  public function __construct($plugin_id, $message = "", $code = 0, Exception $previous = NULL) {
   +    $this->pluginId = $plugin_id;
   

   What happens when the message is empty? Shouldn't we provide a default generic message?

       if (empty($message)) {
         $message = sprinf("Plugin '%s' exception.', $plugin_id);
       }
       parent::__construct($message, $code, $previous);
   

9. +++ b/core/lib/Drupal/Component/Plugin/Exception/PluginException.php
   @@ -5,9 +5,44 @@
   +    parent::__construct($message, $code, $previous);
   +  }
   +  /**
   

   Empty line between the 2 methods.

10. Replace PluginNotFoundException::__construct() docblock with:

      /**
       * {@inheritdoc}
       */
    

EDIT: Add also an interdiff against the last patch.

Did some update based on claudiu suggestions.

@rudins, interdiff?

+++ b/core/lib/Drupal/Component/Plugin/Exception/PluginException.php
@@ -6,8 +6,51 @@
+class PluginException extends Exception implements ExceptionInterface {

My bad. It seems that this breaks. Revert it back.a

Let's try this one.

Looks like some typos:

1. +++ b/core/lib/Drupal/Component/Plugin/Exception/InvalidPluginDefinitionException.php
   @@ -13,33 +13,13 @@
   +      $message = sprinf("Invalid plugin '%s' exception.", $plugin_id);
   

2. +++ b/core/lib/Drupal/Component/Plugin/Exception/PluginException.php
   @@ -6,8 +6,51 @@
   +      $message = sprinf("Plugin '%s' exception.", $plugin_id);
   

Addressed a few errors.

Let's add an interface for this kind of exceptions.

Reverted PluginNotFoundException message.

$ git apply 2121349-37.patch 
error: patch failed: core/lib/Drupal/Core/ImageToolkit/ImageToolkitOperationBase.php:124
error: core/lib/Drupal/Core/ImageToolkit/ImageToolkitOperationBase.php: patch does not apply
error: patch failed: core/lib/Drupal/Core/Menu/MenuLinkBase.php:177
error: core/lib/Drupal/Core/Menu/MenuLinkBase.php: patch does not apply
error: patch failed: core/lib/Drupal/Core/Menu/MenuLinkManager.php:281
error: core/lib/Drupal/Core/Menu/MenuLinkManager.php: patch does not apply
error: patch failed: core/lib/Drupal/Core/Menu/MenuTreeStorage.php:397
error: core/lib/Drupal/Core/Menu/MenuTreeStorage.php: patch does not apply
error: patch failed: core/modules/block/src/BlockPluginCollection.php:56
error: core/modules/block/src/BlockPluginCollection.php: patch does not apply
error: patch failed: core/modules/menu_link_content/src/Plugin/Menu/MenuLinkContent.php:138
error: core/modules/menu_link_content/src/Plugin/Menu/MenuLinkContent.php: patch does not apply

reroll

Re-roll.

Some docs fixes.

PluginExceptionInterface extends ExceptionInterface isn't very self-descriptive, especially since they are both in exactly the same namespace. We should try to come up with a slightly better name.

Hi, I found several places in code, where as id parameter empty string ('') is provided. I'm wondering, if it's valid behaviour and if it should be tolerated, ie:

      throw new PluginException('', $e->getMessage(), NULL, $e);

I'm not sure, but maybe we should provide some sort of failsafe value?

It's not a problem, so no failsafe is needed, but it is definitely confusing, as in that particular context there simply is no plugin ID.

I think we should keep the old PluginException as it is, and introduce a child exception that should be used for problems with specific plugins, rather than any problem in the plugin system.

Just a reroll with an additional change to a new use of the plugin exception in /core/modules/layout_builder/src/SectionComponent.php.

It looks like there's an additional one in DefaultFactory.php that can be updated.

Rerolled

That patch didn't include the new file, PluginExceptionInterface.php. This corrects that and uses sprintf instead of String::format in MenuLinkManager.php.

This was opened almost 7 years ago, long before the approach to BC was adopted.
In order to make this change now, we'd have to introduce a new exception class and deprecate the existing one, not change the constructor of the existing one.

This needs work for #59. What should be name of the new exception class?

Re-roll patch #58 for Drupal 10.x

added updated patch and try to fixed CCF #67

fixed phpcs issues.

I tested the patch on #69 and it works for 10.1.x-dev.
SS attached.

Please don't post screenshots of applying a patch. It's a waste of diskspace and bandwidth. You can just say it applies. Also, the testbot will say if it doesn't apply.

Also, the patch is failing tests, so at this point it needs work anyway.

latest patchs are failing test, so updating the patch for resolving errors and try to fix these errors, also attaching Interdiff along with.