Problem/Motivation

The aim of the issue is to add support to the database abstraction layer for Date and Time manipulation functions so that they are abstracted away from their specific database implementation.

As of #2627512: Datetime Views plugins don't support timezones, this abstraction is provided as a backend-overrideable service for core supported DBMSs. However, alternative database drivers can not use a service, since that can only be specified by a module (AFAIK). But a database driver is NOT a module, not to say that SQL snippets may be also necessary to be available before the container is initialized.

Proposed resolution

  • introduce a PlatformSql abstract class in the Database namespace, implemented for each driver. Similar to the Schema class. It is meant to return db platform specific SQL snippets. Doctrine DBAL does something similar.
  • this class and its driver level implementations provide the 3 methods necessary to return the Sql snippets for dates. Later it could be expanded to more uses (thinking of the RAND() function for random, or the syntax for REGEXP like that is for instance very different in Oracle).
  • add a DefaultDateSql plugin that replaces the default class for the views.date_sql service, and move the MysqlDateSql plugin to be one of the replacing backends. This way the existing core-db plugins keep working as such (at least unless we want to move their code to the corresponding database drivers code). Contrib db drivers, on their end, can implement a Connection::getPlatformSql() method that the default plugin will invoke, and provide platform-specific implementations.

Remaining tasks

  • Agree approach.
  • Code.
  • Review etc.

User interface changes

None.

API changes

New API for getting platform-specific SQL snippets.

Data model changes

None.

Original report

Support for date formats is implemented in such a way that database drivers other than the ones in core are impossible to implement without modifying the module.

This code (just an example there are a few methods involved) was straight ported from D7, where it was also crippled:

function views_get_timezone() {
  global $user;
  if (variable_get('configurable_timezones', 1) && $user->uid && strlen($user->timezone)) {
    $timezone = $user->timezone;
  }
  else {
    $timezone = variable_get('date_default_timezone', 0);
  }

  // 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') {
        db_query("SET TIME ZONE INTERVAL '$offset' HOUR TO MINUTE");
      }
      elseif ($db_type == 'mysql') {
        db_query("SET @@session.time_zone = '$offset'");
      }

      $already_set = true;
    }
  }

  return $timezone;
}

NO WHERE IN CORE THERE SHOULD BE CODE IN THE FORM OF:

if ($db_type == 'mysql') {
  whatever
}
else {
 whetever else
}

Instead, this should be moved in one way or another to the database engine layer:

if ($connection->supportsWhateverFeature()) {
  whatever
}
else {
 whetever else
}

Comments

david_garcia created an issue. See original summary.

david_garcia’s picture

Issue summary: View changes

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

david_garcia’s picture

StatusFileSize
new4.1 KB

This is a patch for Drupal 7.

audriusb’s picture

StatusFileSize
new1.42 KB

patch for D8

david_garcia’s picture

StatusFileSize
new3.85 KB

Rolled D7 patch properly...

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

tim.plunkett’s picture

Priority: Major » Normal

Support for other database drivers *within Views* is not a major bug.

david_garcia’s picture

Title: Date/Date format support is not portable » Add Date function support in DTBNG

Although the patches here are to add support for MSSQL that is something that we don't want (and cannot be) in core.

The aim of the issue is to add support to the database abstraction layer for Date and Time manipulation functions so that they are abstracted away from their specific database implementation.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

mondrake’s picture

Rerolled patch #5 for D8 and added Oracle support for ::getDateFormat

mondrake’s picture

mondrake’s picture

Status: Active » Needs review
Issue tags: +Needs tests
StatusFileSize
new15.98 KB

This patch introduces a PlatformSql abstract class in the Database namespace, implemented for each driver. Similar to the Schema class. It is meant to return db platform specific SQL snippets. Doctrine DBAL does something similar.

For the moment this class implements the 3 methods necessary to abstract the views Sql class for dates, but it could be expanded to more uses (thinking of the RAND() function for random, or the syntax for REGEXP like that is for instance very different in Oracle).

The existing tests for view Sql already cover the changes, but probably more specific tests are needed.

jhedstrom’s picture

mpdonadio’s picture

Component: views.module » database system
Status: Needs review » Needs work

