Date module select lists and text fields are required even if they are hidden (their controlling value is not selected).

I believe the problem stems from how a Date field is specified as required:

Results of print_r($form['field_birthdate']):

Array
(
    [#tree] => 1
    [#weight] => 0
    [#validate] => Array
        (
            [date_widget_validate] => Array
                (
                )

        )

    [0] => Array
        (
            [#type] => date_combo
            [#field] => Array
                (
                    [field_name] => field_birth_date
                    [type] => date
                    [required] => 1
...snip...

With this there are two issues:

  1. Conditional Fields looks for '#required', but not 'required'
  2. Conditional Fields uses element_children() and so does not look at #field property children.

Should the Date module should change to represent required in a normal way? Or should Conditional Fields accommodate the Date module?

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

threexk’s picture

After further research, the Date module does use the #required property for all its form elements. However, it is done by a #process function, which is called after hook_form_alter() where Conditional Fields looks for #required.

At the hook_form_alter() stage when Conditional Fields runs, a date field is sort of a super field that can consist of multiple normal fields; at the #process stage it expands the super field into all its constituent fields.

I found this diagram really helpful for visualizing the sequence: http://drupal.org/node/165104

I am now wondering if Conditional Fields's hook_form_alter() code should instead be executed at the #afterbuild stage, so it deals with the final, processed form (hook_form_alter() could register a #afterbuild callback.) Will need to think about it more...

threexk’s picture

I hereby offer a $5 bounty to anyone who can fix this issue within two months. Payment would be via PayPal.

The money probably is not commensurate with the work, but perhaps it will provide some extra motivation to someone who was already thinking about fixing it.

peterpoe’s picture

Status: Active » Postponed

Postponing all compatibility with non-core cck modules to future releases.

mitchell’s picture

Version: 5.x-1.x-dev » 6.x-1.x-dev
Status: Postponed » Postponed (maintainer needs more info)

Please try the latest releases of both modules and update this issue.

peterpoe’s picture

Title: Date module fields required even if hidden » Compatibilty with date module
Category: bug » feature
Status: Postponed (maintainer needs more info) » Postponed

Please report here any incompatibility with date module.
Please note that work on compatibility issues with non-core cck modules is postponed after the release of Conditional Fields 1.0 (but patches are welcome of course).

GuyPaddock’s picture

I'm still seeing this behavior with the Date and Location modules. Both field types have nested fields with their own #required attributes, but conditional_fields_node_editing_form doesn't seem to recurse.

I should note that the date and location fields are being used within field groups, and it is the field group that has the conditional field setting (i.e. when another field has the right value, the field group becomes visible).

akolahi’s picture

Using Conditional Fields 6.1-dev (Aug 15 2009) and Date 6-2.3 Initial testing is showing that i am able to set a date field as a conditional field that is properly triggered. I tried saving the field triggered and not triggered and so far is appearing to work properly.

Will test further and will report of any strange behavior.

akolahi’s picture

If the Date field is required I am getting an error message saying the date field is required even though the date field is not triggered and thus is unavailable.

If the date field is not required then it looks like it is working properly - at least thus far in my testing...

akolahi’s picture

I'm willing to make a financial contribution to getting the this issue fixed and the date module compatible with conditional fields. Let me know if your interested.

cdale’s picture

Status: Postponed » Needs review
FileSize
12.32 KB

I've done some work on this. I have not extensively test this, so please use with care. I think it should for the most part work however.

cdale’s picture

Small change

cdale’s picture

Missed the returns in the after build. They now return the form, instead of nothing.

cdale’s picture

Last patch has a typo on both _form_validate() lines which causes the required checking to not work. #conditional_date_fields should be #conditional_controlled_fields.

Updated patch attached.

cdale’s picture

I've done a bit more testing, and I think this is working quite well now with the last patch. It doesn't seem to affect anything else, and appears to make date fields work as expected.

The patch itself could probably be re-factored a little bit to reduce duplicate code and checking of conditions, but I don't quite have the time to do that at the moment.

cdale’s picture

Sorry for so many unanswered posts and renditions of this I believe this one is the right patch now. :) The last one would work, this one just cleans up a variable that was made which was never used.

PATCH SUMMARY

  • Changed the node editing form alter to be run in an #after_build. This should catch any modules which expand their fields in a #process callback.

After build callback (Was form alter).

  • Adds a copy and a reference of each controlled field in the afterbuild for use in validation callbacks. (See below)
  • If the controlled field type is 'date', then set the field to be validated right away in the afterbuild, and check the validation later ourselves. (This avoids duplicate validation errors being output).

Validation callback

  • If the controlled field is not triggered, use CCK's default value callbacks to get the default values. If it is a date field, then do some extra work to handle the default value, and finally set it not only in form state, but also in #value on the elements so that if there is a validation error, the form is rebuilt correctly. (This was the reason for the by ref copying).
  • If the controlled field type is a date, then validate the field now (This was the reason for the copied value above, along with the by reference). We have to do this, as part of the date validation routines use #value on the elements to check, so we had to update it before validation could go on.
  • Finally, check the required property on date now that it's value has been reset

Having written that, I'm not 100% sure on the logic behind needing to re-validate the date fields manually. I know as I was putting this patch together there was a reason for it, but now that it's finished, I might need to recheck if that is actually required.

cdale’s picture

Ok, so I just checked what I wrote above, and it turns out I was right, the logic is wrong. Here is what I hopefully think will be the last patch. :)

SUMMARY

  • Form alter made into an after build
  • In after build, a reference to each controlled element is placed in $form
  • In validation, if the field is not triggered, CCK Default value gathering is now used
  • If the field is a date type, then we use a custom process to get the default value
  • We set the default value in $form_state AND on the element copy in form. We have to do this as if there is a validation error, then #value on the element is used to rebuild the form, not $form_state
cdale’s picture

Realized that XOR logic was a bit miffed.

Corrected.

peterpoe’s picture

Version: 6.x-1.x-dev » 6.x-2.x-dev
Status: Needs review » Needs work

@cdale: in 2.x-dev branch I used your suggestion of moving alteration to after_build. Please use the 6.x-2.x-dev branch to test compatibility fixes.

The alteration function has been completely rewritten for greater maintainability and to prepare the code for new features.
To add compatibility with other modules, like your work with Date module, we can't just add the code in the main function, otherwise the module would become in short a messy nightmare, with exceptions for every widget and so on.
We must think about a plugin system so every non-core cck widget has its own include file and can be maintained separately.
Do do this we must identify the things that conditional fields needs to know to guarantee compatibility. My ideas are still not clear on the subject, so contributions are welcome!

cdale’s picture

The only piece of code in that patch specific to the date module was the default value setting. Normally, the CCK callback checking should work, however the date module appears to do its checking in its #process callback.

The main problems I faced when adding this compatibility with the date module were the following:

  1. Fields and widgets added in #process callbacks did not obey conditional required settings. (Fixed by using #after_build).
  2. Setting the default value of non-triggered controlled fields was problematic, as various modules potentially expect different structures at different stages. The problem here was mostly apparent on validation errors, as the form state appeared to be obeyed on submissions.
  3. On validation errors, the form is not rebuilt, just re-rendered, which means for any fields which use #process callbacks, they may actually check $element['#value'] for the value they should use. I fixed this in the above patch by also setting the value put in form state, on the element itself.

I can't think of any other challenges that were faced, but perhaps this gives you a starting point? I might get a chance to look at a plugin system, perhaps using ctools or something similar.

What do you think?

djdevin’s picture

@cdale: we had the same issue with date fields being required even though they were hidden by a controlled field - your patch worked perfectly. thanks!

patched against conditional fields 6.x-1.1, using date 6.x-2.4

thekevinday’s picture

The date fields do not completely work properly under 2.x.

Specifically, the "required fields" do not work properly when the required field is disabled by a conditional field.

When trying to save, when a date field is required but disabled, then all fields after that field fail to be saved properly and end up with having their values set to NULL or an empty string.

If I have 3 fields:
1) a true/false field (controlling #2)
2) a date field (controlled by #1)
3) a text field

When #1 and #2 are in two different groups:
G1:
- #1
G2:
- #2
G3:
- #3

During the form submit process if #1 is false (and #2 requires #1 to be true) and #3 has "Example Text" in its field, then on save #3 will be empty because #2 had some sort of error for being required but unfilled.

YK85’s picture

subscribing - has anyone been able to further develop compatibility with date module? Thank you!

chalee’s picture

I have this problem too

threexk’s picture

Status: Needs work » Fixed

This seems to be fixed in the current 6.x-2.0-beta2. Anyone disagree? I no longer have a problem with a controlled Date field being required when it is not triggered.

Status: Fixed » Closed (fixed)

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

timlie’s picture

When I have a date field and the controlling field doesn't trigger this date field, the value which is previously saved for this date field stays in the database...

scottrigby’s picture

This is a rather old issue, but still a problem on D6 sites - normally this would be fixed first upstream (7x) then backported to 6x. But i'm not sure this is even an issue in 7.x-3.x?

In any case here's a patch against 6.x-2.x, which - as @peterpoe requested in #18 - adds a new function that calls a simple plugin system to help keep things tidy.

The code is self-documenting, but in short, it starts by adding a new function which:

  • Invokes a pseudo-hook WIDGET_MODULE_conditional_fields_reset_default() (Supports specicic non-core CCK widget modules in separate includes)
  • Invokes hook_conditional_fields_reset_default_alter() (Allows other non-core CCK fields to provide support for Conditional fields)

Date support is added in an include, which implements WIDGET_MODULE_conditional_fields_reset_default() - that function is basically what @cdale initially wrote, just isolated and easy to now support additional non-core CCK widget modules.

This passed testing on our end, but would be good to get further reviews and feedback :)

scottrigby’s picture

Status: Closed (fixed) » Needs review
scottrigby’s picture

New updated patch.

Shouldn't include field widget module plugin files more than once.

-      include($file->filename);
+      require_once($file->filename);
peterpoe’s picture

Issue summary: View changes
Status: Needs review » Closed (outdated)