I have a feed with event items with a start date and end date. When the end date is not in the source (i.e. no tag included in the feed), the date mapper, however, fills the end date with the date of importing.
The date mapper should instead leave the fiel blank or better, use the default value for the end date (configured in the field setting of the date field, in my case as 'blank' for the end date) if the datefield is not present.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Hanno’s picture

To clarify, my item has dates as follows:

<item date="2010-07-01 14:00:00" subject="Event without enddate"/> - enddate becomes present date
<item date="2010-07-01 14:00:00" enddate="2010-07-01 15:00:00" subject="item with enddate"/>

I tested and the start date also falls back to the present date if no date is given.

alex_b’s picture

Title: date default value when no date is present » Date defaults to import date when no date is present
Version: 6.x-1.0-beta2 » 6.x-1.x-dev

Thanks for the report, this behavior is clearly not ideal. I would accept a patch to fall back to field default values. OTOH I can't tell what level of effort this will take.

alex_b’s picture

Status: Active » Closed (duplicate)

Actually, this issue is a duplicate of #705588: CCK field default value's not getting picked up - right?

Hanno’s picture

It is related to the same issue. But, the fact that default values is not implemented, and no date is detected, the date field should be empty and not now().
If default values is implemented, I am not sure if it works for the date field, because default values only works if the field is empty.

Hanno’s picture

Title: Date defaults to import date when no date is present » Date is set to now when no date is present
Status: Closed (duplicate) » Active

I did some research and this issue is a bug specific for date fields. In the function buildDateField in FeedsParser.inc dates are filled with now when no dates are found. This is the case for a date with a 'from' and an optional 'to' field, as well as a date field with only a 'from' field.

scenario
1. If I have a test file like: <item date="2010-07-01 14:00:00" subject="Event without end"/> (no enddate)
2. with mapping date->from:activity and enddate->to:activity
3. $use_end get filled however, in function buildDateField for to:activity
4. the if statement in line 332 if ($use_end) fills $node->[field_activity][0]['value2']

If I disable the if routine in line 332, the date of the node doesn't set to now.

I think this issue is caused by the date handling in FeedsParser.inc due to timezones and date granularity. I can't figure out what exactly is going on there. I suppose this needs to be solved first before we can implement default values for cck dates.

chrsnlsn’s picture

Anyone have any other insight on this? I would really like to be able to use my default from cck for this.

alex_b’s picture

I have no time to look into this closer, but next steps are:

