Default timezone setting mismatch in metadata wrapper integration

This issue happens due to default timezone configuration in metadata wrapper.
Provided patch fixed this issue.

How to test:

1. Attach "Date" field to some entity
2. Run script similar to this:
   

   
   
   $entity = entity_create($entity_type, ['type' => $bundle]);
   
   $original = '1482721671';
   var_dump($original);
   
   $entity->wrapper()->{$field_name} = $original;
   $fetch = $entity->wrapper()->{$field_name}->value();
   var_dump($fetch);
   
   if ($original != $fetch) {
     print "WTF!!! Dates mismatch!\n";
   }
   else {
     print "Dates match!\n";
   }
   

Results without patch:

string(10) "1482721671"
string(10) "1482768471"
WTF!!! Dates mismatch!

Results with patch:

string(10) "1482721671"
string(10) "1482721671"
Dates match!

Would love to provide actual test coverage but need some guidance for it.

Setting proper title.

Thanks RoSk0, that patch works for me.

Thanks @Juterpillar!
Moving to needs work because we need to create a test for this case.

I'm not entirely sure what's happening here.

I've added the tests to check the 3 date types (date, datetime & datestamp) and the tests are setting and getting the value from each field.

However... The tests pass.

I even rolled back to 7.x-2.10-rc1 which was current when date-2123039-1.patch was added. Tests still pass there too.

I may be doing something wrong here, but I'm pretty sure these are correct. Given how long this has been sitting here there's a chance that adding this patch may allow someone to tweak it.

Ah nuts. Square bracket array declaration didn't kick in until 5.4. Rerolling...

Fixed the php 5.3 issues with the test-only patch.

I really need to set up the local test env to work with php 5.3 too.

Caught the remaining square bracket array declarations.

Okay... that was embarrassing. Turns out it was copy & paste example code that threw the 'edit' module error.

Fixed that, updated the test name and tidied up the test output a little.

So... That worked. :/ It was supposed to throw errors that the application of this patch would fix.

@RoSk0, can you take a look and see why this is passing when it should be failing?

I bumped into this issue. Thank you for the useful patches.

I tried the patch in #12 and it surely succeeds as unpexpected... I believe that we need to replicate the condition of the timezone handling in the test:

If I create a date field, using the 'date' field type (ISO date format), and set its lowest granularity to 'day', then the field has no timezone handling, and no timezone is stored.

I tried tweaking the patch. I'd like to see how the testbot says.

Hmm. The test-only patch in #14 fails as expected in my environment though...

I can confirm that I see the same errors you see in the image.

When I first ran it though, the Date Entity Metadata Wrappers test was not available. I had to manually enable Entity to get these to appear in the list.

Looking at the output from the test-only patch (click "Show: All") I don't see Date Entity Metadata Wrappers there either. While the test describes entity as being a dependency it appears to be used as a check to see if the test should be available rather than an indicator that it should add the module and enable it for the test.

I'm going to do a bit of digging to see how simpletest treats test_dependencies. It claims to handle it but I'm wondering if we're using it right.

Hmm... A check of the simpletest module returns zero hits for test_dependencies. Many for $info['dependencies'] though.

Testing that...

I still consider this to be Needs Work, but Needs Review will trigger the testing.

Well that crashed and burned harder than expected.

Let's ignore the patch at #17.

The dig into simpletest continues...

Okay, lets see if it's the format of the of the test_dependencies[] entry.

Nuts... That failed too.

If I understand it right we should see Date.DateFieldEntity in the list of tests. Check here for the test run and click All in the "Show: Failing classes All" line to see all the tests. The test does not appear. Can anyone see why?

You need to clear the site's caches in order to have a new test file recognized.

Some minor tweaks to the test file.

The tests fail locally:

Fail      Other      date_entity_metad  116 DateFieldEntityTestCase->testCheckE
    Date (ISO format) plays nicely with Entity Metadata Wrappers. 1482721671 ==
    1482685671
