Deprecate ConfigurablePluginInterface and replace with an interface that doesn't extend DependentPluginInterface

I don't think we need to do this. Why bother adding yet another interface when we can just implement an empty calculateDependencies() method? Currently, Migrate is the only use case for such an interface. If another subsystem needed similar functionality, that might be a reason to decouple DependentPluginInterface from ConfigurablePluginInterface. But until that happens, this seems like overkill to me.

Discussed with @heddn on IRC.

The real problem here is the fact that ConfigurablePluginInterface extends DependentPluginInterface. These two interfaces should be decoupled. And I think we can just go ahead and do that without breaking BC. Why? Because any existing plugin which uses ConfigurablePluginInterface must already have a calculateDependencies() method, thus fulfilling DependentPluginInterface.

So all we need to do is two things:

1. Make ConfigurablePluginInterface independent from DependentPluginInterface.
2. Any existing ConfigurablePluginInterface implementations in core will need to explicitly implement DependentPluginInterface.

This will also benefit existing core plugins by allowing us to remove bogus implementations of calculateDependencies() that return an empty array, solely in order to fulfill DependentPluginInterface.

How about we try that, and see how much stuff breaks? Let's also get framework manager sign-off on this approach.

I don't think we can break that now. What I think we need to do is to make ConfigurablePluginInterface implement DependentPluginInterface, ANotherInterface. Where ANotherInterface is all the methods from ConfigurablePluginInterface. That way new things can implement said interface and we can change places where ConfigurablePluginInterface is checked and swap for DependentPluginInterface / ANotherInterface accordingly because we know that ConfigurablePluginInterface implements both.

Yes #5 is more logical but I think it is impossible to do in backwardsly compatible manner for D8. D9 this makes total sense.

Minor verbiage changes in the IS, and tagging for a change record.

Change record written. https://www.drupal.org/node/2946161

+++ b/core/lib/Drupal/Component/Plugin/ConfigurablePluginInterface.php
@@ -33,4 +33,16 @@ public function setConfiguration(array $configuration);
+   * @deprecated in 8.6.x. One must implement an additional interface if one

Shouldn't that say 'and will be removed in 9.0.0' or something like that?

"One must"...
Docs are usually in the imperative, pronounless form, e.g. 'Implement a yada yada'.

Finally, the last sentence is more or less saying the same as the first sentence: they could be combined:

To have a plugin that is both configurable and declares dependencies, implement DPI in addition to CPI.

I couldn't make it to the slack discussion, so sorry if this is rehashing old ground...

But is this really the best fix?

It's laying groundwork for D9, but it's not changing *anything* for modules that use this right now. If you want configurable plugins, but you don't care about config dependencies, you still have to pointlessly implement calculateDependencies() with an empty return.

I think the solution to add a new interface for ConfigurablePluginInterface to inherit from is far prefererable, as it has immediate benefit. Even if it does pose the rather tricky problem of what to call it.

@joachim. That was where we got stuck. It was a tradeoff between being able to get rid of the calculateDependencies() now, or keep the ConfigurablePluginInterface name for the eventually decoupled interface. The consensus was do it this way so that eventually the decoupled plugin could still be called CPI, in combination with #2852463: Create a trait to implement \Drupal\Component\Plugin\ConfigurablePluginInterface so that the useless methods could be reduced to one copy in a single trait that could be easily cleaned up for 9.x.

So, I think we have a problem with this plan.

If we have:

interface A
interface B extends A
Class C implements B
Class D implements B, A

This will cause a PHP Fatal Error, Class D cannot implement previously implemented interface A.

Technically, "Class D implements A, B" will not throw the error, but this is a quirk of PHP, and I'm not sure I'm comfortable with using it.

Even if we did, I am unsure if it's possible to implement the final step in this list, though I'm hardly an expert in reflection classes and such.

I don't know of any way to tell the difference between Class C and D; They both implement A, and the only means I can find to possibly tell them apart is through

$Dreflection = new ReflectionClass(D);
$Creflection = new ReflectionClass(C);
$Dinterfaces = $Dreflection->getInterfaces();
$Cinterfaces = $Creflection->getInterfaces();

In the above, the interface lists will be in different orders, i.e. the reflection for D will have DependantPluginInterface before ConfigurablePluginInterface in the interface array, a for C the order will be reversed, but again, it's a quirk that they are in that particular order, there is no php documentation that declares they should be in that order, it just happens to be the order that they are processed.