1. Fix #705588: CCK field default value's not getting picked up.
2. Make sure that date elements are not populated if no date is present on source (along the lines of what Hanno found and described in #5).

derhasi’s picture

Status: Active » Needs review
Issue tags: +datetime, +CCK date
FileSize
379 bytes
343 bytes

As I think the main problem of this issue is independent of #705588: CCK field default value's not getting picked up, I attached two solutions for this issue.

The main reason why feeds sets empty dates to 'now' is located in FeedsParser.inc on line 499

// Some PHP 5.2 version's DateTime class chokes on invalid dates.
    if (!strtotime($time)) {
      $time = 'now';
    }

So any invalid date is set to 'now'.

Both solutions do not alter FeedsDateTime, they hop in at date_feeds_set_target(), before FeedsDateTime-objects are created:

The first variant assumes that there should not be set any date or time, when the feed_element is "".

Variant 2 checks if the feed_element could be a date by checking on numeric and strtotime. If both fails, it skips setting any date.

alex_b’s picture

What is the overall effect of these patches on the date field? Is it just going to be empty? I assume it does not fall back to its default value, does it?

derhasi’s picture

@alex_b, you are right. It just get's empty, it does not fall back to the default value. But this is only for CCK date fields. The core "published date" falls back to "now".

But this should not be a problem for this special issue, as the expected behaviour for importing an empty date is setting no date at all.
I'd prefer patch variant 2, as there also invalid dates are set to "no date", but a date like "now" or "now +1day" is still valid.

For the cck default value issue, there is still #705588: CCK field default value's not getting picked up to solve, but as I said, this should not be part of this issue.

alex_b’s picture

Status: Needs review » Reviewed & tested by the community

Ok, thanks. Will commit this in my next round of Feeds work.

alex_b’s picture

Issue tags: -datetime, -CCK date
FileSize
801 bytes

Running tests right now.

alex_b’s picture

Title: Date is set to now when no date is present » Set Date to empty if no date is present
alex_b’s picture

Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

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

marcvangend’s picture

The use of strtotime() in this patch is a problem when trying to process dates before 1901. Please see #987206: strtotime() in class FeedsDateTime breaks dates before 1901 for a fix.

johnv’s picture

Version: 6.x-1.x-dev » 7.x-2.x-dev
Status: Closed (fixed) » Patch (to be ported)

Please apply this patch in 7.x, too. This patch is applied in 6.x, but not in the 7.x branch. I tested it, and it resolved my problem in D7.

BTW, the patch as in Comment #16 didn't work for me. My source contains '00.00.0000' for a default/empty date. (I'll post it in the issue,itself, too)

johnv’s picture

Status: Patch (to be ported) » Needs work

The current patch does not remove an existing date. I propose the following change, which copies an empty value:

     if (is_array($feed_element)) {
       $feed_element = $feed_element[0];
     }
+
+    // Empty string means no time - so clear the date field!
+    if (!is_numeric($feed_element) && !strtotime($feed_element)) {
+       $feed_element = new FeedsDateTimeElement(NULL, NULL);
+//      return ;
+    }
+
-     if ($sub_field == 'end') {
     elseif ($sub_field == 'end') {
       $feed_element = new FeedsDateTimeElement(NULL, $feed_element);
     }
retolist’s picture

Subscribe

muschpusch’s picture

Status: Needs work » Reviewed & tested by the community

#18 works ! Please commit to the d7 branch...

marcvangend’s picture

Status: Reviewed & tested by the community » Needs work

Please do not commit #18. It uses the strtotime() function which unnecessarily limits the capabilities of PHP date handling.

The strtotime function is meant to convert date strings to valid unix timestamps; it is not a way to validate a date string. As the PHP documentation says: "The valid range of a timestamp is typically from Fri, 13 Dec 1901 20:45:54 UTC to Tue, 19 Jan 2038 03:14:07 UTC." This means that importing dates outside this range will cause strtotime() to return false, even when the string is a perfectly valid date that Drupal is able to handle. i have already proposed a better solution to validate the date string in #987206: strtotime() in class FeedsDateTime breaks dates before 1901.

johnv’s picture

@marcvangend, with all respect, both issues handle different problems. For a lot of people 1900+ dates are satisfying, and this issues fixes a problem. Handling historical dates is also a problem, which is treated in your other issue. They both conflict, I know. But you shouldn't prohibit improvements for other people. In the end, it is up to maintainer to solved conflicts of interest.

marcvangend’s picture

@johnv: there is no conflict here. I'm not saying your problem shouldn't be fixed, I'm only saying that it should be fixed correctly. Date strings can be validated without using strtotime. Everybody wins.

zenlan’s picture

#18 not working for me (but thanks for the clue anyway!).

I needed a very quick solution and the following works for me although I dare say not for everyone or in every situation:

function date_feeds_set_target($source, $entity, $target, $feed_element) {
list($field_name, $sub_field) = explode(':', $target, 2);
if (!($feed_element instanceof FeedsDateTimeElement)) {
if (is_array($feed_element)) {
$feed_element = $feed_element[0];
}
if (empty($feed_element)) {
$entity->$field_name = '';
return;
}
else if ($sub_field == 'end') {
$feed_element = new FeedsDateTimeElement(NULL, $feed_element);
}
else {
$feed_element = new FeedsDateTimeElement($feed_element, NULL);
}
}
$feed_element->buildDateField($entity, $field_name);
}

A second issue I discovered was with a date field that has only a year in it. FeedsDateTime doesn't seem to recognise 'YYYY' as a valid date format and builds a default object instead, changing all my year date fields to 1970. Needing to fix this quick I have implemented a workaround in my export to rewrite the year value as "YYYY-01" which gets imported and displayed correctly.

Apologies that this comment a) does not offer a great solution and b) goes off-topic to broach another issue altogether but these workarounds might help someone in the interim.

