Disabled Date value not properly handled

Critical item is that this is for a disabled field. Disabling form elements has known issues that I'll have to dig into later.

Same problem in 6.x.2.7. Thanks for the patch. It seems to work good. Thanks KarenS for your great modules. We really appreciate them.

I need more information about how to reproduce this. There are lots of ways to attempt to disable a field. I have no idea what was being done.

No response and I cannot reproduce the issue.

Exist in D6 version too (6.x-2.7 version). Didn't check latest, but almost 80% sure that it exist there too.

The bug is still there. Steps to reproduce:

• Add a date field, use Pop-Up widget.
• Use custom code to disable the field, in my case some hook_form_alter() contains:
  

  $form[field_name]['#disabled'] = TRUE;
  

  (actually that code is conditional on access schemes.)

• Try to submit the node (or user): the date_popup_validate() will error as the input is a string and not an array.

Patch to handle this is attached! Thanks!

patch with array_key_exists() safeguard

8: date-input_disabled-1190830-7.patch queued for re-testing.

Why not do it this way instead, it's more readable and I think a more standard way of achieving the same thing:

if (!empty($element['#disabled'])) {
  return;
}

you are right the php manual on empty() states that "empty() does not generate a warning if the variable does not exist."

Lets move comment to it's own line as per https://drupal.org/coding-standards#comment

Thanks Zany for patch. Vijay Updated code as per coding standard.

Has anyone tested this with Javascript? The original report specifies that is how they came across the problem, which would not be solved via checking for the #disabled FAPI attribute.

If not, I'll check it out within the next day and see if I can reproduce it that way.

Thanks @stickywes. We can RTBC, once it pass manual testing.

