Add "required" (allow null/empty) option to component parameters

Would it make more sense UX-wise to do the opposite and say "Required" (ticked by default)?

This is the pattern used by Web service client (EDIT: though not sure about the "ticked by default").

Looking good to me. I see some coding style cleanup:

• 'allow null' => !$item['required'] should be 'allow null' => empty($item['required'])
• isset($info['allow null']) ? !$info['allow null'] : true is equivalent to !isset($info['allow null']) || empty($info['allow null'])

Also, there seems to be one tab too many in the $form['required'] array body, i.e. there are 8 spaces when there should be 6.

Just being a bit pedantic. Otherwise the patch works just fine. I'll RTBC the issue once my remark is taken care of.

Also, @mitchell, do you think this needs tests, or is this covered by the allow null test (notwithstanding the UI widget)?

Cool.

This seems like it'll be very useful, but I don't yet have a handle yet on when it makes sense to 'require' parameters or provided variables. The example in OP is no longer valid, because "allow null" will be afforded by default, if unticked is default. So, the question is, how would "Required" values be used? Cool twist.. I'm fond of examples with a system message that makes some sense, e.g. showing a user's last login date. And... what if it's a user's first login?.. Good example? D'oh, no. Again, that's "allow null." Shux.

I think the unticked default is a good choice, because then users wouldn't have to learn how to use "Required" values until they needed that feature. Also, would the introduced "allow null" default behavior affect any existing configurations?

> do you think this needs tests, or is this covered by the allow null test (notwithstanding the UI widget)?
Grepping didn't reveal an allow null test. Could you please link? UI widget tests are not something I'm not familiar with at all. On "needed tests", I would prefer to look to your recommendations. Mine is just general, that if you do decide to make one, please look at it also from the docs side of things. I also advance this, because in my case, I don't understand it yet, and I know 2-3 examples would knock it out of the park.

While I understand that the test cases in Rules Test, which are like rules test case 1 expects 'variable one' to show message 'test 123', are primarily there for testing, though IMHO, it'd be a direct way to solve two hungry birds with one rice grain (ha). It also gives the 80%(?) of users who might look at the code more to 'get' when they read it, which could help them to become developers after having more meaningful insight and staying motivated to dive in further. Maybe I should start a fork of Rules with Functional Tests if I'm really willing to see this through.

Either way, I think this is rtbc, and we can decide, latest, upon commit, if it 'Needs tests', at least based on insights from my recreational Core Watching.

> 'allow null' => !$item['required'] should be 'allow null' => empty($item['required'])
¿Because 'allow null' is good DX, while 'required' is good 'UX'? Wasn't expecting that, but makes sense.

--Tagging with Docs tag, so discussion can re-continue past commit fix. Are the API tags right?

This isn't really an API change, so much as a new UI feature. If you look at rule() docs, it specifically points out the parameter array structure is the same as in hook_rules_action_info(), i.e. 'allow null' is documented there.

The checkbox is there to allow configuration of components not every one of whose parameters must absolutely be non-empty for the basic preconditions of the component to be satisfied, i.e. Rules will not abort the component for you if a non-required parameter is empty. An example would be to set a few attributes (i.e. field values) of a node, some of which are not required; "Set a data value" should abort, but the component itself shouldn't.

Now that I think about it, 'required' is actually another thing isn't it? "Required" should technically mean the inverse of "optional". Perhaps, then, a checked "required" option should be inferred as empty($info['allow null']) as well as empty($info['optional'])

@OnkelTem: If we are in agreement, could you update the patch to reflect this?

Then again, the flipside is whether to label the checkbox "Optional". This will still have to be used in conjunction with "allow null" simply for the scenario in which another component will necessarily pass a parameter, but empty value is possible.

Opinions welcome, particularly regarding the DX/UX distinction.

Thanks. I also meant:

+          $vars[$item['name']] = array('label' => $item['label'], 'type' => $item['type'], 'allow null' => $item['optional'], 'optional' => $item['optional']);

Just for a more well-rounded UX. This way, the parameter can be either just an empty value or left unconfigured altogether.

What do you think?

So where did this get to?