Taxoman’s picture

Does this patch "support" choosing either a fixed date or now() as the default value for empty date fields, on a per-field/content-type basis?

yareckon’s picture

FileSize
741 bytes

Rolled #18 into a git patch format.

Taxoman, no it changes the behavior, it doesn't allow switching between behaviors at all.

czigor’s picture

With #27 I can import empty date fields. The field is empty even if its default value is now(), which is the desired behavior in my case.

zorax’s picture

its' ok form me with #27 on 7.2 dev version.
thanks.

franz’s picture

Ok, so one can use the approach in http://drupal.org/node/987206#comment-3970744 instead of strtotime() and then this patch is good to go?

Maybe we should create a method validate_date() or something like that and use it both places. Too bad idea?

Other than that, the patch worked for me.

mstef’s picture

#27 looks good

wuinfo - Bill Wu’s picture

Version: 7.x-2.x-dev » 7.x-2.0-alpha4
Component: Code » Feeds Import
Assigned: Unassigned » wuinfo - Bill Wu
Status: Needs work » Reviewed & tested by the community

Commit to the next release please. Sometime, it is not wanted to convert empty string to now().

I have a customer who want to import Expense Disclosure data from csv to Drupal node with Feeds module 7.x-2.0-alpha4.

Travel start date and travel end date was imported as Date format.

But, sometime, there is expense without travel start and end date. The fields were left empty in that case. But every time we import such data, date was set to current date. It is not what we wanted at all.

Commit the patch at #27 can make sure those empty string or date with wrong format was set to null and it is more predictable and needed.

franz’s picture

Version: 7.x-2.0-alpha4 » 7.x-2.x-dev
Component: Feeds Import » Code
Assigned: wuinfo - Bill Wu » Unassigned
Status: Reviewed & tested by the community » Needs work

The patch needs work before it's committed, so just saying "please commit" doesn't help it get there. Do not change versions and components, and only assign this to yourself if you're doing some work on it.

franz’s picture

Shiraz Dindar’s picture

success for me with #27

wuinfo - Bill Wu’s picture

#27 patch is working. I have applied this patch to my project http://www.ohrc.on.ca . Until this piece code was committed, I have to apply this patch again whenever there is new version of feeds module.

Again, this patch is working. It may not solve all the outstanding default value issue. It solved the date data type.

franz, would you please let me know what work is needed?

franz’s picture

See #30 and related issue linked. Using strtotime() is not recommend for checking if a date is valid (although it works most of the time).

wuinfo - Bill Wu’s picture

FileSize
786 bytes

Thanks franz. How about this one? I have taken out the strtotime().

franz’s picture

Status: Needs work » Needs review
franz’s picture

Status: Needs review » Needs work

Thanks, wuinfo, but I forgot we shouldn't change the behavior, but rather provide options for it following #1107522: Framework for expected behavior when importing empty/blank values + text field fix. Now I see each mapper will have it's own logic for this, but nevertheless, we should get #860748: Config for mappers? finished to proper fix this issue. Just need to fix the tests there, if someone can work on it. =)

franz’s picture

Title: Set Date to empty if no date is present » Behavior on importing empty/NULL/invalid dates
busla’s picture

I´m having the same problem as #25 with date columns in CSV that only contain YYYY. Does the patch cover that also?

busla’s picture

Ok, updated to 2.x-dev and see that there is still an issue with importing date columns in csv that only contain YYYY.

twistor’s picture

Assigned: Unassigned » twistor

This is a very straight forward problem. Feeds is not validating the date correctly. Issues of setting defaults and handling empty behavior are outside the scope of this issue. Changing behavior is fine in this case, because the current behavior is a bug.

#38 is close, we need more tests.

akolahi’s picture

#38 worked for me.

hey_germano’s picture

#38 worked for me, too. Thanks!

Peacog’s picture

Another thumbs-up for #38. Thanks.

bartmann’s picture

#38 works here also - thanks!

everpat’s picture

#38 worked for me also. Thanks.

