Default value for Date-Only fields is broken when UTC date is different than user current date

Patch at #3 works for me too, tested manually with different time zones (Europe/London, Australia/Sydney, Pacific/Auckland) at 00:01, 12:01, 23:01.

Should $expected_date = new DrupalDateTime('now', DATETIME_STORAGE_TIMEZONE); on DateTimeFieldTest.php:712 and 745 be relative to user's time zone?

UPDATE: Or at least - as per #2627512 comment #84 option 3 - keeping DATETIME_STORAGE_TIMEZONE but adding the offset between DATETIME_STORAGE_TIMEZONE and user's TZ.

As we can't alter the request time, the only way possible is probably what I've been doing manually and - from what I can see - you are already doing on the test: running the checks with a -12 and a +12 TZs and at least one of them will always fail the assertEqual()s for the default date.

I ran the updated test without the patch and it fails tests for Pacific/Kwajalein, Pacific/Funafuti and Pacific/Tongatapu.
With the patch everything passes.

@mpdonadio I didn't say (neither mean) the problem is the request time. What I meant is the most reliable way to demonstrate this properly would be to manipulate the time values to force the test to fail where and how we want, but this is impossible at the moment (it will be when/if introducing Clock Mocking).

So the only way possible I see is to test a wider range of TZs. Without the patch test will always fail.

Do you think there could be a better solution?

If you continue to trace this out, that value eventually gets passed to the \DateTime constructor. This means it is current time, not request time, is the basis for that.

Since using 'now' goes all the way back to PHP, perhaps we don't need to actually have the test use 'now', but rather the request time plus an offset? Meaning, we can assume that 'now' will always work in a similar fashion as something like REQUEST_TIME + 5 hours, since all that logic is contained in PHP itself.

'now' and REQUEST_TIME aren't always the same thing, especially with integration tests when you are installing modules, rebuilding caches, etc.

For the purposes of this issue though, is it specific to a default value of 'now', or is the bug with all default values? If the later, then there shouldn't be a need to test with a default value of 'now', but something close, which would be REQUEST_TIME, or REQUEST_TIME plus some fixed amount of time...

re #16 aaaah, I get it now. I thought this only referred to a test using 'now', forgetting we had this constant at all.

I think this is good to go. It's easiest to review when applied and do a diff ignoring white space.

just fixed some 80 char comment line wrapping since some comment indents changed.
(it does make git diff 8.3.x --ignore-space-change a little less clean)

Also a test only patch.

another nit, https://www.drupal.org/coding-standards#array last time in an array, should have trailing comma

(these were already changed lines in the patch)

@mpdonadio Does that mean committing this would commit a random fail? Is it depending on the date(line) of the testbot? Looking...
https://www.drupal.org/pift-ci-job/425785
Ah, those look like from StableTemplateOverrideTest and TwigTransTest
So not date related.

Back to RTBC. Thanks for the cleanup!

As a bugfix with no disruption, API additions, or internal BC breaks, this issue should be eligible for 8.2.x I think. (As far as I understand it is not an issue in 8.1.x.)

Back to RTBC after spurious fail.

In general rather than just saying something is a "spurious fail" we should document what it is and the existing critical to address it is, because random test failures are critical bugs somewhere in core or with DrupalCI. I get nervous when I see this many random fails in a short time on a patch, because it is possible to introduce failures in places people don't expect.

One fail I see was: https://www.drupal.org/pift-ci-job/435269:

Update.Drupal\views\Tests\Update\FieldHandlersUpdateTest
✗	
runUpdates
fail: [Other] Line 264 of core/modules/system/src/Tests/Update/UpdatePathTestBase.php:
Schema key views.view.files:display.default.display_options.cache failed with: missing schema

That is a potential fail when something happens to the test environment so that data is straight up missing from the bot, but also can happen if something is added without a schema. While this issue does touch some settings that would be reflected in schema, it does not have anything to do with the files view. Fails like this are a common result of #2791163: Random automatic testing failures on SQLite with PHP 5.5 and I think that issue may actually not only be about SQLite. So this issue's patch is almost certainly fine and hopefully fails like these will be less common in the future. :)

Committed 6992cbe and pushed to 8.3.x. Thanks!

Setting to patch to be ported for eventual cherry-pick to 8.2.x once 8.2.0 is released.

Committed 635c476 and pushed to 8.2.x. Thanks!