In #831470: Import fails due to dates not converted correctly, a strtotime() call was introduced in class FeedsDateTime, at line 500 of plugins/FeedsParser.inc. Unfortunately that breaks dates before 1901.

I don't fully understand which problem was solved by calling strtotime(), but it doesn't look right to me. FeedsDateTime extends the DateTime class, which is meant to remove the limitations of unix timestamps. We should be able to please "Some PHP 5.2 version" (as the inline documentation says) without using "strtotime".

I'm willing to help fix this but I'm not sure about the best approach. Thanks in advance.

CommentFileSizeAuthor
#16 feeds-987206-16.patch883 bytesgaele
#11 mappings_xml.png58.17 KBaudua
#11 settings_for_xml.png72.63 KBaudua
#11 xml_snapshot.png71.4 KBaudua
#5 987206-5_feeds-1901.patch700 bytesmarcvangend

Comments

marcvangend’s picture

marcvangend
he/him
Primary language Dutch
Location Amsterdam
commented

I managed to fix this bug by replacing strtotime() with date_create() in both mappers/date.inc and plugins/FeedsParser.inc.

While that works for me, I'm not 100% sure if this is the correct fix for every possible situation. I think we need to test this thoroughly so we don't break one thing while fixing another. One thing to learn from this, it that we need to be careful with code that is merely a workaround for PHP bugs.

kclarkson’s picture

kclarkson commented

I have looked everywhere regarding how to import date fields.

Could you help me out by listing the steps you took to get the date fields to import?

Thanks again,

marcvangend’s picture

marcvangend
he/him
Primary language Dutch
Location Amsterdam
commented

kclarkson: yes I can, but not here. This is a bug report, not a support forum. Please open a new forum topic or a support request issue for your question and send me a pm with the link.

marcvangend’s picture

marcvangend
he/him
Primary language Dutch
Location Amsterdam
commented
Status: Active » Needs review

Here is a patch; I replaced two occurrences of strtotime() with date_create(). In mappers/date.inc, I also needed to check using empty():

    if (empty($feed_element) || !is_numeric($feed_element) && !date_create($feed_element)) {
      return ;
    }

This is needed because strtotime() returns false when given an empty string, while date_create interprets an empty string as 'now' and returns a vaild datetime object.

marcvangend’s picture

marcvangend
he/him
Primary language Dutch
Location Amsterdam
commented
StatusFileSize
new 987206-5_feeds-1901.patch700 bytes

Oops - here is a patch.

johnv’s picture

johnv
Primary language Dutch
commented

I tested your patch together with issue #831470, but your patch didn't work for me.
My source contains '00.00.0000' for a default/empty date. I can imagine other sources have dummy values as well.
I propose the following for your change in date.inc, introducing a 'contains only separators' test. You could also (better?) test for the opposite: contains_any_of('123456789')-test. But I couldn't find the proper php-function :-( .

//if (!is_numeric($feed_element) && !strtotime($feed_element)) {
//if (empty($feed_element) || !is_numeric($feed_element) && !date_create($feed_element)) {
if ( !( strcspn($feed_element , ' 0-.,:/') && date_create($feed_element) ) ) {
  return ;
}
marcvangend’s picture

marcvangend
he/him
Primary language Dutch
Location Amsterdam
commented

@johnv: AFAIK, the current code handles empty values perfectly, they just need to be empty. IMHO your code is fixing the problem in the wrong place. If your source contains dummy values, you can set them to an empty string in a custom parser, or you should try to change the source itself.

audua’s picture

audua commented

marcvangend, can you please direct me to where you solve Kclarkson's problem from above. I could get text field to import but not date field. Thanks in advance.

This is how my date xml looks
Thu, 02/25/2010 - 7:30pm
Thu, 02/25/2010 - 7:30pm

Status: Needs review » Needs work

The last submitted patch, 987206-5_feeds-1901.patch, failed testing.

marcvangend’s picture

marcvangend
he/him
Primary language Dutch
Location Amsterdam
commented

audua, I can't remember getting a pm from kclarkson, I never helped him solve his problem.

audua’s picture

audua commented
StatusFileSize
new xml_snapshot.png71.4 KB
new settings_for_xml.png72.63 KB
new mappings_xml.png58.17 KB

marcvangend, can you please help me with this then? I have an xml that I create with the views_datasource and the views_bonus modules. The xml is attached. I tried to map cck fields called title, location, body and date. But only title, location and body get mapped. CCK Date field is never mapped. I attached the mapping snapshot. I have also attached the settings for xpath xml parser. I don't know if it is a date format issue or I'm missing something. I even applied (manually) the patch from http://drupal.org/node/831470 from #5. Thanks in advance.

johnv’s picture

johnv
Primary language Dutch
commented

@audua, I don't think your request anything to do with the initial issue and patch. Can you open a new 'support request'-issue for your problem, so we keep this thread dedicated to one subject? Thanx.

kclarkson’s picture

kclarkson commented

To all,

Just wanted to give everyone a heads up that I got the date fields to import.

I am using excel to enter text from an old static HTML page.

For the date fields you need to use the custom format cells BEFORE to converting it to a csv file.

Taxoman’s picture

Taxoman commented

Subscribing

franz’s picture

franz
any
Location Montréal
commented

Is this patch still broken?

gaele’s picture

gaele commented
Status: Needs work » Needs review
StatusFileSize
new feeds-987206-16.patch883 bytes

Recreated patch. I have no idea why the patch from #5 failed.

gaele’s picture

gaele commented

BTW I'm still looking for a solution for this for 7.x.

franz’s picture

franz
any
Location Montréal
commented

Can we avoid dealing with empty values for now? We should soon deal with it properly on #1107522: Framework for expected behavior when importing empty/blank values + text field fix.

Just replacing strtotime() looks great for now.

franz’s picture

franz
any
Location Montréal
commented
Status: Needs review » Needs work
twistor’s picture

twistor commented

@franz, we can't avoid dealing with it at the moment. It just needs to follow current behavior, which is that empty values don't get set, unless it makes sense, like 0 for a number field.

twistor’s picture

twistor commented
Status: Needs work » Needs review
twistor’s picture

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