wuinfo - Bill Wu’s picture

Assigned: twistor » Unassigned
Status: Needs work » Reviewed & tested by the community

Change the status, pending for commit.

wuinfo - Bill Wu’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
622 bytes

Updated the patch, so it can now be applied to 7x-2.x again.

SilviaT’s picture

#51 works for me
Any plans to commit this? It would be very useful, many people subscribed for this issue.

greenwork’s picture

Was this committed?

greenwork’s picture

Priority: Normal » Major

I answered my own question. This was not committed. This actually kills feeds for me and is propagating current dates all over my posts.

I would like to say #51 worked for me perfectly

4kant’s picture

#51 worked for importing new nodes.

But:
In my case I had to update nodes that have date-fields filled with data

Eg. a museum with 7 date-fields for every weekday presenting it´s opening time on each day - in one case the muesum is no longer open on Mondays, so there should be no data in this field "Monday" any more.

When I update nodes with a blank field "Monday" the data still remains there.

franz’s picture

4kant, I think this might fall on my early comment #40. On the patch itself, can we get dummy sources posted here so we can test? A simple CSV with a couple of test cases would do.

plopesc’s picture

Patch in #51 worked for me.

Thanks

wuinfo - Bill Wu’s picture

@4kant: Your requirement is to erase the "Monday" data when the field is empty in CSV?

presleyd’s picture

This small patch works fine for what I consider sensible defaults. If there is nothing in the field I would expect it to be empty and not today's date. If you want today's date Feeds Tamper should be involved.

As for wiping out existing data, does Feeds do this currently with text fields or other field types? If so, then certainly date should behave the same.

gnucifer’s picture

As for wiping out existing data, does Feeds do this currently with text fields or other field types? If so, then certainly date should behave the same.

I can't say if it currently does, but it really should in both cases. I have battled lot's of issues with customer-solution because of feeds inconsistent behavior when it comes to default values for empty sources and purging of existing values. It's frustrating this is still an issue (after almost 3 years!) since it's really not a very complex problem to solve.

gnucifer’s picture

Another inconsistency is the handling of multiple values. Currently the date-mapper only handles single-value date imports. This differs from text that handles multiple (as all mappers should). There is really no technical reason why it should be so (I wrote code to fix this in feedapi for drupal 6), so it puzzles me why.

gnucifer’s picture

This patch needs more testing (did not have time to write tests), but I think it might be a petty good approach of solving the problem with empty or invalid dates.

Status: Needs review » Needs work

The last submitted patch, handle_empty_and_invalid_dates-857216-62.patch, failed testing.

gnucifer’s picture

The test fail as a consequence of me removing:

-    // Some PHP 5.2 version's DateTime class chokes on invalid dates.
-    if (!strtotime($time)) {
-      $time = 'now';
-    }

I think this is bad practice though. Sweeping errors under the carpet in the constructor is to fundamentally alter the behavior of the extended class (DateTime), plus strtotime have a limited range(?) and dates before 1901 will be set to 'now'.