(Sorry I haven't been on IRC lately.)

After digging through Rules internals for 5+ months, I've discovered that "optional" can potentially screw with how actions can be executed. Specifically, the number and order of arguments for an action can matter if an action is programmatically executed with variable arguments for RulesAction::execute().

So, actually making a parameter optional (i.e. no "required" marker when executing) is out. That leaves the ambiguity across "Optional" and "Allow null". Given Rules practically uses "empty" to mean "null", I agree that "Allow empty" is best. This fits in with Rules lingo as far as the separation of UI terminology from developer complexity is concerned.

Differences in this patch:

• The 'optional' form element key is now 'allow_null'.
• The 'allow null' value for a variable is only set if it's a parameter and "Allow empty" is actually ticked.
• The "Allow empty" checkbox is only enabled if the variable usage contains "parameter" (which obviously is always true for condition sets).
• The "Allow empty" checkbox is now after the variable usage, considering that it's only applicable if the variable is indeed a parameter.

Ach... The "Allow empty" checkbox is enabled even if the entire row is disabled due to '#states'.

This patch contains the correct selector that applies only to enabled "Usage" select lists.

EDIT: Module-provided default configurations will have the checkbox disabled.

A component (i.e. rule) only recognizes 'optional'. So, even with 'allow null' a component still would not evaluate if the variable has a NULL value. This patch makes a plugin check 'allow null' when setting up state. I have tested null parameter values by importing XML data (via Views XML Backend) with no problem.

The alternative is to define both 'allow null' and 'optional' for a parameter when 'Allow empty' is checked (as I mentioned in #16). However, this makes "Allow empty" ambiguous when displaying the variable form in component settings.

Ideas? Opinions?

Updated issue summary.

Applied #21 manually and it works great. My component used to trigger an error because text field from entityform was empty, now it all works fine. No other errors seen anywhere. Thank you! Hopefully this gets committed soon :) it's been 2 years now ^^

Yep, adding this makes a lot of sense. A few remarks though:
- We need to add test coverage for the added hunks
- The issue summary talks about the UI using required, what I agree is a better from a UX standpoint. Let's make the patch match that.

Attach is a re-roll with the using "required" change on the UI.

I am not strong in drupal testing yet, and although this is as good opportunity as any, and probably a great one, I can't do it right now. Will try to do it at some point if nobody beats me to it.

Leaving it in still work.

Needs review only to see if last patch passes current tests.

I applied the patch manually on 7.29, it works. And I was pleased to see it did not break my rule previously configured with the "allow null" checkbox. Great!

Hi,
Do I understand this correctly - required (checked) - means that value must not be empty otherwise I rules throws an error? If unchecked, component can take empty value.

After applying the patch, it works for me only if there is some php ("return $value") in rules php evaluation field. Otherwise it throws error.

I thought I can "camouflage" the empty value by using php before it hits the component - inside rule using php evaluation:

if (empty($value)) {
        echo 'empty';
    }

However rules does not react on this.

So still needs tests.

I successfully implemented the #24 patch on D7.38 and rules 7.x-2.9.
I have a rule, which call a rule component, with one param set as "allow null". I am still getting the "Error: Missing configuration for parameter" on the rule, when calling the component, leaving the parameter empty.

This is the component code:

{ "rules_f_record_create_sub" : {
    "LABEL" : "F_Record Create sub",
    "PLUGIN" : "rule",
    "OWNER" : "rules",
    "TAGS" : [ "F_Record" ],
    "REQUIRES" : [ "rules", "rules_conditional" ],
    "USES VARIABLES" : {
      "p_rec_type" : { "label" : "Rec Type", "type" : "node" },
      "p_auto_record" : { "label" : "Auto Record", "type" : "node" },
      "p_trans_amount" : { "label" : "Amount", "type" : "decimal" },
      "p_shareholder" : { "label" : "Shareholder", "type" : "user", "allow null" : true }
    },

I exported the rule and manually fixed the code to have the param included and with the value "", and it pass the integrity check.

                      { "component_rules_f_record_create_sub" : {
                          "p_rec_type" : "341",
                          "p_auto_record" : [ "node" ],
                          "p_trans_amount" : [ "escrow-deposit" ],
                          "p_shareholder" : ""
                        }
                      },

Another test, I have done was to add the component into the rule, and leaving the param empty. I get the errors:

• User identifier field is required.
• Missing configuration for parameter p_shareholder.

I think the Ui should be fixed.

I applied the patch, modified one parameter definition of an existing component to allow NULL, but in a rule where the component is called I still can't leave the value empty. Maybe it works only in newly-created components?

I applied the patch in #24 and while it stopped throwing a fatal error when I was passing NULL for the optional parameter, I was still getting the warning "The variable or parameter %name is empty" in my logs and the component wasn't firing correctly.
After some debugging I realised that the data coming from the rule php evaluation to 'rules_unwrap_data()' function was missing the parameter information and thus not knowing that those parameters were optional.

I added an addition to the patch for the php.eval.inc "evaluate()" function to get the parameter information which seems to work for me for all my existing rules and components.

with rules v. 7.x-2.11 and the #33 patch applied I can specify a parameter as not required. However, in a form calling such component (with non-required parameter) the parameter input widget is still marked as required and an attempt to save such form with no value in that widget triggers a validator error (can't save the form). I do not know how other folks in this thread tested the patches, but for me they fail in the described scenario.

updated patch for last version of D7 rules 7.x-2.14