Please note that this patch does not need manual regression testing. Testing would be need to figure out how to invoke this bug from JS. (The OP doesn't seem to be around anymore though.)
My patch does in fact addresses a bug which hopefully has a similar code path.
I'll open a separate issue if that is better suited?

I have verified that JavaScript does cause problems, and am typing up code snippets and a summary.

The bug that seems to be "fixed" by the patch could be better addressed by additional root cause analysis since JavaScript methods of setting form elements to disabled also cause problems. I would argue that it should not be considered a separate issue.

Updating the title and description so that the issue more accurately reflects the current state of things. Note that my hand testing couldn't find problems with respect to #disabled. We could still use some functional tests, though, in case I missed something.

This module has a pretty significant history of issues with trying to validate disabled or hidden field widgets, and this seems like another one. I'd like to advocate that we find the root cause of the problem, and not just deal with what are essentially side effects of a larger issue. If we fix the root cause, the side effects should go away. Maybe... hopefully!

This patch will add tests that will identify issues with elements that have had "#disabled" set to true on the base form element for field widgets.

The patch also refactors DateFieldBasic slightly so that it accepts arguments. I needed to be able to enable a helper module.

index b2a45a0..18e9b65 100644
--- a/tests/date_field.test
+++ b/tests/date_field.test
@@ -12,7 +12,13 @@ abstract class DateFieldBasic extends DrupalWebTestCase {
    */
   protected function setUp() {
     // Load the date_api module.
-    parent::setUp('field', 'field_ui', 'date_api', 'date', 'date_popup', 'date_tools');
+    $modules = func_get_args();
+    if (isset($modules[0]) && is_array($modules[0])) {
+      $modules = $modules[0];
+    }
+    $modules += array('field', 'field_ui', 'date_api', 'date', 'date_popup', 'date_tools');
+
+    parent::setUp($modules);

The added module creates a node type and has a form alter hook that targets the node form for this node type, to ease creating disabled form elements.

This doesn't catch the JavaScript issues, obviously. However, it does confirm that #disabled is still an issue. My hand testing didn't catch it earlier because I did not test the "datetime" or "datestamp" types. Both (I think) of those actually generate PDO exceptions, regardless of widget types.
PDOException: SQLSTATE[22007]: Invalid datetime format: 1292 Incorrect datetime value: '' for column 'field_datetime_text_disabled_value2' at row 1: INSERT INTO ...

The test needs a little more work because it doesn't verify that the correct values have been saved yet, merely that validation and submit errors are not occuring.

@stickywes: If #14 is only treating the symptoms rather than the cause, have you any insight into what the cause is?

Ah, sorry, meant to follow up on this by now.

The root cause of the JavaScript issue is probably the same issue that causes #1419286: Using hide() on Date field form elements causes validation errors and caused other problems when #access = FALSE was set on the widget. The commit by Karen S to fix #1387890: Using form_alter to set a date field to #disabled removes the field from the form (http://drupalcode.org/project/date.git/commit/8450ad5)fixed the symptom with #access but didn't address what seems to be the underlying problem.

Which is: date_combo_validate relies on $form_state['input']. When an input is disabled or absent (either because hide() was called on the element or #access = FALSE), there is no $form_state['input'] because that part for the form state array represents user input.

It might be worth separating the JavaScript portion of this specific issue out, since that would be addressed if/when I fix #1419286: Using hide() on Date field form elements causes validation errors, and addressing whatever is causing a PDO error when #disabled = TRUE as a completely separate issue; I haven't really encountered the PDO exception before so I haven't explored the causes there yet.

I'll be working on dealing with the combo validation callback and its assumptions about user input this weekend (US time), but the quick fixes I've tried so far have just lead to additional issues. If I recall correctly, after "fixing" that assumption the field then began to handle timezone offsets incorrectly and every time a node form was saved the time would be shifted several hours forward/backwards.

Another thing that complicates fixing the issue I reported back in the day with hide() is that it means carefully peeling back some code in an old commit while trying to not cause regression (which means writing more tests).

So (my main interest) hook_form_alter() + #disabled also results in this PDO error?

And fixing what you're talking about (sorry I don't really follow what exactly this is) would also fix this?

i.e. Why can't we keep the scope of this issue to fixing this specific error as a nice quick fix, and if you can come up with a super amazing improvement to the entire module that touches hide()/PDO/combo/timezones/etc as a separate follow-up.

Yeah, this issue got pretty muddy, didn't it! Let's just rehash what we've gone over so far, in hope that it gets made clearer.

1. Originally this issue was submitted because using Javascript to disable elements was causing errors. However, it looks like that was originally a D6 issue that got submitted as a D7 issue and Karen couldn't reproduce it.
2. Then comment #7 suggested a way to reproduce the problem as well as a patch.
3. The test I wrote, which reproduces the steps defined in #7, generates a PDO error. However, it should be noted that the PDO error is not the error that was originally reported, so this makes things a little confusing. Since we're modifying render array elements, this also isn't necessarily addressing the Javascript issue.
4. A hand test of Javascript behavior, which is what the original issue was reporting about, adds another layer because Javascript disabling causes a different issue. Making things even more muddy, unfortunately!

If we move forward and let this specific issue only be the PDO exception, which I'm fine with:
We know that #disabled will cause a PDO error. Do we know why there are inappropriate values being saved to the database? Checking for #disabled will make this bug report go away but doesn't actually address the defect.

If that's good enough for everyone else, sure, let's move forward and mark this fixed after the tests start passing. I realize that root cause analysis can require a significant time investment and this is an open source project that people are contributing their limited free time to. Just understand what the potential costs are when you pick quick fixes: messy code that gets hard to maintain (I feel like this module is already there) while also making other potential related bugs more challenging to fix.

In an effort to provide clarity I'm trimming out a bunch of irrelevant things I included in my last issue description update.

Completed my test and rolled #14 into a single patch.

Is patch date-arrayisempty.patch #21 will be applied to next release?

Did you test it manually to see if it fixed your/the issue? Did it work?

Well #14 works for me, clearly the tested in #31 are having some problems.

Did you test without #14?

I just did a quick manual test this with a Standard install profile, enabling date and date_popup.
I added 6 fields to the basic page (ISO and Unix types with 3 different widgets), and disabled them all with a custom module.

No validation errors happened. Even without #14.

This issue might have been fixed as a side-effect of other issue fixes.

Okay, maybe not...

@stickywes
So any idea what's wrong with the test?
Are you still working on this?

I haven't looked into it yet, no. But I have time to tonight (US time).

The test itself is pretty straightforward, so I think I need to look into the differences between my field configuration when I hand-tested things and the configuration of the fields done during the test setup.

Looks like it wasn't as simple as I thought. Somehow the values are getting through validation with the "value2" column winding up blank even when I seem to have replicated the field configuration exactly, which was the behavior that used to occur during manual testing.

I finally figured out how to make this happen outside of automated testing again.

I'm creating a Feature to capture the specific field configuration so it is easier to confirm and will post config specifics when I do.

Made a sandbox: https://www.drupal.org/sandbox/stickywes/2531848

If you grab the code and enable the Feature, you'll have a node with a few fields configured that will cause the SQL error when you submit them.

If you don't want to deal with the Feature, then add some date fields to a node; a unix date field with a text widget, a date field with a text widget, and a ISO date field with a text widget.

The setting that sets it off is checking "Collect an end date", and then making sure the default end date is set to "No default value." See the attached pic to see.

Disable these fields you've added in a custom module (or grab the sandbox):

/**
 * Implements hook_form_alter().
 */
function date_form_disabled_form_alter(&$form, &$form_state, $form_id) {
  if ($form_id == "date_field_testing_node_node_form") {
    $form['field_date_text_w_end_date_no_va']['#disabled'] = TRUE;
    $form['field_unix_text_w_end_date_no_va']['#disabled'] = TRUE;
    $form['field_iso_text_w_end_date_no_val']['#disabled'] = TRUE;
  }
}

Okay so it's been quite a while without update, stickywes said he was fine with keeping this issue in-scope, presumably the extra stuff he found will make it into a follow-up issue at some point, but for now I'm going to try marking #14 as RTBC on the basis that it passes manual testing.

If we keep the issue scope narrowed it can be described as the following: using Javascript to disable date field inputs generates a PDO Exception...

It seems like the JavaScript problem got fixed in some other patch somewhere, so this issue can be closed, I guess? I kinda feel like I stole a patch credit from zany by slowing down this issue...

Here is some manual testing without patch #14 applied by disabling the fields via my browser's debugger/inspector since I don't have time to write the JavaScript.

Popup widget tests

• Date with default settings: No crash. Reports validation errors.
• Date (Unix) with default settings: No crash. Reports validation errors.
• Date (ISO) with default settings: No crash. Reports validation errors.

Text field tests (with "collect end date" checked and "no default value" selected for default value):

• Date with text field widget: No crash. Reports validation errors.
• Date (Unix) with text field widget: No crash. Reports validation errors.
• Date (ISO) with text field widget: No crash. Reports validation errors.

I skipped select list because that has less of a troubled past (and I'm outta time!)

I need to find a new home for the non-JavaScript trigger. Leaving my notes here for copying to whatever issue turns out more appropriate by me or someone else (but probably me).

**********************************
Here are the manual steps to make it happen without JavaScript.

1. Create a new content type node.
2. Click 'Save and Add Fields'
3. Add a new field: Name: 'Unix Test' Type: Date (unix timestamp) Widget: Text field
4. Click 'Save'
5. Check the 'Collect an end date' checkbox
6. Click 'Save field settings'
7. Expand 'More Settings and Values' Fieldset
8. Change 'Default End Date' select list to 'No default value'
9. Click 'Save Settings'
10. Configure a module to set that field to disabled.
11. Go to /node/add and click the link to create your new test node
12. Add a title, toss in some text in the body field, and then click 'save'

At this point, you will experience the PDO Exception.

I can't see what tests are failing for #31 but we just committed some fixes for test fails in 7.x HEAD (@see #2646646: Tests are broken on current 7.x-2.x branch). Can we re-roll the patch to test again?

vijaycs85: The patch that was RTBC is #14, stickywes went off on a tangent to try and rewrite the entire system, but by his own admission doesn't have the time to finish it, hope that helps.

Rerolled the patch.

Six test failures, so this needs some work.

Re-roll without tests.

Edit: Nevermind.

I realize there's still work to be done, but 55 is working for me, thanks!

The patch is missing all the tests from #52