Problem/Motivation

hook_webform_component_info() allows developers to define components, including the component's conditional type. See below from webform.api.php:

    // Specify the conditional behaviour of this component.
    // Examples are 'string', 'date', 'time', 'numeric', 'select'.
    // Defaults to 'string'.
    'conditional_type' => 'string',

All functions needed for the 'string', 'date', and 'time' conditional types are stored in the webform.conditionals.inc include file; however, in the case of 'numeric' and 'select', the functions needed for their conditional behavior are stored in their respective include files - number.inc and select.inc. As a result, when a module defines a conditional type of either 'numeric' or 'select', then the conditional operator callback will call one or more functions that are not loaded. This will cause a fatal error. (Exception: a 'number' or 'select' component has been added to the form, which would cause the needed include file to be included)

In the case of the 'numeric' conditional type, several conditional operator callback functions call webform_compare_floats(), which is stored in number.inc.

In the case of the 'select' conditional type, several conditional operator callback functions call _webform_select_options(), which is stored in select.inc. It, in turn, calls three additional functions that are also stored in select.inc.

Proposed resolution

There are basically two solutions:

  1. Move the functions required for conditional behavior to webform.conditionals.inc.
  2. In WebformConditionals::executeConditionals, check for missing include files and then include them.

Remaining tasks

Maintainer feedback

User interface changes

None anticipated

API changes

None anticipated

Data model changes

None anticipated

Comments

Chris Burge created an issue. See original summary.

chris burge’s picture

Status: Active » Needs review
StatusFileSize
new2.23 KB

Attached is a patch that implements solution #2.

Status: Needs review » Needs work

The last submitted patch, 2: webform_conditionals_missing_includes-2814119-2.patch, failed testing.

chris burge’s picture

My client is running Webform 7.x-4.12, which doesn't include #2757751: Sub-conditional and/or expressions. (This is why the patch failed to apply.) I'll retest against DEV and post back.

chris burge’s picture

Status: Needs work » Needs review
StatusFileSize
new2.36 KB

This issue continues to exist in 7.x-4.x-dev. Attached is patch rerolled against HEAD.

mfuggle’s picture

I have a webform in which I have an date field and an image upload field. I want to display the image upload field based on the date being later than a certain value. The date consists of select boxes for day, month and year. Am I correct in assuming the issue under discussion here will prevent me from succeeding with this action?

chris burge’s picture

@mfuggle - I don't see anything in your use case that would be affected by this issue. This issue is specific to cases where a developer writes a new Webform component. The functions needed for 'date' conditionals are already available in all cases (because webform.conditionals.inc is always loaded).

liam morland’s picture

Status: Needs review » Needs work

Thanks for the patch. The "no additional files" case statements can be combined. module_load_include() has its own built-in check for whether a file has already been included, so an additional check is not needed here. Please check the coding standards: There is supposed to be a space after "if".

chris burge’s picture

Status: Needs work » Needs review
StatusFileSize
new1.26 KB

Thanks for reviewing the previous patch.

Attached patch 1) removes unnecessary include check (since module_load_include already handles this for us - I see that now that I look at the function), 2) combines the duplicate case statements, and 3) has been run through Coder.

liam morland’s picture

Status: Needs review » Needs work

Thanks. you can do it with:

module_load_include('inc', 'webform', 'components/' . $conditional_type);

Perhaps use in_array() or at least don't even have a case for types that require no action.

chris burge’s picture

Unfortunately, the suggested code in #10 won't work. The conditional type name don't match up with include file names. For the 'numeric' conditional type, the conditional type is 'numeric' and the include file is 'number'. There is no 'string' include file.

The purpose for providing a case for conditional types that require no action is to self-document, to show that we have accounted for all of the conditional types that ship with Webform. This could be moved to a comment. E.g.:

// In the case of the 'string', 'date', and 'time' conditional types, it is not necessary to load their include files for conditional behavior because the required functions are already loaded in webform.conditionals.inc.
liam morland’s picture

Indeed, "numeric" and "number" are different; sorry, I didn't notice.

I would rather go with the comments for documentation.

chris burge’s picture

OK. I'll submit an updated patch later today.

chris burge’s picture

Status: Needs work » Needs review
StatusFileSize
new1.37 KB

Attached patch per #12.

liam morland’s picture

Status: Needs review » Fixed

Thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

vasyok’s picture

Issue summary: View changes

patch not aplied to the current version

chris burge’s picture

@VasyOK. Patch #14 was included in Webform 7.x-4.15. There's no patch to apply. This issue has been resolved.