Remove BC layers in the Plugin component

Expanding the scope a bit, removing all plugin system BC layers.

@todo: Also remove the context/context_definitions thing: \Drupal\Core\Plugin\DefaultPluginManager::fixContextAwareDefinitions.

Removed that failing BC test.

Looks great to go, does it need subsystem maintainer to approve?

+++ b/core/lib/Drupal/Component/Plugin/PluginHelper.php
@@ -25,7 +25,7 @@ class PluginHelper {
    */
   public static function isConfigurable($plugin) {
-    return $plugin instanceof ConfigurableInterface || $plugin instanceof ConfigurablePluginInterface;
+    return $plugin instanceof ConfigurableInterface;
   }

The docblock of this class says that we could deprecate it in D9, maybe we could do that later as this helpful for modules to be compatible with d8 modules that haven't been updated yet. And adding a deprecation now might confuse some of our tools?

(I'm also not quite sure what the difference of this and $plugin->isConfigurable() is, to be honest)

Let's remove the PluginHelper hunk and properly deprecate it in a follow-up issue prior to removal.

I do need to remove that part, that interface is being removed here, but it still works as an API, it just only checks the one interface that is still in D9.

I created a follow-up #3105685: Deprecate PluginHelper::isConfigurable()

+++ b/core/lib/Drupal/Core/Plugin/Context/ContextDefinition.php
@@ -140,11 +108,6 @@ public function __construct($data_type = 'any', $label = NULL, $required = TRUE,
     $this->defaultValue = $default_value;
-
-    if (strpos($data_type, 'entity:') === 0 && !($this instanceof EntityContextDefinition)) {
-      @trigger_error('Constructing a ContextDefinition object for an entity type is deprecated in Drupal 8.6.0. Use ' . __NAMESPACE__ . '\EntityContextDefinition instead. See https://www.drupal.org/node/2976400 for more information.', E_USER_DEPRECATED);
-      $this->initializeEntityContextDefinition();
-    }
   }
 

One more thing, I'm not sure what will happen with this removed and people still trying to do this. We could keep this as an assert() or exception?

I think an assert, similar to the one in #3097879: Remove all @deprecated code in \Drupal\Core\Database, is a great idea.
NW for that

Added that, with a test.

+++ b/core/tests/Drupal/KernelTests/Core/Plugin/ContextDefinitionTest.php
@@ -34,4 +34,15 @@ public function testIsSatisfiedBy() {
+  public function testEntityContextDefinitionAssert() {
+    $this->expectException(\AssertionError::class);
+    $this->expectExceptionMessage('assert(strpos($data_type, \'entity:\') !== 0 || $this instanceof EntityContextDefinition)');
+    new ContextDefinition('entity:entity_test');
+  }
+
+
 }

nit, extra line, otherwise needs RTBC

All feedback has been addressed so RTBC

Applied patch locally and searched for deprecated|trigger_error in core/lib/Drupal/Component/Plugin

There's still some reference to the old configurable interfaces etc in the docblock of PluginHelper

/**
 * A helper class to determine if a plugin is configurable.
 *
 * Because configurable plugins in Drupal 8 might implement either the
 * deprecated ConfigurablePluginInterface or the new ConfigurableInterface,
 * this static method is provided so that a calling class can determine if a
 * plugin is configurable without checking it against a deprecated interface.
 * In Drupal 9, this check should be reduced to checking for
 * ConfigurableInterface only and be deprecated in favor of calling classes
 * checking against the interface directly.
 */
class PluginHelper {

We should clean those up here too

See #11-13, I created a follow-up for that. This API is not deprecated, so we could only add a new D10 deprecation here and I kind of like the helper, that does make it useful to keep code compatible with modules that still use the old interface and D9. Deprecating it now for D10 would make it harder for modules to update now as tools would report that as a problem.

Discussed with @larowlan, we agreed on just replacing that docblock with a @todo Consider deprecating in issue-url?` or so.

Will do that tomorrow unless someone gets to it first..

Fixed #17 and #21

The idea was to replace that existing block completely with the todo, as we don't need the D8 explanation anymore.

+++ b/core/lib/Drupal/Core/Plugin/Context/ContextDefinition.php
@@ -141,10 +109,7 @@ public function __construct($data_type = 'any', $label = NULL, $required = TRUE,
+    assert(strpos($data_type, 'entity:') !== 0 || $this instanceof EntityContextDefinition);

Would this be better as an exception so we could give the caller a reason, and also we can test for this?

+++ b/core/tests/Drupal/KernelTests/Core/Plugin/ContextDefinitionTest.php
@@ -34,4 +34,14 @@ public function testIsSatisfiedBy() {
+  /**
+   * @covers ::__construct
+   */
+  public function testEntityContextDefinitionAssert() {
+    $this->expectException(\AssertionError::class);
+    $this->expectExceptionMessage('assert(strpos($data_type, \'entity:\') !== 0 || $this instanceof EntityContextDefinition)');
+    new ContextDefinition('entity:entity_test');
+  }

assert() throws an AssertError when it is enabled in php.ini and we are testing it.

Ah, didn't see that. Is it still worth leaving a comment above with a link to the change record in case anyone triggers this assertion? Or adding an assertion description?

#26 is just nitpicking so otherwise this is ready to go.

Committed 267ccab and pushed to 9.0.x. Thanks!

FILE: ...e/tests/Drupal/KernelTests/Core/Plugin/ContextDefinitionTest.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 35 | ERROR | [x] Expected 1 blank line after function; 2 found
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

Time: 53ms; Memory: 8MB


FILE: ...core/tests/Drupal/Tests/Core/Plugin/DefaultPluginManagerTest.php
----------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------
 10 | WARNING | [x] Unused use statement
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

diff --git a/core/tests/Drupal/KernelTests/Core/Plugin/ContextDefinitionTest.php b/core/tests/Drupal/KernelTests/Core/Plugin/ContextDefinitionTest.php
index b323c208ee..9194c096f6 100644
--- a/core/tests/Drupal/KernelTests/Core/Plugin/ContextDefinitionTest.php
+++ b/core/tests/Drupal/KernelTests/Core/Plugin/ContextDefinitionTest.php
@@ -34,7 +34,6 @@ public function testIsSatisfiedBy() {
     $this->assertTrue($requirement->isSatisfiedBy($context));
   }
 
-
   /**
    * @covers ::__construct
    */
diff --git a/core/tests/Drupal/Tests/Core/Plugin/DefaultPluginManagerTest.php b/core/tests/Drupal/Tests/Core/Plugin/DefaultPluginManagerTest.php
index b6692665f1..10955fc82e 100644
--- a/core/tests/Drupal/Tests/Core/Plugin/DefaultPluginManagerTest.php
+++ b/core/tests/Drupal/Tests/Core/Plugin/DefaultPluginManagerTest.php
@@ -7,7 +7,6 @@
 use Drupal\Component\Plugin\Exception\PluginNotFoundException;
 use Drupal\Core\Extension\ModuleHandlerInterface;
 use Drupal\Core\Form\FormStateInterface;
-use Drupal\Core\Plugin\Context\ContextDefinition;
 use Drupal\Core\Plugin\PluginFormInterface;
 use Drupal\Tests\UnitTestCase;
 

Fixed the coding standards errors on commit.

I was wrong as my if conditions were reversed, going to look at this further but please disregard :D