Thoughts?

That's fine for the test, but it's not the primary problem. The primary problem is that we can't leave ConfigurablePluginInterface extending DependantPluginInterface for BC reasons AND have classes directly implementing both. It's one or the other, which means there would be no way to have a plugin that implements both work with both Drupal 8 and 9, other than the quirky interface order hack I mentioned above.

Which means to separate them in a backwards compatible way, I think you have to rename one or the other or both (which we wanted to avoid), unless I'm not thinking of something (entirely possible).

In trying to understand this issue, I noticed that there are three options discussed to deal with this issue.

Options:

1. Keep it as is
   ConfigurablePluginInterface keeps extending DependentPluginInterface and this issue is closed as won't fix.
   Cons:
   • Plugin types that do nothing with dependency calculation need to implement a method redundant to them: calculateDependencies().
2. Make existing ConfigurablePluginInterface no longer extend DependentPluginInterface
   All plugin types implementing ConfigurablePluginInterface that need to have dependencies calculated need to be updated to also implement DependentPluginInterface.
   Cons:
   • Breaks backwards compatibility: classes cannot implement the same interface twice (explained in #21).
3. Create a second ConfigurablePluginInterface that does not extend DependentPluginInterface
   A new class called ConfigurablePluginWithoutDependenciesInterface is added with all the methods ConfigurablePluginInterface now has. ConfigurablePluginInterface itself will then become empty and it will extend ConfigurablePluginWithoutDependenciesInterface and DependentPluginInterface.
   Cons:
   • Introduces a new interface, thus adds complexity.

It seems that option 2 is out of the question, because in that case there is no way for contributes modules to provide configurable plugins with dependency calculation that will work with both the current situation and the new situation. This would mean that these contributed modules will be forced to raise their core dependency. And/or provide a new major version to keep supporting both core versions.

So that leaves us to decide between option 1 and 3. Personally, I don't see why having some plugin types need to implement a redundant method is that much of a problem compared to what issues get blocked by this issue now. Option 3 might have the result that some subsystems need to check for ConfigurablePluginWithoutDependenciesInterface instead. So the ones that want to, will not be compatible with older core versions. As I see it, not that much of an issue either: these contributes modules will just raise their core dependency. Something that Commerce already does regularly.

So I'm fine with either option 1 or option 3. But we need to make a decision to unblock other issues.

This issue blocks:
#2852463: Create a trait and base class to implement \Drupal\Component\Plugin\ConfigurableInterface
#2937177: Migrate plugin base classes should implement ConfigurablePluginInterface
Feeds Migrate

In discussions in slack with various managers and maintainers, I feel like we keep coming back to option 3, and then get hung up on what to call the new interface. In the interests of trying to move things forward, I would specifically propose the following:

In Drupal 8.7.0 (and specifically as part of this issue):

1. Create a new interface, \Drupal\Component\Plugin\ConfigurableInterface, containing the getConfiguration, setConfiguration and defaultConfiguration method definitions from the current \Drupal\Component\Plugin\ConfigurablePluginInterface
2. Leave \Drupal\Component\Plugin\DependentPluginInterface alone.
3. Have \Drupal\Component\Plugin\ConfigurablePluginInterface extend DependentPluginInterface and ConfigurableInterface and deprecate it.
4. Replace all implementations of ConfigurablePluginInterface in core with either ConfigurableInterface, DependentPluginInterface, or just ConfigurableInterface as needed.

In a follow up issue (i.e. #2852463: Create a trait and base class to implement \Drupal\Component\Plugin\ConfigurableInterface) I propose we add \Drupal\Component\Plugin\ConfigurablePluginTrait and/or \Drupal\Core\Plugin\ConfigurablePluginBase which would use the trait. There is still debate on base class vs trait vs both. and that is a debate for the other issue, not this one.

In Drupal 9.0.0 we remove ConfigurablePluginInterface. If we want at that point, I think we can immediately deprecate ConfigurableInterface and re-implement it as ConfigurablePluginInterface if we really really wanted to have the name back, but that also would be a debate for another issue.

+1 to #27. If a framework manager agrees, let’s do it!

Signing off on #27 as one of the plugin subsystem maintainers

Here's a starter patch to see what #27 would look like. From scratch, so no interdiff.

+++ b/core/lib/Drupal/Core/Field/PluginSettingsInterface.php
@@ -9,7 +9,12 @@
  *
  * @deprecated in Drupal 8.1.0 and will be removed before Drupal 9.0.0. Use
- *   \Drupal\Component\Plugin\ConfigurablePluginInterface instead.
+ *   \Drupal\Component\Plugin\use Drupal\Component\Plugin\ConfigurableInterface;
+use Drupal\Component\Plugin\DependentPluginInterface;
+use Drupal\Component\Plugin\ConfigurableInterface;
+use Drupal\Component\Plugin\DependentPluginInterface;
+ConfigurableInterface, DependentPluginInterfaceConfigurableInterface, DependentPluginInterfaceConfigurableInterface, DependentPluginInterfaceConfigurableInterface, DependentPluginInterface
+ * ConfigurableInterface, DependentPluginInterfaceConfigurableInterface, DependentPluginInterfaceConfigurableInterface, DependentPluginInterfaceConfigurableInterface instead.
  */
 interface PluginSettingsInterface extends PluginInspectionInterface, ThirdPartySettingsInterface {

This looks pretty weird, did something go wrong here?

Yes. Something went very wrong there. Thanks.

Status change.

One other question I had was whether we wanted to add an optional $key parameter to getConfiguration, to retrieve a specific configuration value. It could accept nested configuration keys as an array. I realize this is not specifically in scope for this, and adds complexity, but if it's something we want to do, It should be done now, as we create a new interface. Since this interface is basically api level, we can't really change the method signature later without breaking implementations.

I like the idea and I think it would improve the DX even more. However, you're certainly right that it has complicated BC implications. I defer to the august judgment of the framework managers and subsystem maintainers on this one.

But personally, +1 for #39.

@mikelutz
Thanks for working on this!

I prefer to not add an optional $key parameter to getConfiguration(), for the following reasons:

• Code that is passing the getConfiguration() call to an other plugin will be forced to pass the optional parameter. So contrib modules need to update their code to become compatible with the new situation. DefaultLazyPluginCollection for example does this:
  

  /**
   * {@inheritdoc}
   */
  public function getConfiguration() {
    $instances = [];
    // Store the current order of the instances.
    $current_order = $this->instanceIDs;
    // Reorder the instances to match the original order, adding new instances
    // to the end.
    $this->instanceIDs = $this->originalOrder + $current_order;
  
    foreach ($this as $instance_id => $instance) {
      if ($instance instanceof ConfigurablePluginInterface) {
        $instances[$instance_id] = $instance->getConfiguration();
      }
      else {
        $instances[$instance_id] = $this->configurations[$instance_id];
      }
    }
    // Restore the current order.
    $this->instanceIDs = $current_order;
    return $instances;
  }

• Code that is now just implementing the existing ConfigurePluginInterface need to update their code to become compatible with the new situation. They need to do that to become compatible with code that is passing the optional parameter.
• You can also get a specific key this way: $plugin->getConfiguration()['some_key'];.
• It adds extra complexity and thus decreases the chance that we can get this issue fixed soon.

So -1 for #39 for me.

If we do want that optional parameter, let's decide that in a follow-up instead.

@MegaChriz

You make some good points, and I'm certainly happy to not do this now. I think you are right, If we change the interface that the core base classes use, then we break BC for extensions, so scratch it. I wish there was a way to do it, because I hate the $this->getConfiguration()['key'] pattern, but if that's the way we are going, I'll get used to it.

That reminds me similar discussion about how field settings should return values (they do merge them from field storage). Cant find the issue but it was the same dx bikeshed

I've updated the issue summary:

• I made the problem/motivation somewhat more clear.
• In the proposed resolution, I summarized the three options that were noted in #26.
• Also in the proposed resolution, the proposal for option 3 is added (taken from #27).
• Noted remaining tasks and API changes.

Updated the CR.

Looks great, thanks! And thanks for sticking with this one.

Adding credit

+++ b/core/lib/Drupal/Core/Plugin/ContextAwarePluginBase.php
@@ -78,7 +78,7 @@ public function setContextValue($name, $value) {
-    $configuration = $this instanceof ConfigurablePluginInterface ? $this->getConfiguration() : $this->configuration;
+    $configuration = $this instanceof ConfigurableInterface ? $this->getConfiguration() : $this->configuration;

+++ b/core/modules/aggregator/src/Plugin/AggregatorPluginSettingsBase.php
@@ -19,7 +20,7 @@
-abstract class AggregatorPluginSettingsBase extends PluginBase implements PluginFormInterface, ConfigurablePluginInterface {
+abstract class AggregatorPluginSettingsBase extends PluginBase implements PluginFormInterface, ConfigurableInterface, DependentPluginInterface {

I don't we lose the implements ConfigurablePluginInterface from all the core plugins. This means that anything in contrib that does something like $this instanceof ConfigurablePluginInterface ? $this->getConfiguration() : $this->configuration; as ContextAwarePluginBase will suddenly not work.

I think we need to do something even more tricky which is to change the plugins to do something like
abstract class AggregatorPluginSettingsBase extends PluginBase implements PluginFormInterface, ConfigurableInterface, DependentPluginInterface, ConfigurablePluginInterface { and remove the @trigger_error() from the interface and add it to the block plugin manager that reads all the block plugins and trigger if something implements ConfigurablePluginInterface and not ConfigurableInterface. Then in Drupal 9 we can remove ConfigurablePluginInterface and update all the blocks.

Putting back to needs review to get more opinions. I might be wrong though as BC is hard to work out all the implications.

Alright, I think that only works if ConfigurablePluginInterface does not extend ConfigurableInterface and DependentPluginInterface, and defines it's own methods. Otherwise we run into "Cannot implement previously implemented interface" Fatals, and there's no way to check if something implements ConfigurablePluginInterface without ConfigurableInterface, because anything that implements ConfigurablePluginInterface would automatically implement ConfigurableInterface.

At that point, I *think* we can add a test that scans all classes in core and fails on ones that implement CPI without CI.

I'm not sure how best to trigger a reliable deprecation error for contrib though. BlockPluginManager wouldn't cover all the plugins that use this.

Blocks are one of nine core plugin types that use CPI.

https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Component%21Plug...

\Drupal\Core\Block\BlockPluginInterface
\Drupal\Core\Condition\ConditionInterface
\Drupal\Core\Display\VariantInterface
\Drupal\Core\Layout\LayoutInterface
\Drupal\filter\Plugin\FilterInterface
\Drupal\image\ImageEffectInterface
\Drupal\media\MediaSourceInterface
\Drupal\search\Plugin\ConfigurableSearchPluginInterface
\Drupal\workflows\WorkflowTypeInterface

Let's just go back in time to before CPI was ruined without the sign-off of any plugin subsystem maintainers

What if we just put the trigger_error in the PluginBase constructor?

What's 781 fails between friends?

#57: Probably, if that is the only point of contention, I'll be ecstatic.

Added follow up issue and addressed feedback in #57.

I think the CR can stay as it is, no need to advertise the BC layer.

Changed issue title.

@mikelutz this is looking really great.

Core plugins still implement the old interface. Our generic plugin instanceof checks support both and if you have a plugin that doesn't yet implmement ConfigurableInterface you will get a warning.

This gives contrib and custom time to move over. It might be tricky for us to remember to add both ConfigurableInterface and ConfigurablePluginInterface to new plugins / or decide if that is the correct course of action but at least contrib code that does the instanceof check can be updated the same way as core. It seems this might be missing from the change record. Ie.

+++ b/core/lib/Drupal/Core/Plugin/ContextAwarePluginBase.php
@@ -86,7 +87,7 @@ public function getContextMapping() {
-    if ($this instanceof ConfigurablePluginInterface) {
+    if ($this instanceof ConfigurableInterface || $this instanceof ConfigurablePluginInterface) {

We need something in the CR that says in order to support all configurable plugins in Drupal 8 you need to do this.

I updated the change record. Cheers!

If we are going to recommend contrib check for both interfaces, then we need a core helper method somewhere to do the check. Otherwise we are telling contrib to check for an interface in D8 which won't exist in D9 and they won't have a way to write a module to be compatible with both. There needs to be a method on PluginBase or a static method somewhere in core that returns ($foo instanceof CPI || instanceof CI) in D8 and just instanceof CI in D9.

I don't have time to code it right now, but would love recommendations as to where best to put it.

I'd put it on the PluginBase (mostly because I can't think of another place to put it, except maybe the Plugin annotation class). Something like:

final public static function isConfigurable($plugin) {
  return $plugin instanceof ConfigurableInterface || $plugin instanceof ConfigurablePluginInterface;
}

I think that would be the ideal spot, I'm just concerned that that is a very common base class, and adding a method to it with a name as common as isConfigurable() is going to break somebody, particularly if it's final. I may be wrong. Drupal/system/Action implements an isConfigurable, but that is on Entity, not Plugin so I guess I don't see any isConfigurable's on PluginBase in core.

I really don't want to add a static class just for this one check.

Trying it as a method on PluginBase

Almost feels like a random test failure...

1. +++ b/core/lib/Drupal/Component/Plugin/PluginBase.php
   @@ -93,4 +97,17 @@ public function getPluginDefinition() {
   +  public function isConfigurable() {
   +    return $this instanceof ConfigurableInterface || $this instanceof ConfigurablePluginInterface;
   +  }
   

   I'm wondering if we should make this static method to show that that is makes no change to object state and it should accept $plugin which is an object. That way any of these check outside the plugin system can use this. Not sure. Hmmm... on reviewing more code I became convinced that the non-static way is potentially better... so let's not do this.

2. +++ b/core/lib/Drupal/Component/Plugin/PluginBase.php
   @@ -54,6 +54,10 @@ public function __construct(array $configuration, $plugin_id, $plugin_definition
   +      @trigger_error('Drupal\Component\Plugin\ConfigurablePluginInterface is deprecated in Drupal 8.7.0 and will be removed before Drupal 9.0.0. Implement ConfigurableInterface and/or DependentPluginInterface directly as needed. See https://www.drupal.org/node/2946161', E_USER_DEPRECATED);
   

   I think this should state that for maximum compatibility in Drupal 8 plugins should implement both ConfigurablePluginInterface and ConfigurableInterface since this is a bit complex.

3. +++ b/core/lib/Drupal/Core/Action/ConfigurableActionBase.php
   @@ -2,14 +2,19 @@
   + * @todo Remove ConfigurablePluginInterface before Drupal 9.0.0
   + * @see https://www.drupal.org/project/drupal/issues/3018896
   

   I don't think we should add this @todo everywhere. I think the commitment to remove the interface is enough. Since removing it will make this code break.

4. +++ b/core/modules/system/src/Entity/Action.php
   @@ -133,7 +132,7 @@ public function execute(array $entities) {
      public function isConfigurable() {
   -    return $this->getPlugin() instanceof ConfigurablePluginInterface;
   +    return $this->getPlugin()->isConfigurable();
      }
   

   This hints that (a) the non-static isConfigurable() on the base class is a good way to go and (b) we should deprecate this helper method in a follow-up.

I opened a followup for #4, and removed all the todos. I expanded the deprecation error, But I don't like telling contrib that they have to implement a deprecated interface for maximum compatibility. We are telling them to implement an interface that won't exist in Drupal 9.

I think we need to either NOT deprecate ConfigurablePluginInterface, and just keep it, or create a ConfigurableDependentPluginInterface extends ConfigurableInterface, DependentPluginInterface, ConfigurablePluginInterface {} that we intend to keep in Drupal 9 and tell contrib to use that. That way they will pass 'instanceof ConfigurablePluginInterface' checks without actually implementing the deprecated interface.

In Drupal 9.0 we remove ConfigurablePluginInterface, change ConfigureableDependentPluginInterface to extend DependentPluginInterface, ConfigurableInterface.

In 9.1 we could deprecate ConfigurableInterface in favor of a NEW ConfigurablePluginInterface and do the whole dance again, and by Drupal 10 we could have a sane hierarchy, lol.

The subsystem maintainer review tag was removed a bit prematurely, but I took the time to rereview this since the last time I signed off (which was on the patch in #37).
The changes since then make a lot of sense.
Here's an interdiff from then for anyone else curious.

+1 to RTBC

Actually the CR needs to be updated to account for PluginBase::isConfigurable(). NW for that real quick

CR Updated

Crediting @joachim, @MegaChriz, @phenaproxima and myself for issue reviews. I think we've ended up in the best place with the right compromises in order to get to a better structure for Drupal 9 (rather than Drupal 10)

Committed b1f4962 and pushed to 8.7.x. Thanks!

+++ b/core/lib/Drupal/Core/Plugin/DefaultLazyPluginCollection.php
@@ -112,7 +111,7 @@ public function getConfiguration() {
-      if ($instance instanceof ConfigurablePluginInterface) {
+      if ($instance->isConfigurable()) {

This check assumes that the new interface is implemented on the plugins. Ctools is failing tests because of this at the moment. Any suggestions on how to move forward with support for 8.7? It's a strong possibility this is not a big deal and my understanding on prophecy is weak...

#3030158: Fix test fails for 8.7

+++ b/core/lib/Drupal/Component/Plugin/PluginBase.php
@@ -93,4 +97,14 @@ public function getPluginDefinition() {
+  public function isConfigurable() {

Maybe this method just needs to be added to the ConfigurableInterface?

Reverted for #85. Thanks @joelpittet for reporting this!

Assigning to myself to replace ->isConfigurable with a static helper

Here is the original patch with a new failing test highlighting the issue discovered in ctools. Because we don't have ->isConfigurable() on an interface, it can't be mocked in a prophecy, making it very hard to write tests when the pluginCollections need to access that method.

And a refactor using a static helper method to do the check rather than an undeclared method on PluginBase.

Thanks a bunch @mikelutz, this fixes ctools test error.

With the latest patch in #92

Testing Drupal\Tests\ctools\Unit\VariantCollectionTraitTest
.......                                                             7 / 7 (100%)

Time: 11.53 seconds, Memory: 8.00MB

OK (7 tests, 19 assertions)

With reverted code in HEAD at moment.

[vagrant@dvm drupal]$ vendor/bin/phpunit --configuration core  modules/ctools/tests/src/Unit/VariantCollectionTraitTest.php
PHPUnit 6.5.14 by Sebastian Bergmann and contributors.

Testing Drupal\Tests\ctools\Unit\VariantCollectionTraitTest
.......                                                             7 / 7 (100%)

Time: 5.65 seconds, Memory: 8.00MB

OK (7 tests, 19 assertions)

With the previous patch in #76

[vagrant@dvm drupal]$ vendor/bin/phpunit --configuration core  modules/ctools/tests/src/Unit/VariantCollectionTraitTest.php
PHPUnit 6.5.14 by Sebastian Bergmann and contributors.

Testing Drupal\Tests\ctools\Unit\VariantCollectionTraitTest
...ESSS                                                             7 / 7 (100%)

Time: 6.18 seconds, Memory: 8.00MB

There was 1 error:

1) Drupal\Tests\ctools\Unit\VariantCollectionTraitTest::testAddVariant
Prophecy\Exception\Doubler\MethodNotFoundException: Method `Double\VariantInterface\P8::isConfigurable()` not found.

/var/www/sites/d8/drupal/core/lib/Drupal/Core/Plugin/DefaultLazyPluginCollection.php:160
/var/www/sites/d8/drupal/core/lib/Drupal/Core/Plugin/DefaultLazyPluginCollection.php:171
/var/www/sites/d8/drupal/modules/ctools/src/Plugin/VariantCollectionTrait.php:22
/var/www/sites/d8/drupal/modules/ctools/tests/src/Unit/VariantCollectionTraitTest.php:107

ERRORS!
Tests: 7, Assertions: 6, Errors: 1, Skipped: 3.

I find it hard to believe that that patch has Drupal\Tests\media_library\FunctionalJavascript\MediaLibraryTest::testWidgetUpload as its one and only failing test..
requeuing..

I updated the CR to reference the new static helper.

+++ b/core/lib/Drupal/Core/Plugin/DefaultSingleLazyPluginCollection.php
@@ -67,7 +67,7 @@ protected function initializePlugin($instance_id) {
-    if ($plugin instanceof ConfigurablePluginInterface) {
+    if ($plugin && PluginHelper::isConfigurable($plugin)) {

@@ -81,7 +81,7 @@ public function getConfiguration() {
-    if ($plugin instanceof ConfigurablePluginInterface) {
+    if ($plugin && PluginHelper::isConfigurable($plugin)) {

These can now be simplified back to just the check, as it can safely accept a NULL (it was needed before for the method call)

Good catch, I missed that...

+1 to RTBC! Thanks for the quick turnaround @mikelutz and thanks for catching it @joelpittet!

The patch looks good and applies successfully. +1 for RTBC.

The random JavaScript test failures seem to be getting worse lately..

Second time lucky.

Committed 4bb82a8 and pushed to 8.7.x. Thanks!

It'd be great if someone could update the release notes snippet for this.