Very quick look; thanks for starting this. I think we need to do this as a backend_overridable service, https://www.palantir.net/blog/d8ftw-customizing-your-back-end.

Also moving this to database system. Even though this currently only happens in Views, we are shooting for a core solution that can be used all over.

mondrake’s picture

@mpdonadio

I think we need to do this as a backend_overridable service

That's what #2627512: Datetime Views plugins don't support timezones does, and the cause of my concerns in #2627512-253: Datetime Views plugins don't support timezones and #259.

As a database driver developer, I may want to run CI testing on the core PHPUnit Drupal test suite just swapping the database driver. HEAD is only #2605284: Testing framework does not work with contributed database drivers away from being able to do this structurally. My concern is that to be able to use a service, that service should be available from the container, and that service can (AFAIK) only be specified by a module. But a database driver is NOT a module, not to say that SQL snippets may be also necessary to be available before the container is initialized. The database driver could also add a module that defines the service, but that would not allow us to run the PHPUnit tests as such - somehow tests should install the module before running the tests.

mondrake’s picture

Status: Needs work » Needs review
StatusFileSize
new2.47 KB

#2627512: Datetime Views plugins don't support timezones has been committed.

A new start here: the idea is to have a DefaultDateSql plugin that replaces the default class for the views.date_sql service, and the MysqlDateSql plugin moved to as one of the replacing backends. This way the existing core-db plugins keep working as such (at least unless we want to move their code to the corresponding database drivers code). Contrib db drivers, on their end, can implement a Connection::getPlatformSql() method that the default plugin will invoke, and we can run core test on the contrib db driver without having to add a backend_overridable service (that would require the db driver to also implement itself as a full-blown module with a xxx.services.yml).

No interdiff as it won't make sense.

Status: Needs review » Needs work

The last submitted patch, 18: 2657888-18.patch, failed testing. View results

mondrake’s picture

Status: Needs work » Needs review
jhedstrom’s picture

The approach in #18 makes sense to me, but would we still move most of the date logic to the core database system, so it can be used outside of views?

mondrake’s picture

@jhedstrom

but would we still move most of the date logic to the core database system, so it can be used outside of views?

well I guess that's the discussion we need to have here. If we were to do so, then eventually all the code currently in the 3 core plugins would be moved to corresponding driver classes, so that the 3 plugins will be redundant (hence, deprecated?).

From my POV, we could even leave that to a separate issue, and here just introduce an abstract PlatformSQL class like in #14, a stub implementation, and some tests.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

mondrake’s picture

gchauhan’s picture

Status: Needs review » Reviewed & tested by the community
mondrake’s picture

Status: Reviewed & tested by the community » Needs work

@gchauhan sorry but this is still far from being able to be committed... needs discussion on the approach and in any case a good bunch of tests.

mondrake’s picture

Issue summary: View changes

Updated IS since the scenario changed quite a bit since OP.

mondrake’s picture

Title: Add Date function support in DTBNG » Add Date function support in DBTNG

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

mondrake’s picture

daffie’s picture

@mondrake: Now that #3120096: Support contrib database driver directories in a fixed location in a module has landed. It should now be possible for you to create a views plugin in the module part of your database driver. In this way we do not have to change Drupal core for your database driver. Does this a solution work for you? If not, then please say why, because I would love to have this issue fixed.

mondrake’s picture

@daffie I can work that out, yes. Only, the core tests won't work as such with the module-driver because we'd have to install the module also in the test by adding that to the $modules list, which we can only do by extending the test class and running it separately.

However, I think this issue is somehow different.

It's to move the data functions in the DB API, since as someone pointed out, the current implementation is only views specific whereas someone may want to use DB date functions in other contexts. See #2745197: Move date SQL handling from Views to core database system, and #2627512-3: Datetime Views plugins don't support timezones for instance. See also my comment in #14.

mondrake’s picture

Version: 8.9.x-dev » 9.1.x-dev
Category: Bug report » Feature request

Actually, this is a feature request I think.

colan’s picture

Would be good if the folks involved in this could comment on #3064640: Provide alternate storage backends for native DB date fields. Might that one be considered a duplicate? It sounds like a similar strategy, but different tactics.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.