Decouple ExecutableInterface from Conditions and FormInterface

It doesn't make sense to me that Condition extends Executable (in any sense: interface, base class, or manager). Evaluating a condition is not conceptually similar to executing something.

Well, the motivation for that was totally to be able to share code between actions and conditions.

Technically seen executing actions or evaluating conditions really is the same - it's just that the one returns a boolean while actions could return anything (and conditions are not expected to change anything). Thinking of correct sub-type relationships I think conditions could be even modelled as a sub-type from actions, as you could use a condition in an action context but not the other way round. Getting back from theoretic CS to the real world, conditions and actions are generally perceived as separate concepts, so sub-typing conditions from actions would be confusing.

Getting back to executables, I have to agree that the wording "executing a condition" does not fit so well. Is it likewise confusing? To me, having a common way to execute an action or condition was appealing, but I do not think having that is something really necessary.

Given that we have factored out much of the commonalities on the plugin-layer already it might be possible to totally scratch the concept of executables and go with evaluating conditions and executing actions once we solve #1920816: Factor getConfig(), setConfig(), getConfigDefinition(), and getConfigDefinitions() out of ExecutablePluginBase also.
One possible problem I see here is if we'd support condition groups in core (not sure what's the roadmap here?), as for that Rules we'd need a way to intercept condition evaluation/execution for providing it's debugging features - what right now could be done by proxying the execution-managers execute() method. I guess that could be solved differently also though.
Another thing we right now have at the executable-layer is the summary, but as it's summarizing the configured contexts I think that could be moved to the contextual plugins layer also.

Furthermore, it's a WTF that $condition->execute() returns the exact opposite of $condition->evaluate() when the 'negate' configuration is true. $condition->evaluate() should take all its configuration into account, including 'negate'.

Indeed - still we need a simple way for people to implement the condition evaluation logic without having to care about the general negation feature.

+1
I wanted to use Executable for Actions/Operations, but it seems way too intertwined.

Mostly because ExecutableInterface::setExecutableManager() is only used by the Conditions system.
There are no decoupled tests to show how someone else could use this.

Also, why is ExecutableInterface forced to extend FormInterface? This means that all executables need to provide forms... Why?

First step.

Okay, here's the full patch. Talked this through with fago, he agreed with moving these to ConditionInterface, since conditions are the only ones that need that yet. If eventually other plugins want to do those things, we can abstract it out elsewhere.

Note that ExecutablePluginBase still indirectly implements ContextAwarePluginInterface, but that is fine, you are not forced to extend that.

I missed one addition of FormInterface to ConditionPluginBase.

+++ b/core/lib/Drupal/Core/Condition/ConditionInterface.php
@@ -32,4 +33,17 @@ public function isNegated();
+   * Provides a human readable summary of the executable's configuration.

should be condition now

+++ b/core/lib/Drupal/Core/Executable/ExecutableInterface.php
@@ -7,41 +7,14 @@
- * Executable plugins are context-aware and configurable. They support the
- * following keys in their plugin definitions:
- * - context: An array of context definitions, keyed by context name. Each
- *   context definition is a typed data definition describing the context. Check
- *   the typed data definition docs for details.
- * - configuration: An array of configuration option definitions, keyed by
- *   option name. Each option definition is a typed data definition describing
- *   the configuration option. Check the typed data definition docs for details.
- *

This documentation needs to move over to conditionInterface as well then and talk about conditions.

Ah yes, thanks.

+++ b/core/lib/Drupal/Core/Condition/ConditionPluginBase.phpundefined
@@ -8,11 +8,12 @@
+abstract class ConditionPluginBase extends ExecutablePluginBase implements ConditionInterface, FormInterface {

Given what you've done here I think I'd be more comfortable with the ConditionInterface extending FormInterface. You had also spoken of a SubFormInterface in Portland and I think that's perhaps a thing we should do first. Conditions need a form almost always and those that don't still need to respond to the methods, so it's not a thing the Base class can be doing, the interface actually needs to force it. If you want to move that out of ExecutableInterface, ok fine whatever I don't really care, but the Condition DOES need it.

+++ b/core/lib/Drupal/Core/Executable/ExecutableInterface.phpundefined
@@ -7,41 +7,14 @@
-interface ExecutableInterface extends ContextAwarePluginInterface, FormInterface {

Kind of not ok with removing the context aware stuff here. Need a bit of justification.

Other than what I've mentioned here I don't see anything odd here. If we can square away these two points I'm pretty ok with what's being attempted here.

Eclipse

I talked to @EclipseGc, and moved the FormInterface dependency onto ConditionInterface while opening #2006248: Add an EmbeddableFormInterface so that FormInterface can delegate to multiple objects to provide a better one.

I articulated my concerns about removing ContextAwarePluginInterface, it was mostly confusion about the resulting interfaces on ConditionPluginBase, which weren't clear from the patch.

If that passes, I'm completely on board.

Eclipse

#12: executable-1920822-12.patch queued for re-testing.

It claims it failed to apply. Which is nonsense. To the end of the line :(

Committed 0c5ebea and pushed to 8.x. Thanks!