Setting parent of plugin to incompatible container removes its current parent

Tested adding an action to a conditional with #1:

• #1666802-2: Keep embedded elements under compatible containers in container UI: Works as expected.
• #1666802-25: Keep embedded elements under compatible containers in container UI: Isn't necessary, as to be expected, because you can't add an action to a conditional.

Hmm. I found the relationship between making RulesPlugin throw an error if you try to apply an invalid parent plugin setting along with RulesContainerPluginUI making options like these unavailable in the UI to be very interesting. These seem almost like implied behaviors, whereas it could be possible to integrate the concepts in a deeper way and expose this functionality through Rules' API and UI API with more reused functionality. Could an abstract plugin instances' declared embeddables be reflected in entity_info and these patches be based on that?

What about adding a property to Rules Configurations to represent the embeddables provided to RulesAbstractPluginDefaults? Perhaps, both patches could also be reworked similar to what I said above, and then another could rework checkParameterSettings()? This is getting significantly above my head now. But wow, that //TODO: Make sure used values are allowed. (key/value pairs + allowed values) almost matches excatly!

I'll reply to #4 separately.
Edited slightly.

> My personal judgment says 'reaction rule' should be embeddable in 'event set'.
This seems like a perfect match to the topic in #841336: [Meta] Interchange with other rules languages by aligning concepts.

Edit: And I think it should be a separate issue.

> I am unsure as to what you meant about #1666802-25: Keep embedded elements under compatible containers in container UI being unnecessary.
Oh, I see how my words came out now. I meant to say that testing #25 with this patch isn't necessary, because it prevents this error from occurring. That's all.

+++ b/includes/rules.core.inc
@@ -394,25 +394,27 @@ abstract class RulesPlugin extends RulesExtendable {
-    if ($this->parent == $parent) {
+    if ($this->parent === $parent) {

unrelated change?

But otherwise I think this makes sense, we should definitively replace those hard-coded Action/Condition classes and interfaces. Marking as RTBC, but leaving for fago as he knows this one better than me.

In RulesPlugin::setParent(), the existing parent is removed before the check for compatible containers. In the odd instance of setting the parent of an action to an incompatible container, if the RulesEvaluationException is actually caught, the existing parent is forcibly removed before the error check.

ouch. yep let's fix that, however the patch does much more than that.

+++ b/includes/rules.core.inc
@@ -394,25 +394,27 @@ abstract class RulesPlugin extends RulesExtendable {
-    if ($this->parent == $parent) {
+    if ($this->parent === $parent) {

As klausi said, unrelated. Also I don't think we should do that, as the UI might treat with some serialized and unserialized configs.

+++ b/includes/rules.core.inc
@@ -394,25 +394,27 @@ abstract class RulesPlugin extends RulesExtendable {
+    $pluginInfo = $this->pluginInfo();
+    if (empty($pluginInfo['embeddable'])) {
+      throw new RulesEvaluationException('This element cannot be embedded.', array(), $this, RulesLog::ERROR);
+    }

Embeddable is supposed to be an UI-helper-flag, whereas the API only limits embedding things to the interfaces.

Any reasons why we should change that?

Okay the problem with using 'embeddable' is that RulesEventSet extends from RulesRuleSet and uses RulesRuleSet::rule() to add a rule, which adds it as an action, which in turns tries to call setParent().

Exactly, I doubt this would work. So why was that RTBC?

Of course, I can create that as a separate issue.

Yes, please. As said I'm hesitate of doing so as I fear it might break the UI - see below.

What did you mean by "some serialized, some unserialized"?

During editing Rules in the UI configurations get serialized, while others get loaded. So one might end up having two different objects of the same configuration instantiated. I guess, taking the passed parent would be the right thing to do in any case - but it needs solid testing to make sure nothing breaks.