Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
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.
Comment | File | Size | Author |
---|---|---|---|
#71 | handle_empty_and_invalid_dates-857216-71.patch | 2.92 KB | gnucifer |
#62 | handle_empty_and_invalid_dates-857216-62.patch | 2.91 KB | gnucifer |
#51 | handle_empty_date-857216-51.patch | 622 bytes | wuinfo - Bill Wu |
#38 | re.patch | 786 bytes | wuinfo - Bill Wu |
#27 | feed_empty_date_27.patch | 741 bytes | yareckon |
Comments
Comment #1
Hanno CreditAttribution: Hanno commentedTo clarify, my item has dates as follows:
I tested and the start date also falls back to the present date if no date is given.
Comment #2
alex_b CreditAttribution: alex_b commentedThanks 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.
Comment #3
alex_b CreditAttribution: alex_b commentedActually, this issue is a duplicate of #705588: CCK field default value's not getting picked up - right?
Comment #4
Hanno CreditAttribution: Hanno commentedIt 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.
Comment #5
Hanno CreditAttribution: Hanno commentedI 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.
Comment #6
chrsnlsn CreditAttribution: chrsnlsn commentedAnyone have any other insight on this? I would really like to be able to use my default from cck for this.
Comment #7
alex_b CreditAttribution: alex_b commentedI 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).
Comment #8
derhasi CreditAttribution: derhasi commentedAs 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
So any invalid date is set to 'now'.
Both solutions do not alter
FeedsDateTime
, they hop in atdate_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.
Comment #9
alex_b CreditAttribution: alex_b commentedWhat 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?
Comment #10
derhasi CreditAttribution: derhasi commented@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.
Comment #11
alex_b CreditAttribution: alex_b commentedOk, thanks. Will commit this in my next round of Feeds work.
Comment #12
alex_b CreditAttribution: alex_b commentedRunning tests right now.
Comment #13
alex_b CreditAttribution: alex_b commentedCommitted, thank you:
http://drupal.org/cvs?commit=443264
Comment #14
alex_b CreditAttribution: alex_b commentedComment #16
marcvangendThe 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.Comment #17
johnvPlease 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)
Comment #18
johnvThe current patch does not remove an existing date. I propose the following change, which copies an empty value:
Comment #20
retolist CreditAttribution: retolist commentedSubscribe
Comment #21
muschpusch CreditAttribution: muschpusch commented#18 works ! Please commit to the d7 branch...
Comment #22
marcvangendPlease 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.
Comment #23
johnv@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.
Comment #24
marcvangend@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.
Comment #25
zenlan CreditAttribution: zenlan commented#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.
Comment #26
Taxoman CreditAttribution: Taxoman commentedDoes 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?
Comment #27
yareckon CreditAttribution: yareckon commentedRolled #18 into a git patch format.
Taxoman, no it changes the behavior, it doesn't allow switching between behaviors at all.
Comment #28
czigor CreditAttribution: czigor commentedWith #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.
Comment #29
zoraxits' ok form me with #27 on 7.2 dev version.
thanks.
Comment #30
franzOk, 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.
Comment #31
mstef CreditAttribution: mstef commented#27 looks good
Comment #32
wuinfo - Bill Wu CreditAttribution: wuinfo - Bill Wu commentedCommit 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.
Comment #33
franzThe 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.
Comment #34
franzSome related issue (for blank values): #1107522: Framework for expected behavior when importing empty/blank values + text field fix
Comment #35
Shiraz Dindarsuccess for me with #27
Comment #36
wuinfo - Bill Wu CreditAttribution: wuinfo - Bill Wu commented#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?
Comment #37
franzSee #30 and related issue linked. Using strtotime() is not recommend for checking if a date is valid (although it works most of the time).
Comment #38
wuinfo - Bill Wu CreditAttribution: wuinfo - Bill Wu commentedThanks franz. How about this one? I have taken out the strtotime().
Comment #39
franzComment #40
franzThanks, 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. =)
Comment #41
franzComment #42
busla CreditAttribution: busla commentedI´m having the same problem as #25 with date columns in CSV that only contain YYYY. Does the patch cover that also?
Comment #43
busla CreditAttribution: busla commentedOk, updated to 2.x-dev and see that there is still an issue with importing date columns in csv that only contain YYYY.
Comment #44
twistor CreditAttribution: twistor commentedThis 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.
Comment #45
akolahi CreditAttribution: akolahi commented#38 worked for me.
Comment #46
hey_germano#38 worked for me, too. Thanks!
Comment #47
Peacog CreditAttribution: Peacog commentedAnother thumbs-up for #38. Thanks.
Comment #48
bartmann CreditAttribution: bartmann commented#38 works here also - thanks!
Comment #49
everpat CreditAttribution: everpat commented#38 worked for me also. Thanks.
Comment #50
wuinfo - Bill Wu CreditAttribution: wuinfo - Bill Wu commentedChange the status, pending for commit.
Comment #51
wuinfo - Bill Wu CreditAttribution: wuinfo - Bill Wu commentedUpdated the patch, so it can now be applied to 7x-2.x again.
Comment #52
SilviaT CreditAttribution: SilviaT commented#51 works for me
Any plans to commit this? It would be very useful, many people subscribed for this issue.
Comment #53
greenwork CreditAttribution: greenwork commentedWas this committed?
Comment #54
greenwork CreditAttribution: greenwork commentedI 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
Comment #55
4kant CreditAttribution: 4kant commented#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.
Comment #56
franz4kant, 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.
Comment #57
plopescPatch in #51 worked for me.
Thanks
Comment #58
wuinfo - Bill Wu CreditAttribution: wuinfo - Bill Wu commented@4kant: Your requirement is to erase the "Monday" data when the field is empty in CSV?
Comment #59
presleyd CreditAttribution: presleyd commentedThis 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.
Comment #60
gnucifer CreditAttribution: gnucifer commentedI 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.
Comment #61
gnucifer CreditAttribution: gnucifer commentedAnother 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.
Comment #62
gnucifer CreditAttribution: gnucifer commentedThis 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.
Comment #64
gnucifer CreditAttribution: gnucifer commentedThe test fail as a consequence of me removing:
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'.
Comment #65
wuinfo - Bill Wu CreditAttribution: wuinfo - Bill Wu commentedAccording 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.
Comment #66
gnucifer CreditAttribution: gnucifer commentedThat'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.
Comment #67
wuinfo - Bill Wu CreditAttribution: wuinfo - Bill Wu commentedManually 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 .
Comment #68
wuinfo - Bill Wu CreditAttribution: wuinfo - Bill Wu commented@presleyd at #59,
Text did not wiping out existing data
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
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.
Comment #69
wuinfo - Bill Wu CreditAttribution: wuinfo - Bill Wu commentedComment #70
gnucifer CreditAttribution: gnucifer commentedPerhaps I'm mistaken, but wouldn't the patch in #51 set the date to 'now' on empty or invalid values?
Comment #71
gnucifer CreditAttribution: gnucifer commentedForgot to bail out on the exception, here's a new patch that I think will solves the issue in #67.
Comment #73
gnucifer CreditAttribution: gnucifer commentedI 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.
Comment #74
franzOk, so just to clarify, this is RTBC for patch in #51, right?
Comment #75
franzCommitted. Thanks.
Comment #77
donSchoe CreditAttribution: donSchoe commentedSorry to reopen this, but for me this is not fixed.
This patch in #51 only addresses the
date:start
field but not thedate:end
subfield: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:
Is it just me?
Comment #78
MegaChriz CreditAttribution: MegaChriz commentedComment #81
MegaChriz CreditAttribution: MegaChriz as a volunteer commentedThis should be fixed as part of #1107522: Framework for expected behavior when importing empty/blank values + text field fix.