I created content type with date CCK field to use in signup. To support event preparation workflow, I configured this field so it can be left empty (required checkbox not ticked). That allows an editor to create an event entry as a proposal (without a date), have a workflow around it (like group review, approval, date confirmation, etc.).

I expect signup.module to disable signups for all nodes that have date not entered yet, but it still tries to show such nodes, links to signup, etc. A bigger problem is that it spews a bunch of warnings:

warning: date_timezone_set() expects parameter 1 to be DateTime, null given in /sites/all/modules/signup/includes/date.inc on line 488.
warning: date_modify() expects parameter 1 to be DateTime, null given in /sites/all/modules/signup/includes/date.inc on line 491.
warning: date_format() expects parameter 1 to be DateTime, null given in /sites/all/modules/signup/includes/date.inc on line 492.

CommentFileSizeAuthor
#6 signup.empty_date20090815.patch503 bytesiva2k

Comments

iva2k’s picture

iva2k commented
Status: Active » Needs review

I tracked code responsible for warnings to function _signup_date_node_completed () in file includes/date.inc:

function _signup_date_node_completed($node) {
  $field = signup_date_field($node->type);
  if ($field && $field != 'none' && isset($node->{$field['field_name']})) {
    // Grab whatever date value we actually have, regardless of format.
    $date_value = $node->{$field['field_name']}[0]['value'];

The fix is to add the following statement right after the above code:

    if (empty($date_value)) {
      return FALSE;
    }

(Please let me know if you'd rather have a patch file)

iva2k’s picture

iva2k commented

The above fix also applies to CVS HEAD.

dww’s picture

dww
we/he/they
commented
Priority: Critical » Normal
Status: Needs review » Needs work

Yes, a real patch would be lovely.

However, I'm not sure I'm sold on this approach. This seems like a band-aid over a deeper wound. If you have a node type configured where signup thinks a specific field holds the start date, but that field isn't required and you can effectively have untimed nodes, _signup_node_completed() isn't the only place you're going to have trouble.

It seems like a better fix is in _signup_date_get_node_scheduler(). In fact, that's already trying to handle this case, it just seems to be doing it wrong. ;) Anyway, that's the function to look at. If the node is configured with a CCK field as its signup start time, but that field is empty, _signup_date_get_node_scheduler() should return 'none', not 'date'. Then, in theory, none of the rest of this should be happening.

iva2k’s picture

iva2k commented

Thanks for the pointer. Will the below change (line 4) alone to includes/date.inc do the trick?

Was:

function _signup_date_get_node_scheduler($node) {
  $field = signup_date_field($node->type);
  if (isset($node->{$field['field_name']})) {
    return 'date';
  }
  if (isset($node->{$field['database']['columns']['value']['column']})) {
    return 'date';
  }
  return 'none';
}

New:

function _signup_date_get_node_scheduler($node) {
  $field = signup_date_field($node->type);
  if (isset($node->{$field['field_name']})) {
    return empty($node->{$field['field_name']}[0]['value']) ? 'none' : 'date';
  }
  if (isset($node->{$field['database']['columns']['value']['column']})) {
    return 'date';
  }
  return 'none';
}
dww’s picture

dww
we/he/they
commented

patches, please! ;)

I'm a little worried about hard-coding [0] in there, but I guess signup doesn't handle multi-valued date fields right now, anyway.

Better would be something like:

function _signup_date_get_node_scheduler($node) {
  $field = signup_date_field($node->type);
  if (!empty($node->{$field['field_name']}[0]['value'])) {
    return 'date';
  }
  if (!empty($node->{$field['database']['columns']['value']['column']}[0]['value'])) {
    return 'date';
  }
  return 'none';
}

Someone would have to actually test this and inspect the relevant data structures to make sure that's all right, but it seems clearer to just do the full check in one step instead of the nested thing you've got.

iva2k’s picture

iva2k commented
Status: Needs work » Needs review
StatusFileSize
new signup.empty_date20090815.patch503 bytes

Here's the patch. Please review.

mattyoung’s picture

mattyoung commented
Version: 6.x-1.0-rc4 » 6.x-1.0-rc6

I am getting maybe the same errors:

warning: date_timezone_set() expects parameter 1 to be DateTime, null given in /var/www/html/drupal/sites/all/modules/signup/includes/date.inc on line 485.
warning: date_modify() expects parameter 1 to be DateTime, null given in /var/www/html/drupal/sites/all/modules/signup/includes/date.inc on line 488.
warning: date_format() expects parameter 1 to be DateTime, null given in /var/www/html/drupal/sites/all/modules/signup/includes/date.inc on line 489.

However, my datetime field for signup is required so it's set to something for sure.

snorkers’s picture

snorkers commented

I'm seeing this too, but only since RC6. (had created new entry as hadn't spotted issue due to slightly misleading title #588046: Error if Date Field Null for content type that is Signup-enabled). My comment as below:

I have a content type Course that has an optional start/end date/time, and the content type is enabled for signup, but is also optional.

I had no problems with previous releases, but in the latest update (6.x-1.0-rc6) I get the following error for a new Course node with no date/time values set (ie, using Blank default values), regardless of whether signup is enabled/disabled for this node:

warning: date_timezone_set() expects parameter 1 to be DateTime, null given in .../sites/all/modules/signup/includes/date.inc on line 485.
warning: date_modify() expects parameter 1 to be DateTime, null given in .../sites/all/modules/signup/includes/date.inc on line 488.
warning: date_format() expects parameter 1 to be DateTime, null given in .../sites/all/modules/signup/includes/date.inc on line 489.

If a start date/time is entered, the error does not appear. This error only appears on initial create; subsequent edit/save operations do not generate this error. Time zone handling is set to Site's time zone.

dww’s picture

dww
we/he/they
commented

@mattyoung and @snorkers: Does the patch in #6 help?

snorkers’s picture

snorkers commented

Just rolled the patch at #6 against RC6 and the error is gone. Thanks @iva2k and @dww :)

Apologies - hadn't tried the patch as I thought this was an RC6-emergent issue only.

steveparks’s picture

steveparks commented

I too have tested the patch at #6 against rc6, and it makes these errors go away, without any apparent side effects.

OliverColeman’s picture

OliverColeman commented

#6 seems to work for me, thanks!

OliverColeman’s picture

OliverColeman commented

Well, after fixing this problem, I discovered why it was now popping up in the first place (after upgrading to rc6): Signup was preventing any CCK fields being saved for any new nodes. So for any one turning up here with the same problem, see the issue (with patch) over at #605594: node_load() during signup's node_save() breaks node data cached by CCK.

iva2k’s picture

iva2k commented
Status: Needs review » Reviewed & tested by the community

So 3 people tested patch in #6 already, have warnings gone and no side effects. Bumping to "reviewed & tested by the community".

karens’s picture

karens commented

I can also confirm this fix is needed and it works.

bibo’s picture

bibo commented

Works for me too! Commit this please.

Mac Clemmens’s picture

Mac Clemmens commented

Confirmed. Patch #6 resolved my issue. Agree with KarenS; ready to commit.

d.cox’s picture

d.cox commented
Issue tags: +multilingual

Confirm here also patch #6 solved my issue also.
My issue was slightly different as I have a multilingual site.
When a node is created, using i18n_auto to create the draft nodes in the other enabled languages, I would receive the error. Even though the date is past to the nodes with the CCK translation and Synchronize translations modules.

tomogden’s picture

tomogden commented

I wish I could say the same, but I have RC6 installed (Signup 6.x-1.0-rc6), and I still get the warnings, and my date field goes to NULL.

warning: date_timezone_set() expects parameter 1 to be DateTime, null given in /sites/all/modules/signup/includes/date.inc on line 485.
warning: date_modify() expects parameter 1 to be DateTime, null given in /sites/all/modules/signup/includes/date.inc on line 488.
warning: date_format() expects parameter 1 to be DateTime, null given in /sites/all/modules/signup/includes/date.inc on line 489.
warning: date_timezone_set() expects parameter 1 to be DateTime, null given in /sites/all/modules/signup/includes/date.inc on line 485.
warning: date_modify() expects parameter 1 to be DateTime, null given in /sites/all/modules/signup/includes/date.inc on line 488.
warning: date_format() expects parameter 1 to be DateTime, null given in /sites/all/modules/signup/includes/date.inc on line 489.

dww’s picture

dww
we/he/they
commented

@tomogden: rc6 doesn't contain the fix. Did you apply the patch in comment #6 above?

tomogden’s picture

tomogden commented

My misunderstanding, sorry. Confused between RC6 and Patch 6.

ezra-g’s picture

ezra-g commented
Status: Reviewed & tested by the community » Fixed

This is committed. Thanks!

http://drupal.org/cvs?commit=470414

ezra-g’s picture

ezra-g commented
Version: 6.x-1.0-rc6 » 6.x-2.x-dev
Status: Fixed » Patch (to be ported)