Datetime Views plugins don't support timezones

Thanks for the patch. Couple quick thoughts.

This needs test coverage, and preferably a test-only patch demonstrating the problem, too. (NW for this part).

Not sure if adding a switch here is the best thing to do. What if someone wants to add support for Oracle or a NoSQL database in contrib? I think adding a method onto the database driver class may be the best thing to do. Going to try to raise a DB maintainer for an opinion.

A) these plugins are already SQL bound, heavily so

B) within SQL , instead of amending the database layer every time you can use the backend override mechanism and define services like

foo:
  class: Drupal\views\whatever
    tags:
      - { name: backend_overridable }
mysql.foo:
  class: Drupal\views\MysqlSpecificWhatever
pgsql.foo:

then injecting $container->get('foo') will get the right version. If someone writes the Oracle driver, they can just add an oracle.foo service. It's not magic, it's Drupal 8 :) Check BackendCompilerPass or talk to me if it doesn't make any sense.

+++ b/core/modules/datetime/src/Plugin/views/argument/Date.php
@@ -31,8 +32,29 @@ class Date extends NumericDate {
+    $db_type = Database::getConnection()->databaseType();

Nyet. If you need the DB connection, inject it.

That said, chx is correct. Any time you find yourself tempted to switch on $db_type, make a backend_overridable service instead.

Shameless plug: https://www.palantir.net/blog/d8ftw-customizing-your-back-end

That's much more like it, Ted!

1) The generic implementation should probably be called GenericDateField, rather than DateField, since we assume it will likely never get used.

2) Having a service for one single-line method seems wasteful, even if it is the correct approach here. Do we expect there to be other date-time-related DB branching? Perhaps we should generalize this class from a Views class to a generic class in core somewhere that Views can use, and we can add other date-time-related DB methods to it? Or is this logic Views-specific? Not sure on this one.

Factoring all of those switch statements out to a single swapped service seems logical, yes, since they would all change together. What those methods are doing now is unquestionably wrong. :-)

Having a service for one single-line method seems wasteful, even if it is the correct approach here. Do we expect there to be other date-time-related DB branching?

My spidey sense tells me that we may have some more MySQL specific date handling in core that doesn't have test coverage (a lot of Date and Views code was straight port w/o refactoring as D8 evolved), so it may or may not be working in PG or SQLite. I think adding something to centralize SQL date functions in the \Drupal\Core\Datetime namespace makes the best sense in the long run, rather than sticking this in with Views.

I would say we keep make the service w/ one function in this issue to address the bug, so we can get it into 8.0.x. Then make a followup to handle Database::getConnection()->databaseType() usage relating to date functions, and target it for 8.1.x.

Setting NR so TestBot runs. Here is an interdiff between #8 and #11. Still need tests to demonstrate the problem.

Untested. Just me reading the manuals for PostgreSQL and SQLite to stub out the files, and haven't tacked tests yet.

Still torn about keeping this Views centric, or making it more generic to live somewhere else.

Let's see if this fixes PostgreSQL. Help via @pwolanin.

This gets the Postgresql test passing. However, now it is entirely dealing with integer timestamps on the backend, which is probably ok, but a change nonetheless.

I also cleaned up some updated coding standards here.

Haven't had a chance to look at the SQLite fails. The PostgreSQL fails are because the changes in #22 to Sql::getDateFormat() impact more than just the argument plugin, so they break (filter and sorting).

I wonder if we could push all of the db-specific switch statements in \Drupal\views\Plugin\views\query\Sql into these backend-overridable plugins as part of this fix?

I don't see why not? The SQL class shouldn't have to know about these types of details.

This steps back from the datetime-specific backend overridable date field services, and moves them further up the stack into views. It doesn't yet address the bug here. The issue is that the offset is calculated in Sql::getDateField(), yet in the datetime module we skip that method entirely in the sort and argument date plugins:

class Date extends NumericDate {

  /**
   * {@inheritdoc}
   */
  public function getDateField() {
    // Return the real field, since it is already in string format.
    return "$this->tableAlias.$this->realField";
  }

So somehow (probably a new public method to Sql) we need to be able to get that offset in the datetime plugins.

Re-uploading to queue against 8.2.x.

This adds a test, and a fix. It could probably still use some unit tests for the new date sql service that was introduced above.

Oops, just realized MySQL and SQLite will fail spectacularly on the above patches :) Fixes for those 2 coming soon.

I think this has MySQL and PostgreSQL passing. SQLite needs work. (Re-uploading the test-only patch as it wasn't properly queued in #34).

This is turning into quite the issue. If we do the proper fix for arguments, it impacts the sort and filter plugins (which currently do not properly support timezones, although filter sort of does).

So, we essentially have to rescope this to cover all three set of plugins in order to fix the original bug?

re #40 I think that would be the best way to approach this. As I dig in, the timezone support is all over the place for datetime (the filter plugin does a conversion in code to UTC, while the sorting plugin ignores it entirely). If we are able to fix most of this in the views sql plugin, that seems ideal. Thoughts?

I should have something with more tests passing tomorrow.

This should have improvements in all 3 db backends. String date handling is now done natively in the sql plugin, rather than hacked in at the datetime plugin level, so it should be much cleaner.

The fails on mysql & sqlite were related to the date-only storage test. Now that timezone offsets are properly calculated, the test failed for the test timezone (Australia/Sydney). To address this, a new parameter was added to Sql::getDateField() to switch off timezone calculation.

The other fails on pgsql look random.

+++ b/core/modules/datetime/datetime.views.inc
@@ -39,6 +39,8 @@ function datetime_field_views_data(FieldStorageConfigInterface $field_storage) {
+          'entity_type' => $field_storage->getTargetEntityTypeId(),
+          'field_name' => $field_storage->getName(),

These additions were necessary for the FieldAPIHandlerTrait methods to work properly on datetime's argument plugins.

The pgsql fails look totally unrelated...and it looks like 8.2.x already has some fails on pgsql?

This adds unit tests for the 3 database-specific date query plugins.

Assuming this come up green (or just the PGSQL randoms), do you think this is ready for review? We want to get @dawehner or @tim.plunkett for final review since have delved into the bowels or Views now.

@mpdonadio it's ready for initial review anyway. I'm guessing there will be some tweaks needed once there are more eyes on this.

Wow. Not really seeing anything here; this is awesome work. The webtest looks good, covering a date changeover b/c timezone. The unit tests on the new classes look good, too. Going to ping some people for other looks, since this is some major refactoring.

1. +++ b/core/modules/datetime/src/Plugin/views/argument/Date.php
   @@ -22,12 +25,34 @@
   +    // If this is a date-only field, do not calculate the timezone offset.
   +    $definition = $this->getFieldStorageDefinition();
   +    if ($definition->getSetting('datetime_type') == DateTimeItem::DATETIME_TYPE_DATE) {
   +      $this->calculateOffset = FALSE;
   +    }
   +  }
   

   Maybe expand this to explain why?

2. +++ b/core/modules/views/src/Plugin/views/query/DateSqlInterface.php
   @@ -0,0 +1,56 @@
   +  /**
   +   * Set the database to the given timezone.
   +   *
   +   * @param string $offset
   +   *   The timezone.
   +   */
   +  public function setTimezoneOffset($offset);
   

   Maybe explain why this is needed?

3. +++ b/core/modules/views/tests/src/Unit/Plugin/views/query/MysqlDateSqlTest.php
   @@ -0,0 +1,97 @@
   +    $this->database = $this->prophesize(Connection::class)->reveal();
   +  }
   

   Note to self, use Prophecy more :)

Updating the title to reflect that this is all datetime Views plugins, rather than just arguments.

I also think this meets the criteria for a major issue since it would impact any site using configurable timezones, and datetime fields with views.

Really great work is going on here!

1. +++ b/core/modules/datetime/src/Plugin/views/argument/Date.php
   @@ -22,12 +25,34 @@
   +    // If this is a date-only field, do not calculate the timezone offset.
   +    $definition = $this->getFieldStorageDefinition();
   +    if ($definition->getSetting('datetime_type') == DateTimeItem::DATETIME_TYPE_DATE) {
   +      $this->calculateOffset = FALSE;
   +    }
   

   Just an example: those examples docs should better describe why something is done, not what its doing.