wuinfo - Bill Wu’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/plugins/FeedsParser.incundefined
@@ -602,11 +603,6 @@ class FeedsDateTime extends DateTime {
-    // Some PHP 5.2 version's DateTime class chokes on invalid dates.
-    if (!strtotime($time)) {
-      $time = 'now';
-    }
-

According to http://www.php.net/manual/en/datetime.construct.php, it is default behavior if there is nothing pass to the constructor of DateTime.

Agree: It is not the best way to prevent invalid date time string to the constructor, but it is a necessary step to avoid errors during import. And it is not the good place to fix current issue.

gnucifer’s picture

That's beside the point. If you pass NULL or 'now' to PHP's DateTime you get the current date, nothing wrong with that. If you pass an invalid date (like for expample '2010-20-12' in the test) you get an exception.

If you pass '2010-20-12' to FeedsDateTime you don't, you get an object for the current date, which is fundamentally changing the behavior of DateTime (in a bad way). As mentioned in my comment above you will also be unable use pre-1901 dates because strtotime is used.

wuinfo - Bill Wu’s picture

Manually tested patch at #62

After apply the patch:

An AJAX HTTP error occurred. HTTP Result Code: 200 Debugging information follows. Path: /batch?id=5472&op=do StatusText: OK ResponseText: Fatal error: Call to a member function buildDateField() on a non-object in .

wuinfo - Bill Wu’s picture

Status: Reviewed & tested by the community » Needs work

@presleyd at #59,

As for wiping out existing data, does Feeds do this currently with text fields or other field types? If so, then certainly date should behave the same.

Text did not wiping out existing data

Eg. a museum with 7 date-fields for every weekday presenting it´s opening time on each day - in one case the muesum is no longer open on Mondays, so there should be no data in this field "Monday" any more.

Feeds do this currently with text fields, text field is not wiping out the existing data if it is empty. Check out this issue for your request: #1107522: Framework for expected behavior when importing empty/blank values + text field fix .

@franz

4kant, I think this might fall on my early comment #40. On the patch itself, can we get dummy sources posted here so we can test? A simple CSV with a couple of test cases would do.

Patch at #51 did not change the behavior for empty date field. Actually, it fixes it. This issue is slightly different from the issue you have listed in #40 #1107522: Framework for expected behavior when importing empty/blank values + text field fix .

I think we are safe to commit patch at #51 and close this issue.

wuinfo - Bill Wu’s picture

Status: Needs work » Reviewed & tested by the community
gnucifer’s picture

Perhaps I'm mistaken, but wouldn't the patch in #51 set the date to 'now' on empty or invalid values?

gnucifer’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
2.92 KB

Forgot to bail out on the exception, here's a new patch that I think will solves the issue in #67.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, handle_empty_and_invalid_dates-857216-71.patch, failed testing.

gnucifer’s picture

Status: Needs work » Reviewed & tested by the community

I reviewed the code again and the approach in #51 seems sound. From what I can tell will not set any unexpected 'now'-values (as I first thought). I might still prefer my patch though since it gets rid of the strtotime-usage and only parses the date once. For me in #51 is also pretty hard to follow. It's not super-trivial to figure out what BuildDateField will perform on "new FeedsDateTimeElement(NULL, NULL)", and some change in any of the methods involved in the call could later on introduce new bugs or unexpected behaviors. #51 is probably safer to apply right now, since it doesn't change the way FeedsDateTime works so less potential for side-effects, short term at least.

franz’s picture

Ok, so just to clarify, this is RTBC for patch in #51, right?

franz’s picture

Status: Needs work » Fixed

Committed. Thanks.

Status: Fixed » Closed (fixed)

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

donSchoe’s picture

Issue summary: View changes
Status: Closed (fixed) » Needs work

Sorry to reopen this, but for me this is not fixed.

This patch in #51 only addresses the date:start field but not the date:end subfield:

diff --git a/mappers/date.inc b/mappers/date.inc
index 3de6a69..a96143a 100644
--- a/mappers/date.inc
+++ b/mappers/date.inc
@@ -51,7 +51,10 @@ function date_feeds_set_target($source, $entity, $target, $feed_element) {
     if (is_array($feed_element)) {
       $feed_element = $feed_element[0];
     }
-    if ($sub_field == 'end') {
+    if (empty($feed_element) || !is_numeric($feed_element) && !date_create($feed_element)) {
+      $feed_element = new FeedsDateTimeElement(NULL, NULL);
+    }
+    elseif ($sub_field == 'end') {
       $feed_element = new FeedsDateTimeElement(NULL, $feed_element);
     }
     else {

I came across this issue because it is not working as desired for me. If an :END field is empty/NULL/invalid it gets set to the :START value. Is this the desired behaviour?

I wish that empty subfields also stay empty:

-      $feed_element = new FeedsDateTimeElement(NULL, $feed_element);
+      $feed_element = new FeedsDateTimeElement(NULL, NULL);

Is it just me?

MegaChriz’s picture

rowbay queued 27: feed_empty_date_27.patch for re-testing.

The last submitted patch, 27: feed_empty_date_27.patch, failed testing.

MegaChriz’s picture

Status: Needs work » Closed (fixed)