Fail      Other      date_entity_metad  116 DateFieldEntityTestCase->testCheckE
    Date plays nicely with Entity Metadata Wrappers. 1482721671 == 1482685671

Ah! The tests don't run because the new dependency needs to be added to the module first. So lets do that first.

Ok, now that the test dependency is committed we can now use it in the tests.

Ok, so now we're getting somewhere - the tests fail as follows:

• fail: [Other] Line 116 of sites/all/modules/date/tests/date_entity_metadata_wrappers.test:
  Date (ISO format) plays nicely with Entity Metadata Wrappers. 1482721671 == 1482685671
• fail: [Other] Line 116 of sites/all/modules/date/tests/date_entity_metadata_wrappers.test:
  Date plays nicely with Entity Metadata Wrappers. 1482721671 == 1482685671

Excellent. Thanks Damien.

So the Drupal test system checks out module, meets any dependencies and then patches. This feels like a bug report for that. :)

I'll recheck this with RoSk0s original patch now.

Looks good.

I've rerolled RoSk0s patch with the tests.

The submission on this form really needs a check for "are you sure your Status is correct?" if a new file is added and the status doesn't change. :)

Awesome work Gold, RoSk0s and hgoto! Test coverage to show the problem, and then a fix for the problem.

Gold, Damien, Great! Thanks!! I didn't know that we need to add the new test dependency in advance.

I didn't add the assertion to confirm that the timezone setting is saved properly in the patch #14. Please let me add one line for that.

I didn't know that we need to add the new test dependency in advance.

That's what caught me out too. That was doing my head in. :)

New test looks sensible, patch still applies, tests pass.

I vote RTBC.

Is there any possibility that $info['field']['settings']['tz_handling'] wouldn't exist?

I'm just adding a Date field now and the "Time zone handling" field has no empty values in the options. There is a value of 'none', but that should be stored.

Confidence is high that $info['field']['settings']['tz_handling'] would always exist if added from the UI.

Someone building a form in code may miss it though. But, IMHO, that would be a bug in their code.

This still feels RTBC to me.

Committed! Thanks everyone for helping to fix this!

Great!! Thanks!

Regarding #35/#36 I just ran into an issue where $info['field'] didn't exist, when loading from a token that was coming from a rule.

-  $timezone_db = !empty($item['timezone_db']) ? $item['timezone_db'] : 'UTC';
+  $timezone_db = date_get_timezone_db($info['field']['settings']['tz_handling']);

I think this may need a test for that case.

Also the function docblock mentions it returns UTC.

Getter callback to return date values as datestamp in UTC.

I think this patch needs a revert unfortunately. Nice that it has tests, just needs a bit more tests I guess or another approach.

Reverted. Thanks for catching that, Joel.

@joelpittet, thanks. I tried to reproduce the case where $info['field'] does not exist with a rules config but couldn't. Can you please share a rules config I can reproduce the case with?

@hgoto, sorry for the delay. I've had this commit fail on two different unrelated sites, so there may be more cases where this is incorrect. But it's not the rule I think it's the token replacement where things break so I should be able to create a quick script to show that failure.

Please consider #42 indicates the output should be UTC I wonder why it even looks at the $item['timezone_db'] in the first place.

So I think this needs more tests.

In regards to #42: Dates stored in DB in site default timezone - this is the reason why original code was looking for $item['timezone_db'] , but that doesn't change the fact that returned date is in UTC.

I added test for token replacement to confirm issue(actually there is not issues here and all works as expected).

Also, I wasn't able to reproduce issue with Rules. I have no test for it, but attaching with the rules config I used to test. I think whatever issues @joelpittet had with new code may be not in scope for this module/issue.

Looks good for me:

• Resolves the issues for me ✓
• Provides test cases ✓
• Using the same method to determine the timezone in the setter and getter seems cleaner. ✓

Rerolled, and I renamed the new test.

Committed. Thanks everyone.