Provide and use API methods for retrieving base plugin and derivative names from block plugin IDs

I tried to understand the problem. I think we can create wrapper functions of getPluginId() like
getBasePluginId()
getDerivativeId()

Here is the start.

#6: 1874498-6.patch queued for re-testing.

Many things implement PluginInspectionInterface without extending PluginBase

And many of them don't use derivatives.

Ok got it. Removed the changes in PluginInspectionInterface.

It still needs an interface somewhere. Maybe a new one that all classes with 'derivative' in the annotation *should* implement? Not sure. Traits would probably have been useful here...

As per @tim.plunkett's comment, setting this back to "needs work"

seems related to this ancient issue #1678612: Make getBasePluginId and getDerivativeId static and public. There doesn't really seem to be a stated problem here though. The "problem" in the summary is a stating of facts and I'm not going to try and figure out what from that 400 comment thread we're talking about. Could we be more clear about the problematic use case?

Hopefully this patch will illustrate the problem this solves. The issue summary should still be updated.

And yes, I know that I am contradicting myself in #10, but I think that it's better to add these methods here than to force another interface on, at least without traits.

Okay, nevermind. Here's the middle ground: 2 interfaces, but the base class just implements both. You're not forced into it.

I hate BlockTemplateSuggestionsUnitTest.

Because this needed a rerole.

#21: plugin-1874498-21.patch queued for re-testing.

Rerolled. Let's stop propagating this ugliness.

Does this overlap with #2035345: Reconsider whether to use ':' as separator for derivative plugins?

Does this overlap with #2035345: Reconsider whether to use ':' as separator for derivative plugins?

I would say so as the other issue moves the responsibility to define the separator into another object.

There was a new test added recently.

Reroll.

1. +++ b/core/lib/Drupal/Component/Plugin/PluginBase.php
   @@ -58,6 +58,29 @@ public function getPluginId() {
   diff --git a/core/lib/Drupal/Component/Plugin/PluginInspectionInterface.php b/core/lib/Drupal/Component/Plugin/PluginInspectionInterface.php
   
   diff --git a/core/lib/Drupal/Component/Plugin/PluginInspectionInterface.php b/core/lib/Drupal/Component/Plugin/PluginInspectionInterface.php
   index 31e5e46..71fee3b 100644
   
   index 31e5e46..71fee3b 100644
   --- a/core/lib/Drupal/Component/Plugin/PluginInspectionInterface.php
   
   --- a/core/lib/Drupal/Component/Plugin/PluginInspectionInterface.php
   +++ b/core/lib/Drupal/Component/Plugin/PluginInspectionInterface.php
   
   +++ b/core/lib/Drupal/Component/Plugin/PluginInspectionInterface.php
   +++ b/core/lib/Drupal/Component/Plugin/PluginInspectionInterface.php
   @@ -30,4 +30,5 @@ public function getPluginId();
   
   @@ -30,4 +30,5 @@ public function getPluginId();
       *   plugin manager.
       */
      public function getPluginDefinition();
   +
    }
   

   Sort of out of scope but I don't care.

2. +++ b/core/modules/block/lib/Drupal/block/Tests/BlockPreprocessTest.php
   @@ -10,9 +10,9 @@
   -class BlockPreprocessUnitTest extends WebTestBase {
   

   HAHAH

#29: plugin-1874498-29.patch queued for re-testing.

Rerolled

+++ b/core/tests/Drupal/Tests/Component/Plugin/PluginBaseTest.php
@@ -0,0 +1,115 @@
+  public function testGetPluginId() {
...
+  public function testGetBasePluginId() {
...
+  public function testGetDerivativeId() {

those should use data providers to avoid code duplication

also, no longer applies

Good point.

Updated issue summary.

A few commas missing. I'll do a proper review in the day few days.

1. +++ b/core/tests/Drupal/Tests/Component/Plugin/PluginBaseTest.php
   @@ -0,0 +1,134 @@
   +      array()
   

   Missing comma.

2. +++ b/core/tests/Drupal/Tests/Component/Plugin/PluginBaseTest.php
   @@ -0,0 +1,134 @@
   +      array()
   

   here

3. +++ b/core/tests/Drupal/Tests/Component/Plugin/PluginBaseTest.php
   @@ -0,0 +1,134 @@
   +      array()
   

   here

4. +++ b/core/tests/Drupal/Tests/Component/Plugin/PluginBaseTest.php
   @@ -0,0 +1,134 @@
   +      array('value', array('key' => 'value'))
   

   here

Good catches!

Looks good to me.

1. +++ b/core/lib/Drupal/Component/Plugin/PluginBase.php
   @@ -58,6 +58,29 @@ public function getPluginId() {
   +    if (strpos($plugin_id, ':')) {
   

   I wonder if it might not be a good idea to use a constant for the separator, since this is also used by DerivativeDiscoveryDecorator and the whole derivative system currently relies on the separator being a colon. See #2035345: Reconsider whether to use ':' as separator for derivative plugins as well.

2. +++ b/core/modules/block/lib/Drupal/block/Tests/BlockPreprocessUnitTest.php
   +++ b/core/modules/block/lib/Drupal/block/Tests/BlockPreprocessUnitTest.php
   @@ -2,7 +2,7 @@
   
   @@ -2,7 +2,7 @@
    
    /**
     * @file
   - * Contains \Drupal\block\Tests\BlockPreprocessUnitTest.
   + * Contains \Drupal\block\Tests\BlockPreprocessTest.
     */
    
    namespace Drupal\block\Tests;
   @@ -10,9 +10,9 @@
   
   @@ -10,9 +10,9 @@
    use Drupal\simpletest\WebTestBase;
    
    /**
   - * Unit tests for template_preprocess_block().
   + * Web tests for template_preprocess_block().
     */
   -class BlockPreprocessUnitTest extends WebTestBase {
   +class BlockPreprocessTest extends WebTestBase {
   

   The class name does not match the file name.

Thank you for the review.

41: plugin-1874498.patch queued for re-testing.

41: plugin-1874498.patch queued for re-testing.

Status was wrong.

Re-rolled. +1 for RTBC from me.

It works!

Committed/pushed to 8.x, thanks!