2. +++ b/core/modules/datetime/src/Tests/Views/ArgumentDateTimeTest.php
   @@ -59,6 +61,31 @@ public function testDatetimeArgumentYear() {
   +    $view->setDisplay('default');
   +    $this->executeView($view, ['2003']);
   +    $expected = [];
   +    $expected[] = ['nid' => $this->nodes[3]->id()];
   +    $this->assertIdenticalResultset($view, $expected, $this->map);
   +    $view->destroy();
   +
   +    // Test as a user with a different timezone.
   +    $this->config('system.date')
   +      ->set('timezone.user.configurable', TRUE)
   +      ->save();
   +    $user = $this->drupalCreateUser();
   +    $user->set('timezone', 'America/Vancouver');
   +    $user->save();
   +    $this->drupalLogin($user);
   +
   +    $view->setDisplay('default');
   +    $this->executeView($view, ['2002']);
   +    $expected = [];
   +    // Only the 3rd node is returned here since UTC 2002-01-01T00:00:00 is still
   +    // in 2001 for this user timezone.
   +    $expected[] = ['nid' => $this->nodes[3]->id()];
   +    $this->assertIdenticalResultset($view, $expected, $this->map);
   +    $view->destroy();
   

   Nice test!

3. +++ b/core/modules/views/src/Plugin/views/query/DateSqlInterface.php
   @@ -0,0 +1,56 @@
   +
   +/**
   + * Defines an interface for handling date queries with SQL.
   + */
   +interface DateSqlInterface {
   +
   

   On the longrun (not part of this issue at all) it would be nice if this interface would actually be part of the database system itself. Nothing here is bound to views itself, which is really nice!

4. +++ b/core/modules/views/src/Plugin/views/query/QueryPluginBase.php
   @@ -338,6 +343,33 @@ public function getCacheTags() {
   +  /**
   +   * Applies a timezone offset to the given field.
   +   *
   +   * @param string &$field
   +   *   The date field, in string format.
   +   * @param int $offset
   +   *   The timezone offset to apply to the field.
   +   */
   +  abstract public function setFieldTimezoneOffset(&$field, $offset);
   

   Can we provide a default implementation? Otherwise classes extending this class will break.

5. +++ b/core/modules/views/src/Plugin/views/query/QueryPluginBase.php
   index cfe593c..753fbb4 100644
   --- a/core/modules/views/src/Plugin/views/query/Sql.php
   
   --- a/core/modules/views/src/Plugin/views/query/Sql.php
   +++ b/core/modules/views/src/Plugin/views/query/Sql.php
   

   Does this mean Sql.php effectively implements DateSqlInterface?

6. +++ b/core/modules/views/src/Plugin/views/query/Sql.php
   @@ -1754,175 +1765,40 @@ public function aggregationMethodDistinct($group_type, $field) {
       */
   -  public function getDateField($field) {
   -    $db_type = Database::getConnection()->databaseType();
   -    $offset = $this->setupTimezone();
   -    if (isset($offset) && !is_numeric($offset)) {
   -      $dtz = new \DateTimeZone($offset);
   -      $dt = new \DateTime('now', $dtz);
   -      $offset_seconds = $dtz->getOffset($dt);
   -    }
   -
   -    switch ($db_type) {
   -      case 'mysql':
   -        $field = "DATE_ADD('19700101', INTERVAL $field SECOND)";
   -        if (!empty($offset)) {
   -          $field = "($field + INTERVAL $offset_seconds SECOND)";
   -        }
   -        break;
   -      case 'pgsql':
   -        $field = "TO_TIMESTAMP($field)";
   -        if (!empty($offset)) {
   -          $field = "($field + INTERVAL '$offset_seconds SECONDS')";
   -        }
   -        break;
   -      case 'sqlite':
   -        if (!empty($offset)) {
   -          $field = "($field + $offset_seconds)";
   -        }
   -        break;
   +  public function getDateField($field, $string_date = FALSE, $calculate_offset = TRUE) {
   +    $field = $this->dateSql->getDateField($field, $string_date);
   +    if ($calculate_offset && $offset = $this->getTimezoneOffset()) {
   +      $this->setFieldTimezoneOffset($field, $offset);
        }
   -
        return $field;
      }
    
      /**
       * {@inheritdoc}
       */
   -  public function setupTimezone() {
   -    $timezone = drupal_get_user_timezone();
   -
   -    // set up the database timezone
   -    $db_type = Database::getConnection()->databaseType();
   -    if (in_array($db_type, array('mysql', 'pgsql'))) {
   -      $offset = '+00:00';
   -      static $already_set = FALSE;
   -      if (!$already_set) {
   -        if ($db_type == 'pgsql') {
   -          Database::getConnection()->query("SET TIME ZONE INTERVAL '$offset' HOUR TO MINUTE");
   -        }
   -        elseif ($db_type == 'mysql') {
   -          Database::getConnection()->query("SET @@session.time_zone = '$offset'");
   -        }
   +  public function setFieldTimezoneOffset(&$field, $offset) {
   +    $this->dateSql->setFieldTimezoneOffset($field, $offset);
   +  }
   

   Nice improvements!

7. +++ b/core/modules/views/views.services.yml
   @@ -80,3 +80,16 @@ services:
   +  views.date_sql:
   +    class: Drupal\datetime\GenericDateField
   +    tags:
   +       - { name: backend_overridable }
   +  mysql.views.date_sql:
   

   Just nice!

Thanks for the detailed review!

• 1. I updated the new documentation to be about *why*
• 3. See #2745197: Move date SQL handling from Views to core database system
• 4. Done
• 5. Not technically anyway. It has similar methods, but isn't as clean as the new interface due to having to work with both legacy code and string + numeric dates. The new interface always works with a consistent format within each backend implementation.

Looked at latest patch, tests, and think this is good to go. I think my involvement in the current version of this patch has been limited to comments (think all of my code is now gone), so I don't see an issue with setting RTBC on this. If anyone disagrees, set back to Needs Review and we will see if we can get someone else to look at it again.

I see the hard work done so far, but the bug still persists for Date-only field and Views filter.

How to reproduce:
- Add a 'Date only' date field.
- Create a entity using today as date, i.e. 2016-07-15.
- Create a View, with a filter for the date field above and 'Is equal to' as operator
- Filter the View using 2016-07-15
You won't get any result (it will work if search for 2016-07-16).
I need to mention my timezone is Europe/London, not sure if does or doesn't affect the calculation, but the can reproduce the test on simplytest.me.

The reason is due the date being saved as 2016-07-15 onto the DB, but the query looks for DATE_FORMAT('2016-07-14T23:00:00', '%Y-%m-%d')

Not sure how to fix this. I can suggest few options when datetime type isDATETIME_TYPE_DATE like validating the user input - i.e.: strtotime($input) > 0 - and using the raw input value without formatting OR adding the offset to the UNIX timestamp so $this->dateFormatter->format() calls will calculate the right date.
But I'm sure there is a better D8 way.

@gambry can you try this again now that #2739290: UTC+12 is broken on date only fields is committed?

@jhedstrom I'm assuming running a 8.2.x instance + patch at #54 from simplytest includes both works. And the issue persists.

I'm now running a local instance just to double check.

UPDATE:
Yes, issue is still there.
The problem lives on how strtotime() defaults to the server time zone if a time zone parameter is not specified. So for instance appending 'UTC' would force strtotime() to read the string as UTC, fixing the issue.

Here a patch with the test to demonstrate this. The additional test use nodes dates values as view filters (testing both '=' and 'between' operators) and it fails.

I've also attached a possible patch, including the additional test and a fix. As mentioned on #62 the problem is strtotime() using the default/server timezone to render the UNIX time from the string, which can or can not be UTC.

The patch append DATETIME_STORAGE_TIMEZONE to the filter values in order to force the conversion to be done in UTC. I've applied the fix only on the Filter plugin, but I'm assuming other plugins have got the same issue and need to be fixed too.

PS: I've been using D8 since 5 days so I'm sorry for best practices or not-logical mistakes. Feel free to swear.

I've updated the patch in order to pass all tests, however I think generally there is something going wrong on the "time string -> UNIX timestamp -> ISO" conversion, at least on the Date views filter.

If you run the tests core/modules/datetime/src/Tests/Views/FilterDateTest.php when your request time is 1 min after midnight, on some scenarios they fail.
For example using REQUEST_TIME as 1468710060 which would be a 'now' if ran at '2016-07-17 00:01:00' from Europe/London timezone, but it's '2016-07-16 23:01' for UTC which the SQL renders as '2016-07-16' on its selects.

My original patch - appending DATETIME_STORAGE_TIMEZONE to all filter values for date-only fields - could potentially fix the issue, but it's impossible to use concurrently with $origin > 0 according with this:

Be careful not to use different time zones in each parameter unless that is intended.
PHP: strtotime

I'm also looking at $this->calculateOffset = FALSE; as another potential cause. I understand we don't deal with time for date-only dates, but during the filtering process we do a lot of conversions from/to time based values (the time strings, the UNIX times, the SQL date function parameters), so I think an offset calculation somewhere in the stack can be the solution.

Removing hardcoded leftover.

Here is the interdiff between #54 and #70.

Thanks @gambry for picking this up!

+++ b/core/modules/datetime/src/Plugin/views/filter/Date.php
@@ -101,8 +101,17 @@ public static function create(ContainerInterface $container, array $configuratio
+    // Add UTC timezone for not offset 'date' type dates.
+    if ($this->dateFormat === DATETIME_DATE_STORAGE_FORMAT && $origin === 0) {
+      $a .= ' ' . DATETIME_STORAGE_TIMEZONE;
+      $b .= ' ' . DATETIME_STORAGE_TIMEZONE;
+    }

@@ -120,7 +129,15 @@ protected function opBetween($field) {
+    // Add UTC for not offset 'date' type dates.
+    if ($this->dateFormat === DATETIME_DATE_STORAGE_FORMAT && $origin === 0) {
+      $value .= ' ' . DATETIME_STORAGE_TIMEZONE;
+    }

Simply appending the timezone as a string here makes me nervous, since the input could potentially be something that doesn't deal well with that.

What if instead we drop strtotime entirely, and use something along the lines of:

$a = new DateTimePlus($value);

and then set the timezone to the storage timezone for date-only fields with the setTimeZone() method on the datetime objects.

We can then convert to a timestamp using $a->format('U').

(This will simplify the implementation of #2749483: Datetime views filter plugin should allow granularity to be configurable too I think.)

+++ b/core/modules/datetime/src/Tests/Views/FilterDateTest.php
@@ -107,4 +107,43 @@ public function testDateOffsets() {
+    // Test offset for between operator. Only 'today' and 'tomorrow' node should appear.

Nit: comment needs to wrap at 80 characters.

If you run the tests core/modules/datetime/src/Tests/Views/FilterDateTest.php when your request time is 1 min after midnight, on some scenarios they fail.

I made a conceptual code in order to test the statement above and indeed it fails.
The idea is for 'now' time string to be Today at 00:01.

public function testDateOffsetEdge() {

    // Today, at 00:01.
    $request_time = strtotime($this->nodes[1]->field_date->first()->getValue()['value'] . 'T00:01');
    $request = $this->prepareRequestForGenerator();
    $request->server->set('REQUEST_TIME', $request_time);

    $this->testDateOffsets();
  }

I won't add this test to the patch queue because it would start looking messy, I'll leave it here for you to read and see my point, may be running locally and add to the patch later.

I would advise using DateTimePlus() instead of the native functions if we can. Once we get the time service in core (can't find issue now), then testing will be a lot easier. Overriding the builtins is a royal pain.

I'll work on this asap. In the meanwhile have you got any suggestion RE the issue at #73?

Basically relative dates - i.e. 'now' (20th July 2016) - can actually be different - i.e. becoming 19th July 2016 - when converted to ISO passing from UTC timezone.

I think we do need to cover 3 scenarios then, with different requirements:

• If datatime_type is DATETIME_TYPE_DATETIME - the default scenario - don't touch the value, $this->calculateOffest will do the conversion
• If datatime_type is DATETIME_TYPE_DATE and value type is date use DateTimePlus() as described by #72
• If datatime_type is DATETIME_TYPE_DATE and value type is offset [[here I need someone else hints]]

Attached my changes.

I'm using DateTimePlus() as suggested, I've also corrected the > 80 chars comment and also introduced the test at #73.

I just want to flag the FilterDateTimeTest() tests twice a scenario when view date filter operator is 'between' but there isn't a 'min' value.
However I can't replicate this from the UI.
Trying to create a filter without specifying a 'min' value raises a validation error (both min and max are required), and as exposed filter filtering without 'min' ignores it at all.
So ideally test should be removed or - at least - documented as "not replicable from the UI".
Due a big difference between how strtotime() and DateTimePlus() process "" as date string (1970-01-01 00:00:00 the first, 'now' the last), the test above forced me to add the following line in the filter/Date::opBetween():

    // Process empty 'min' value as UNIX timestap 0.
    $min = (!empty($this->value['min'])) ? $this->value['min'] : '@0';

fixing Drupal\Tests\views\Unit\Plugin\query\SqlTest, adding $date_sql parameter to instance at line 404.

Reviewing interdiff-54-76.txt to just look at the last set of changes.

+++ b/core/modules/datetime/src/Plugin/views/filter/Date.php
@@ -100,14 +101,27 @@ public static function create(ContainerInterface $container, array $configuratio
+    $origin_offset = ($this->value['type'] == 'offset') ? $this->requestStack->getCurrentRequest()->server->get('REQUEST_TIME') - REQUEST_TIME : 0;

I don't see how $this->requestStack->getCurrentRequest()->server->get('REQUEST_TIME') and REQUEST_TIME are ever different, so the $origin_offset will always be 0? Few times in the patch.

+++ b/core/modules/datetime/src/Tests/Views/FilterDateTest.php
@@ -107,4 +107,75 @@ public function testDateOffsets() {
+    // Today, at 00:01.
+    $request_time = strtotime($this->nodes[1]->field_date->first()->getValue()['value'] . 'T00:01');
+    $request = $this->prepareRequestForGenerator();
+    $request->server->set('REQUEST_TIME', $request_time);
+

Since we are avoiding strtotime() in the patch b/c TZ weirdness, we should probably avoid it here, too. Seems to be that we can get the value, convert it to a timestamp, and then just add / subtract fudge factors to simulate the edge cases.

+++ b/core/modules/datetime/src/Tests/Views/FilterDateTest.php
@@ -40,11 +40,11 @@ protected function setUp() {
-      \Drupal::service('date.formatter')->format(static::$date + 86400, 'custom', DATETIME_DATE_STORAGE_FORMAT, DATETIME_STORAGE_TIMEZONE),
+      \Drupal::service('date.formatter')->format(static::$date + 86400, 'custom', DATETIME_DATE_STORAGE_FORMAT, drupal_get_user_timezone()),
       // Today.
-      \Drupal::service('date.formatter')->format(static::$date, 'custom', DATETIME_DATE_STORAGE_FORMAT, DATETIME_STORAGE_TIMEZONE),
+      \Drupal::service('date.formatter')->format(static::$date, 'custom', DATETIME_DATE_STORAGE_FORMAT, drupal_get_user_timezone()),
       // Yesterday.
-      \Drupal::service('date.formatter')->format(static::$date - 86400, 'custom', DATETIME_DATE_STORAGE_FORMAT, DATETIME_STORAGE_TIMEZONE),
+      \Drupal::service('date.formatter')->format(static::$date - 86400, 'custom', DATETIME_DATE_STORAGE_FORMAT, drupal_get_user_timezone()),

Scratching my head here on whether this is a valid change. Storage is always DATETIME_STORAGE_TIMEZONE (UTC), and the widgets / formatters convert back and forth when dealing with fields, so it doesn't seem right that we are using the user TZ here and saving that right into the field value.

Thoughts on all of this?

I don't see how $this->requestStack->getCurrentRequest()->server->get('REQUEST_TIME') and REQUEST_TIME are ever different, so the $origin_offset will always be 0? Few times in the patch.

I think the idea of $this->requestStack->getCurrentRequest()->server->get('REQUEST_TIME') is to get a mock-up-able REQUEST_TIME value to use as origin, feature I actually use in FilterDateTest::testDateOffsetEdge*().

They will be different as soon as something changes the RequestStack REQUEST_TIME value, and I think we should consider the difference.

I'm also calculating the offset - and not the origin as before - because DateTimePlus doesn't take origin parameter on processing relative date string. The offset is added later on right before using the timestamps.

Since we are avoiding strtotime() in the patch b/c TZ weirdness, we should probably avoid it here, too

I agree. I'll change it as soon as everybody is happy with the review.

Scratching my head here on whether this is a valid change. Storage is always DATETIME_STORAGE_TIMEZONE (UTC)

Yes, storage is always UTC.
And specifically when storing 'date' type dates the system doesn't convert the values, considering them UTC since the form submission.
But when the view filters process the value(s) they use the user/default timezone (before with strtotime() as well as now with DateTimePlus()).
The change is needed for all the tests using the dates values as filters. If we use UTC on creating the values, then we have to change the user/system timezone to UTC right before executing the view.

As I think the idea of the test is to simulate the closest possible the UI/UX, I felt more reasonable using the user's time zone on creating the dates values.

To test this it's easy, just compare $this->nodes[1] date value (today) with its generated storage query condition to see their gap. Before #78 the values are different, potentially causing error described on #73.

I understand it looks like a continuous ping-pong of TZ conversion, however it kind of makes sense and mainly it's the only reasonable way I can see to get all tests/scenarios pass.

I think the idea of $this->requestStack->getCurrentRequest()->server->get('REQUEST_TIME') is to get a mock-up-able REQUEST_TIME value to use as origin, feature I actually use in FilterDateTest::testDateOffsetEdge*().

Yes, getting the REQUEST_TIME via the current request is the best practice.

They will be different as soon as something changes the RequestStack REQUEST_TIME value, and I think we should consider the difference.

Nobody should ever be changing the value of the REQUEST_TIME outside of tests. In fact, the global is (hopefully) going to be deprecated soon (see #2717207: Add a time service to provide consistent interaction with time()/microtime() and superglobals). I think we can ignore the difference and simplify the logic here.

As I think the idea of the test is to simulate the closest possible the UI/UX, I felt more reasonable using the user's time zone on creating the dates values.

The problem is, that the test isn't matching what is really going on. I think it would be better if the test takes the REQUEST_TIME and user timezone into consideration, and then ensures that the values that gets into storage is correct, without those lines changing. The net result should probably be the same values, but we can comment the process to make it very clear what is happening and why (which is very important in tests).

And please don't take this as harsh. This was a very good catch, and I think this patch is very close now and shows good work. We have just had way too many TZ related problems that I want to be extra careful that we don't just get a passing test. I want to ensure that our tests demonstrate as close as possible what really goes on (especially in ones like these where they are closer to kernel tests that true integration tests) and also prevent future regressions.

I think we can ignore the difference and simplify the logic here.

The problem is I'm not aware of any function other than strtotime() taking in consideration an origin parameter.
And as we dropped strtotime() the only way to consider the requestStack REQUEST_TIME other than the original REQUEST_TIME on processing relative date strings is to calculate their offset with $this->requestStack->getCurrentRequest()->server->get('REQUEST_TIME') - REQUEST_TIME, to be added to the date timestamp.

If we remove the - REQUEST_TIME bit than we need to either:
- Remove the tests where we change the requestStack REQUEST_TIME (FilterDateTest::testDateOffsetEdge*())
- Rethink the whole $origin consideration on processing relative dates with DateTimePlus().

Open to any suggestion.

The problem is, that the test isn't matching what is really going on. I think it would be better if the test takes the REQUEST_TIME and user timezone into consideration, and then ensures that the values that gets into storage is correct, without those lines changing.

I'm not sure if here you are still referring to the REQUEST_TIME offset bit or to using the user TZ instead of UTC on the test.
The test now takes the user TZ into consideration and as we are dealing with 'date' types there isn't any storage conversion on saving them.

Can you be a bit more specific?

And please don't take this as harsh.

Please be! I don't mind. We are developer after all, we have the rights to be rude. :)
And honestly we all want this issue to be sorted - may be before 8.2.x window closes - so everybody should be aware of the pressure for producing a high quality patch.

The problem is I'm not aware of any function other than strtotime() taking in consideration an origin parameter.
And as we dropped strtotime() the only way to consider the requestStack REQUEST_TIME other than the original REQUEST_TIME on processing relative date strings is to calculate their offset with $this->requestStack->getCurrentRequest()->server->get('REQUEST_TIME') - REQUEST_TIME, to be added to the date timestamp.

If we remove the - REQUEST_TIME bit than we need to either:
- Remove the tests where we change the requestStack REQUEST_TIME (FilterDateTest::testDateOffsetEdge*())
- Rethink the whole $origin consideration on processing relative dates with DateTimePlus().

I think we can remove that particular set of tests. When we get the time service in core, we can look into better coverage of edge cases.

I'm not sure if here you are still referring to the REQUEST_TIME offset bit or to using the user TZ instead of UTC on the test.
The test now takes the user TZ into consideration and as we are dealing with 'date' types there isn't any storage conversion on saving them.

I am not comfortable with this hunk:

+++ b/core/modules/datetime/src/Tests/Views/FilterDateTest.php
@@ -40,11 +40,11 @@ protected function setUp() {
-      \Drupal::service('date.formatter')->format(static::$date + 86400, 'custom', DATETIME_DATE_STORAGE_FORMAT, DATETIME_STORAGE_TIMEZONE),
+      \Drupal::service('date.formatter')->format(static::$date + 86400, 'custom', DATETIME_DATE_STORAGE_FORMAT, drupal_get_user_timezone()),
       // Today.
-      \Drupal::service('date.formatter')->format(static::$date, 'custom', DATETIME_DATE_STORAGE_FORMAT, DATETIME_STORAGE_TIMEZONE),
+      \Drupal::service('date.formatter')->format(static::$date, 'custom', DATETIME_DATE_STORAGE_FORMAT, drupal_get_user_timezone()),
       // Yesterday.
-      \Drupal::service('date.formatter')->format(static::$date - 86400, 'custom', DATETIME_DATE_STORAGE_FORMAT, DATETIME_STORAGE_TIMEZONE),
+      \Drupal::service('date.formatter')->format(static::$date - 86400, 'custom', DATETIME_DATE_STORAGE_FORMAT, drupal_get_user_timezone()),

since it it not mirroring what is going on inside the widget. I think we can alter the code that computes `static::$date` to better mimic what is going on inside DateTimeWidgetBase, and end up with the same value, so that there is no change to the tests themselves.

And honestly we all want this issue to be sorted - may be before 8.2.x window closes - so everybody should be aware of the pressure for producing a high quality patch

.

Since this is a bug, this can get into core at any time, but I do know a lot of people are anxious to get this wrapped up.

I am not comfortable with this hunk:
[...]
since it is not mirroring what is going on inside the widget.

I got your point. I felt free to use a different timezone as the DateTimeWidgetBase for DateTimeItem::DATETIME_TYPE_DATE is TZ agnostic, so whatever TZ you input either on the widget or directly saving the node is not converted.

And just for clarity the reason I opted for the user TZ is instead of the storage one is due the 'today' created with \Drupal::service('date.formatter')->format(static::$date, 'custom', DATETIME_DATE_STORAGE_FORMAT, DATETIME_STORAGE_TIMEZONE), not being equal with the 'today' created with the 'now' string in the offsets tests.
While the 'today' created with \Drupal::service('date.formatter')->format(static::$date, 'custom', DATETIME_DATE_STORAGE_FORMAT, drupal_get_user_timezone()) it is equal.

I'm seeing only 3 solutions in here:
1. Using the drupal_get_user_timezone() on creating the date values.
2. Restoring DATETIME_STORAGE_TIMEZONE on creating the date values and using it as user/default TZ so relative conversions work.
3. Restoring DATETIME_STORAGE_TIMEZONE on creating the date values and considering the offset between the user TZ and UTC while calculating the`static::$date`.

I see option 1 more linear and self-explaining, 2 is totally to be dropped because the aim of this issue is to consider timezones other than UTC, 3 better mirrors the date widget and I think it's the one you are proposing here, correct?

So far I've collected these reviews/changes:
- Removing the $origin calculation, until we get the time service in core.
- Removing edge tests, until we get the time service in core.
- Replacing strtotime() call with DateTimePlus on edge tests (suppressed from change above)
- Implement solution 3 from the list above.

If you are happy I'll apply these changes.

#84, I think that sounds like plan to wrap this up, but I pinged @jhedstrom to see if he has any additional thoughts on this.

I agree that option 3 in #84 is the way to proceed here, since as noted it mimics the behavior in the date widget and formatters.

Attached the reviewed patch.
To simplify review against original jhedstrom's patch at #54 I've attached the interdiff with that as well.

This is looking great. Thanks @gambry!

I'll leave it for @mpdonadio to do the final RTBC.

+++ b/core/modules/datetime/src/Tests/Views/FilterDateTest.php
@@ -30,8 +30,9 @@ class FilterDateTest extends DateTimeHandlerTestBase {
+    // Set to UTC timestamp equivalent to user's 'now'
+    // in order for them to match when formatted as Y-m-d.
+    static::$date = REQUEST_TIME + timezone_offset_get(new \DateTimeZone(drupal_get_user_timezone()), new \DateTime('now', new \DateTimeZone(DATETIME_STORAGE_TIMEZONE)));
 

I think this looks good and RTBC, but I am looking at this and wondering if the 'now' in the hunk above will lead to spurious fails, instead of something derived from REQUEST_TIME.

Maybe

$now = new \DateTime('@' . REQUEST_TIME);
$now->setTimezone(new \DateTimeZone(DATETIME_STORAGE_TIMEZONE));
static::$date = REQUEST_TIME + timezone_offset_get(new \DateTimeZone(drupal_get_user_timezone()), $now);

would be safer? Middle line is there because TZ is ignored with @ formats...

No, that hunk is just getting the timezone offset from the \DateTime; you can use just about anything there near REQUEST_TIME and get the same value. The parameter is so that the calculation works for dates where time zones (the named ones, not the offset) have changed DST rules / handling (we actually have test coverage of this somewhere).

Think this is good to go, and test coverage is good, and comment #36 has a test-only patch that shows that this is a real bug.

We just entered 8.2.x beta. According to https://www.drupal.org/core/d8-allowed-changes#beta, this is allowed per

bug fixes and contributed project blockers, if they are non-disruptive, or if the impact outweighs the disruption

as this is a non-disruptive bug fix.

I do not think this is safe to assume this works with 8.1.x b/c #2739290: UTC+12 is broken on date only fields changed the way date-only worked, and I would be OK with this just getting into 8.2.x.

+++ b/core/modules/datetime/src/Tests/Views/FilterDateTest.php
@@ -30,8 +30,9 @@ class FilterDateTest extends DateTimeHandlerTestBase {
+    static::$date = REQUEST_TIME + timezone_offset_get(new \DateTimeZone(drupal_get_user_timezone()), new \DateTime('now', new \DateTimeZone(DATETIME_STORAGE_TIMEZONE)));

The issue seems to live here, on the '+' sign.

Tests run on Australia/Sydney timezone which is UTC +10, Australia/Sydney 10:01 am = UTC 00:01 am.
So when a date value is processed by the date.formatter service, the format() method adds the offset again. Consequently $static::date needs to be REQUEST_TIME - offset too.

Tests never spotted this because #87 patch has always been tested before Australia/Sydney midnight! :)

#78 has been tested out of this range, but its test didn't have this code/bug.

I won't update the patch yet as (1) I would like to have your thoughts and (2) I want to test a TZs with negative offset to see if REQUEST_TIME - offset works as well.

I forgot to mention locally tests pass subtracting the offset, instead of adding.

Let's add the time zone loop like we do in the field tests to make sure we really squash this.

I made a POC of test which seems to work for all timezones.

The core of the change is:
REQUEST_TIME + timezone_offset_get(new \DateTimeZone(drupal_get_user_timezone()), new \DateTime('now', new \DateTimeZone(DATETIME_STORAGE_TIMEZONE)));
becomes
REQUEST_TIME - abs(timezone_offset_get(new \DateTimeZone(drupal_get_user_timezone()), new \DateTime('now', new \DateTimeZone(DATETIME_STORAGE_TIMEZONE))));
subtracting the absolute value of the offset.

The rest of the changes are due date values calculations and storage can't stay on setUp() anymore.
My understanding is we can't have @dataProvider on WebTests, so I made my best to keep everything clean and clear.

The patch in #104 seems to have lost the new sql date plugins (thus the fails).

Sorry. Re-uploading.

Tests pass subtracting the offset when UTC time value is pm, but adding when value is am.
At least we - hopefully - hit the bottom.

Before updating the patch with a weird sign switcher, I want to see if it's possible to get UTC equivalent timestamp of user's 'now' using DateTimePlus() instead of working out the right offset with awful code.

On the test I've replaced this bit:

REQUEST_TIME + timezone_offset_get(new \DateTimeZone(drupal_get_user_timezone()), new \DateTime('now', new \DateTimeZone(DATETIME_STORAGE_TIMEZONE)));

with a DateTimePlus() implementation.

After re-factoring #78 I lost a required bit for offset type date filters, and I think last errors were due to that being missing.

Tests pass locally, I've been also changing my machine date setting to test all TZs at different time/date.

I've been manually adding tests to cover different time ranges. All pass successfully.

I've just added last one now and if passes as well I think we just need:
- a test-only patch again, for double checking.
- a review of FilterDateTest() re-factoring, after including TZs loop. #108 interdiff.
- a review of the re-added missing part. #112 interdiff.
- a beer and a honest burger to everyone.

These updates look good to me. Leaving for @mpdonadio for final review.

Took me a while to understand what was going on with the last set of interdiffs to handle the TZ loop, but I think we are good to go.

It's my fault as I was assuming the fix for the Date filter was OK and issue was in the test and specifically the way we calculate the current users date as UTC timestamp.
I was wrong so my comments between #101 and #111 are not to be considered.

#112 corrects both as I restored the right code fix and re-factored the way we calculate the date, using DateTimePlus().

Thanks for reviewing and sorry again for the mess.

Needs asap reroll

faced with the issue trying to use sqlite
Drupal\Core\Database\DatabaseExceptionWrapper: Exception in Episodes Next Last[episodes_next_last]: SQLSTATE[HY000]: General error: 1 no such function: UNIX_TIMESTAMP: SELECT node__broadcast_time.broadcast_time_value AS node__broadcast_time_broadcast_time_value, node_field_data.nid AS nid, node_field_data.langcode AS node_field_data_langcode FROM {node_field_data} node_field_data LEFT JOIN {node__duration} node__duration ON node_field_data.nid = node__duration.entity_id LEFT JOIN {node__broadcast_time} node__broadcast_time ON node_field_data.nid = node__broadcast_time.entity_id WHERE (( (node_field_data.status = :db_condition_placeholder_0) AND (node_field_data.type IN (:db_condition_placeholder_1)) AND (UNIX_TIMESTAMP(node__broadcast_time.broadcast_time_value) + node__duration.duration_value >= :time) )) ORDER BY node__broadcast_time_broadcast_time_value ASC LIMIT 4 OFFSET 0; Array ( [:db_condition_placeholder_0] => 1 [:db_condition_placeholder_1] => episode [:time] => 1472563553 ) in Drupal\views\Plugin\views\query\Sql->execute() (line 1488 of /srv/core/modules/views/src/Plugin/views/query/Sql.php).

rerolled

Back to rtbc

Reroll bacause of #2721901: Fix Drupal.Commenting.FunctionComment.MissingParamName

-   * @param \Symfony\Component\HttpFoundation\RequestStack
+   * @param \Symfony\Component\HttpFoundation\RequestStack $request_stack

This issue does include API additions / internal changes, so it should be targeted against 8.3.x at this point.

Really great to see all the test coverage.

+++ b/core/modules/datetime/src/Plugin/views/sort/Date.php
@@ -14,12 +16,38 @@
+  /**
+   * {@inheritdoc}
+   *
    * Override to account for dates stored as strings.
    */

We're not supposed to combine @inheritdoc with other docs - this is just not supported by api.d.o. We should just move the comment inside the method.

I've not got time to finish my review. If another committer thinks the rest of this patch is okay - maybe just fix that on commit.

Do we need a change record for contrib/custom modules that are extending the date plugins changed in this patch? I think so.

Change record added: https://www.drupal.org/node/2813317. Can somebody review and bump this back to RTBC?

A lot of this relates to the CR. I may be misreading some of these (not reviewing it in place right now).

@alexpott, how much detail do we need in this CR? Can we just mention the new DateSqlBase / DateSqlInterface, and that database backends need to implement, or do we need to go into all of the gory details here?

1. +++ b/core/modules/views/src/Plugin/views/query/DateSqlBase.php
   @@ -0,0 +1,29 @@
   +abstract class DateSqlBase implements DateSqlInterface {
   +
   

   Needs to be mentioned in CR?

2. +++ b/core/modules/views/src/Plugin/views/query/DateSqlInterface.php
   @@ -0,0 +1,56 @@
   +interface DateSqlInterface {
   +
   

   Needs to be mentioned in CR?

3. +++ b/core/modules/views/src/Plugin/views/query/QueryPluginBase.php
   @@ -206,11 +206,16 @@ function loadEntities(&$results) {}
   -  public function getDateField($field) {
   +  public function getDateField($field, $string_date = FALSE, $calculate_offset = TRUE) {
   

   API change on a public function, so needs to be mentioned in CR?

4. +++ b/core/modules/views/src/Plugin/views/query/QueryPluginBase.php
   @@ -346,6 +351,36 @@ public function getCacheTags() {
   +  public function setFieldTimezoneOffset(&$field, $offset) {
   +    // No-op. Timezone offsets are implementation-specific and should implement
   +    // this method as needed.
   +  }
   

   API addition, needs to be mentioned in CR?

5. +++ b/core/modules/views/src/Plugin/views/query/QueryPluginBase.php
   @@ -346,6 +351,36 @@ public function getCacheTags() {
   +  public function getTimezoneOffset() {
   +    $timezone = $this->setupTimezone();
   +    $offset = 0;
   +    if ($timezone) {
   +      $dtz = new \DateTimeZone($timezone);
   +      $dt = new \DateTime('now', $dtz);
   +      $offset = $dtz->getOffset($dt);
   +    }
   +    return $offset;
   

   API addition, needs to be mentioned in CR?

6. +++ b/core/modules/views/src/Plugin/views/query/Sql.php
   @@ -126,11 +133,14 @@ class Sql extends QueryPluginBase {
   -  public function __construct(array $configuration, $plugin_id, $plugin_definition, EntityTypeManagerInterface $entity_type_manager) {
   +  public function __construct(array $configuration, $plugin_id, $plugin_definition, EntityTypeManagerInterface $entity_type_manager, DateSqlInterface $date_sql) {
   

   Added a plugin parameter, so I guess we need an empty hook_update_N() to force a container rebuild?

Change record added for the new date handling interface, and changes to the Views query plugin: https://www.drupal.org/node/2818693

@mpdonadio suggested an update hook be added to force the service container to be rebuilt, which makes total sense.

The update hook needs to be on views, since that's where the change is.

OK, I think the CR is adequate to announce the low level changes to Views, and we can adjust as needed if there are questions. Think this is ready again.

Are we going to consider this for 8.2.x, too? Not sure if it is too disruptive. If not, then we need to consider a sort-term fix for 8.2.x, #2825127: Views exposed filter on date only field gives incorrect results.

I think these are known, random fails related to update tests.

https://www.drupal.org/pift-ci-job/531288

https://www.drupal.org/pift-ci-job/523908

https://www.drupal.org/pift-ci-job/531327

Checking to see if there is a general testbot problem right now, but noticed this:

+++ b/core/modules/views/views.install
@@ -408,3 +408,19 @@ function views_update_8200() {
+
+/**
+ * @addtogroup updates-8.3.0
+ * @{
+ */
+
+/**
+ * Rebuild the container to add a new container parameter.
+ */
+function views_update_8300() {
+  // Empty update to cause a cache rebuild so that the container is rebuilt.
+}
+
+/**
+ * @} End of "addtogroup updates-8.3.0".
+ */
diff --git a/core/modules/views/views.services.yml b/core/modules/views/views.services.yml

Rer @catch:

We shouldn't use hook_update_N() for empty updates to trigger a cache flush - it adds all the potential complexity of schema versions between minor branches, conflicts with other patches, inconsistencies if there's a revert etc. hook_post_update_NAME() achieves the same thing without most of that trouble.

And per @catch the post-update hook can be empty in this case to achieve the same thing.

Fails seem completely unrelated. Missing schema files, etc? Seems like the testbot ran out of storage or something...

@dawehner, @tim.plunkett, @alexpott, @cilefen, and I agreed that this is a major bug.

Reuploading @jhedstrom's patch to retest, which should be with 5.6 now.

Did a quick re-review, and still think this is RTBC.

Resetting RTBC b/c https://www.drupal.org/pift-ci-job/572210 looked like an unrelated CI error.

Adding #2786577: The Views integration Datetime Range fields should extend the views integration for regular Datetime fields as a related issue. This issue is currently blocking progress on that one.

I think I found an error here. I guess this line was copied from the orBetween method, but the array key for accessing the value is incorrect.

If #149 comes up green, then we have a problem with test coverage.

https://www.drupal.org/pift-ci-job/585127 b/c a known random fail.

What baffles me is why this didn't trigger a fail, before or after.

FilterDateTest uses date-only fields, and the simple/offset variants of filtering, so the code path in the interdiff should have been hit and at least triggered a notice, which would cause a test fail.

?

'min' is a valid array key, and by default is an empty string. So PHP shouldn't trigger the notice.
Tests should also pass because as #92 states you can pass to that chunk almost anything. It just needs a valid Datetime object to create the timezones offset.

Am I correct?

I agree it should change to something else, may be 'value' is a good candidate, but if my observation is correct we should also mention it in a comment.

#153, yeah timezone_offset_get() needs the second argument to compute the offset for a particular date, as the offset for zone names can change. There have been very few in recent times, which is what I mostly meant in #92. And, yes, min/max are defaulting to empty strings, so no notices.

I eyeballed this again in-place, and think the code-change in #149 is good. Reviewing this again applied, I don't think it is worth contriving a test to cover this rare edge case. I also think the code is clear and commented enough.

I'm setting this back to RTBC. We need this patch in.

first it needs to be commited to current dev branch

I thought bugs, esp triaged majors, could get into the 8.3.x first at this point and then cherry picked to 8.4.x?

@mpdonadio not sure about "alpha stage" so 8.3 or 8.4 but anyway first commit should go to 8.4.x and then backport, mostly always commit should go to current dev

BUT I see php7 tests fail

Retriggering the PHP7 tests because they look unrelated JSTB fails:

https://www.drupal.org/pift-ci-job/586861

https://www.drupal.org/pift-ci-job/586862

https://www.drupal.org/pift-ci-job/586863

I'll create a f/up, but these look like they could be from the yet-to-have-an-issue mentioned in #2829040: [meta] Known intermittent, random, and environment-specific test failures and #2843693: Random test failure in CKEditor AjaxCss.

Had a very similar issue to what is described in #60 (on 8.2.5) and #149 fixed it nicely.

I've found a bug with this with relative dates.

I have a view with relative date filters:
- Date is greater than or equal to "first day of this month"
- Date is less than or equal to "last day of this month"

This results in the following query after the patch:

DATE_FORMAT(node__field_date.field_date_value, '%Y-%m-%d') >= DATE_FORMAT('2017-02-02T10:37:57', '%Y-%m-%d')) AND (DATE_FORMAT(node__field_date.field_date_value, '%Y-%m-%d') <= DATE_FORMAT('2017-03-01T10:37:57', '%Y-%m-%d')) ))

Which has clearly been affected by timezones after being converted from the relative string.

Is that a date+time field or date only? Should be pretty easy to add test coverage for this.

@mpdonadio that's date only, if no one else gets to it by this afternoon I can have a stab at the tests

#162, I think the problem is that those aren't relative dates. Relative dates are supposed to be offsets from the current time. "First day of month" and "Last day of month" are dynamic dates, but not offsets from the current time. When I choose offset, I see the problem you posted; when I choose date in any machine readable format", I get what I expect (and I am in the window now where it is "tomorrow" UTC).

@jhedstrom? @gamby? Thoughts. I don't think this is a code bug, just due to the fact that the help text on those widgets isn't clear (think I made an issue about that a few days ago).

EDIT: It seems to be working now, I think I missed applying the patch before testing (monday-itis) which means that we definitely need to update those descriptions as the functionality has changed for each option with this patch.

I don't think this is a code bug, just due to the fact that the help text on those widgets isn't clear (think I made an issue about that a few days ago).
PHP documentation mix these as well so I'm not sure how we can better explain than what the descriptions already say.

However I think any Relative Format should be used only on offset and this is because those formats are relative to the user (or the website) TZ.
I.e.: First day of the months for UTC+1 at 2017-03-01 00:00:01 !== First day of the months for UTC at the same UNIX timestamp.
The plugin will calculate and apply the offset accordingly.

And in fact if I use the values at #162 with type offset it works, although I've only tested for my TZ which is UTC+0 so I'm not reliable.

Then if I'm correct we can update the description of the offset type linking the allowed PHP relative formats and we need to fix the bug, if any.
#166 are you saying using the patch + still offset types it works?

UPDATE:
Using any relative date format different than an offset (i.e. now +1 day) results in a wrong calculation with both types:

• with type = date the offset is not considered, so the resulted date is relative to UTC:
  i.e.: 2017-03-01 00:14 Pacific/Kwajalein - first day of the month - DATE_FORMAT('2017-02-01T12:14:14', '%Y-%m-%d')
• with type = offset the offset is considered, but as the relative format is resolved at the beginning the offset is applied to the UTC date:
  i.e.: 2017-03-01 00:14 Pacific/Kwajalein - first day of the month - DATE_FORMAT('2017-02-02T12:14:14', '%Y-%m-%d')

Not sure how to react to this problem.

If the findings in #167 are caused by this patch, then we need additional test coverage and a fix. If it is already an issue without this patch, then let's pursue that fix in a follow-up and get this in, as it is quite a major blocker for a lot of folks.

#168, I don't know how we can really address this w/in the scope here since we would now be messing with a broader portion of Views (all of dates, not just datetime) and potentially string changes. Since we take text input and stuff it into `new DateTimePlus()` a user can type in just about anything. And, with string input, I don't know how we can differentiate relative versus non-relative input.

Maybe this just needs a CR to announce the behavior change, and outline the pitfalls of relative dates and using things like "first day of month" for entry?

My suggestion is to get this patch in, and create a f/up to improve the UX of these plugins, both from a user input perspective and help text perspective. This is a hard patch to review as it is, I think followups are the best option.

Ideally, I think `Drupal\views\Plugin\views\filter\Date` needs to be adjusted to use the '#datetime' form element for 'date' values, and '#text' for the relative option. For datetime fields, we would adjust the datetime form to just show the date for date-only. We could also have some shenanigans to switch out the '#datetime' for a '#text' if someone wants to do 'first day of this month'. And then, combine this with better help text. All of that can probably done without any b/c concerns, and we can do it in 8.4.x where we can do string changes.

#167here's a retest of several scenarios (with more coffee):

My timezone is Australia/Melbourne (UTC+10) and it's around 09:00AM on 7th February. I'm using a date only field.

With the patch applied (from #148)
Options set to "an offset from the current time", I get:
"first day of the month" translates to 2017-02-02T09:00:27
"last day of the month" translates to 2017-03-01T09:00:27

Options set to "a date in any machine readable format", I get:
"first day of the month" translates to 2017-02-01T22:01:55
"last day of the month" translates to 2017-02-28T22:01:55

Without the patch applied:
Options set to "an offset from the current time", I get:
"first day of the month" translates to 2017-01-31T22:08:32
"last day of the month" translates to 2017-02-27T22:08:32

Options set to "a date in any machine readable format", I get:
"first day of the month" translates to 1970-01-01T00:00:00
"last day of the month" translates to 1970-01-31T00:00:00

So you're right, with the patch the correct option to use is "a date in any machine readable format", and before the patch the offset option was broken anyway.

As you say though it would be very hard to detect a relative date with everything just being a string, I agree that this should be broken off into a separate follow up issue.

Uploading a patch for 8.2.5

Setting back to RTBC, and made #2850276: Improve the UX of the Views Date plugins as the followup. I postponed it initially; not sure if anything else we have in the pipeline also needs to block it.

Applying suggested change from #2851902: Replace $date->format('U') with $date->getTimestamp().
And as format() returned a string while getTimeStamp() returns an integer we don't need the intval() wrappers anymore as well.

Needs re-reroll b/c #2248983: Define the revision metadata base fields in the entity annotation in order for the storage to create them only in the revision table. Should be easy hand merge in core/modules/views/views.post_update.php

This is the re-roll of #174. As #2248983: Define the revision metadata base fields in the entity annotation in order for the storage to create them only in the revision table already defines a hook_post_update_NAME for 8.3.x group I'm not 100% sure what the best practices are here: if still keeping the empty hook or being happy with the other one triggering the cache rebuild.
However as I'm not sure they will be released in the same minor version - and anyway better safe than sorry - I left the views_post_update_date_sql_service() in.
This is a re-roll after all so there shouldn't be any code change.

Looks like a good re-roll. Took another quick look at patch and still think this is RTBC.

I'm not going to do the back-and-forth w/ the version, but since this is a major triaged bug, it should be 8.3.x eligible, even after 8.3.0 gets tagged. I think this is an important bug, so I am adding "rc target triage" to see if we can get this into 8.3.x-RC1.

A committer will have to make the final call of where the post update hook goes. That can be a fix-on-commit, and some empty ones have been getting stripped out anyway in cases where another post update hook is there.

Thanks @mpdonadio. As mentioned we're just going to look at issues discretionarily after commit for RC now. Although, dang, this poor issue has had a rough time bouncing into and out of the RTBC queue over months.

It's not a small patch though and is an addition, so it will probably be 8.4.x only. Hopefully we can get it landed in 8.4.x soon though because it's been disrupted so many times by so many things and I can only imagine how frustrating that is. :( Thanks for highlighting the importance of the issue.

Note on https://www.drupal.org/core/d8-allowed-changes#patch it says:

non-disruptive bug fixes

That applies even to major bugs. And then all of these things are minor-only:

Functionality and frontend

• new features
• changes to behavior that existing sites might rely on
• string, user interface, or render array changes
• CSS, markup, or template changes (except for stable base themes)

APIs

• changes to @internal APIs
• new APIs or API improvements with backward compatibility (BC)
• internal refactoring or improvements to maintainability with BC
• new deprecations

Third-party code

• patch- and minor-level library updates
• new library dependencies

Coding standards

• new coding standards rules

Disruptive changes

• changes requiring an upgrade path
• other disruptive bug fixes or high-risk changes

https://www.drupal.org/core/d8-allowed-changes#minor

Edit: I totally get though that this issue has been RTBCish for a long time including since right around the alpha and also on and off before that. Several other Views major bugfixes are in that category as well, unfortunately.

Re-rolling due #2776975: March 3, 2017: Convert core to array syntax coding standards for Drupal 8.3.x RC phase

#187 is missing unversioned (new) files, besides git for some reason added some trailing spaces. Re-uploading same patch as #181 with only the changes due #2776975: March 3, 2017: Convert core to array syntax coding standards for Drupal 8.3.x RC phase.

Big patch, and this comes up green; a lot of work went into the test coverage. I eyeballed this, too. I think this is a good re-roll.

Re-rolled.

Thanks for the quick reroll.

+++ b/core/modules/views/views.post_update.php
@@ -213,3 +213,10 @@ function views_post_update_revision_metadata_fields() {
diff --git a/core/modules/views/views.post_update.php.rej b/core/modules/views/views.post_update.php.rej

diff --git a/core/modules/views/views.post_update.php.rej b/core/modules/views/views.post_update.php.rej
new file mode 100644

new file mode 100644
index 0000000..52e36c1

index 0000000..52e36c1
--- /dev/null

--- /dev/null
+++ b/core/modules/views/views.post_update.php.rej

+++ b/core/modules/views/views.post_update.php.rej
+++ b/core/modules/views/views.post_update.php.rej
@@ -0,0 +1,20 @@

Looks like some cruft from the got into the patch with the `git add -A`. Otherwise, this looks good.

Oops, I got a bit sloppy there.

Patch to patch 8.3.x. Please ignore.

+++ b/core/modules/views/src/Plugin/views/query/QueryPluginBase.php
@@ -206,11 +206,16 @@ public function loadEntities(&$results) {}
+   *   If set to TRUE, the timezone offset will be included in the returned field.

Line exceeding 80 characters
Did minor change of coding standard to the #197 , suggest me if i am wrong.

Setting back to RTBC because the patch in #195 has not changed - the two patches since are not relevant to 8.4.x.

@PavanBS in future it is probably not worth correcting coding standards in a patch that someone has marked "please ignore".

The patch in #199 is 5k smaller than #195 and #197, but with a one line interdiff. When I get to work I will look into this and make sure the proper patch gets final review.

Aha, I now see what @PavanBS was trying to correct, but he somehow managed to delete a few files by accident too.

@mpdonadio would you like me to update the patch in #195 (there are also a couple of coding standards issues too)? Or shall I just leave it with you?

The changes were so straightforward that I went ahead and made them anyway. I hope you don't mind @mpdonadio.

This is a complex patch. I'm pretty confident we have good test coverage, but I won't have time to give it proper attention until tonight to make sure we didn't break anything or introduce any out-of-scope changes.

Compared #195 (the last RTBC patch) and #204 very closely, and they are all in-scope changes to the issue. This is a hard patch to run phpcs on, since it touches so much, but I still think this is OK. Before we polish this any further, lets see what a committer says. So, re-rolls only when absolutely necessary (true test failure b/c another issue or patch doesn't apply). And, if one of these does happen, please post what you had to adjust in the patch. For rerolls, the output from the rebase helps a lot. Thanks.

I'm getting a fatal from core/modules/datetime/src/Plugin/views/argument/Date.php when I apply this patch to 8.3 in combination with #2325899: UI fatal caused by views argument handlers no longer being able to provide their own default argument handling

Recoverable fatal error: Argument 4 passed to Drupal\datetime\Plugin\views\Argument\Date::__construct() must implement interface Drupal\Core\Routing\RouteMatchInterface, none given, called in core/modules/views/src/Plugin/views/PluginBase.php

Possibly the patch here should add the create() method in addition to the constructor?

Or at least, whichever patch goes in first, the other will need a fix.

Odd that ArgumentDateTimeTest doesn't fail. The Drupal\datetime\Plugin\views\Argument\Date class currently extends Drupal\views\Plugin\views\argument\Date, which has a create method and implements ContainerFactoryPluginInterface. But, #2325899: UI fatal caused by views argument handlers no longer being able to provide their own default argument handling removes those.

Gotta think some more about this.

I think it's ok to leave this at RTBC. The additional fix to the other patch to make it work together was tiny (remove the 4th constructor arg).

Whoever triggered the retest, do you have a link to the failed job so we can see what the failure was to see if it is a known one or a new one?

Re-rolled for 8.3.0. without the post-update hook.

Reuploading patches for bot runs.

2627512-214-w-update.patch has the post-update, and apples to 8.4.x
2627512-214-wo-update.patch doesn't have the post-update, and apples to 8.4.x and 8.3.x

PG fails were fixed in #2867887: statistics_get() test mashes 0 too much. This is still RTBC.

Heads up to anyone applying this patch to their composer-managed drupal install via the composer-patches plugin... the new files this patch introduced will not be stored in the proper location. See https://github.com/cweagans/composer-patches/issues/43 for more information on why.

I'm currently working on https://www.drupal.org/node/2868014 and the copious amount of tests required to make it succeed. I'm unsure on how best to proceed for tests that need to test hour and minute level granularity. The test date is created with UTC but the opSimple and opBetween methods assume that the timezone will only be UTC for date inputs that are restricted to 'Y-m-d'. This is a problem since my dates will include 'Y-m-d H' and 'Y-m-d H:i'. The code works in views but fails in the tests since timezones for those dates are always derived from drupal_get_user_timezone which for me differs from UTC considerably. Should we rethink just passing in the date values and allow for passing in a timezone as well? I don't want to impede this ticket from continuing but I don't want to take us in the wrong direction with my next patch if some reason I might be wrong in my thinking. @mpdonadio

core/modules/datetime/src/Plugin/views/filter/Date.php:101

  /**
   * Override parent method, which deals with dates as integers.
   */
  protected function opBetween($field) {
    $timezone = ($this->dateFormat === DATETIME_DATE_STORAGE_FORMAT)
      ? DATETIME_STORAGE_TIMEZONE
      : drupal_get_user_timezone();

core/modules/datetime/src/Plugin/views/filter/Date.php:145

  /**
   * Override parent method, which deals with dates as integers.
   */
  protected function opSimple($field) {
    $timezone = ($this->dateFormat === DATETIME_DATE_STORAGE_FORMAT)
      ? DATETIME_STORAGE_TIMEZONE
      : drupal_get_user_timezone();

#217, the code you show in #217 is primarily related to #2739290: UTC+12 is broken on date only fields. Basically, for the date-only flavor of datetime fields, the timezone is ignored (ie, the date is always in the local time for the user). For date+time fields, the timezone offset is applied. This patch enforces this behavior in views.

So, I suspect you have two problems in #2868014: [PP-1] Views Date Filter Datetime Granularity Option. One, is that it sounds like you are blocked on this issue (at least soft blocked). Two, you need to account for both date-only and date+time field options, and adjust the choices appropriately and then test the right cases.

I was able to fix my testing issue with the following code that was introduced in the above patch:

$user = $this->drupalCreateUser();
$user->set('timezone', 'UTC');
$user->save();
$this->drupalLogin($user);

More known randoms. Back to RTBC.

OK, reroll is b/c #2863267: Convert web tests of views.

Rolled back to c2f6edd, remove the hunks for FilterDateTest, applied 2627512-214-w-update.patch and rebased that against 8.4.x.

Then hand edited the FilterDateTest hunks to account for the move, and did a `git apply -3` to get them in. And adjusted the setup a smidge to install the node_access schema.

Passes locally...

Looks good to me, but need to see what the bot says. If these pass, I'll add the tests for the other DB backends.

These will no longer apply to 8.3.x b/c #2863267: Convert web tests of views was only against 8.4.x.

OK, here's a fix. Should be fully green now. Had to rework ArgumentDateTimeTest a bit now that it is a KTB. See interdiff for the change.