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
PlatformSqlabstract class in theDatabasenamespace, implemented for each driver. Similar to theSchemaclass. 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
DefaultDateSqlplugin that replaces the default class for theviews.date_sqlservice, and move theMysqlDateSqlplugin 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
}
| Comment | File | Size | Author |
|---|---|---|---|
| #18 | 2657888-18.patch | 2.47 KB | mondrake |
Comments
Comment #2
david_garcia commentedComment #4
david_garcia commentedThis is a patch for Drupal 7.
Comment #5
audriusb commentedpatch for D8
Comment #6
david_garcia commentedRolled D7 patch properly...
Comment #9
tim.plunkettSupport for other database drivers *within Views* is not a major bug.
Comment #10
david_garcia commentedAlthough 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.
Comment #12
mondrakeRerolled patch #5 for D8 and added Oracle support for ::getDateFormat
Comment #13
mondrakeDb abstraction is dealt with in #2627512: Datetime Views plugins don't support timezones.
Comment #14
mondrakeThis 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.
Comment #15
jhedstromI've closed #2745197: Move date SQL handling from Views to core database system as a duplicate of this one.
Comment #16
mpdonadioVery 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.
Comment #17
mondrake@mpdonadio
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.
Comment #18
mondrake#2627512: Datetime Views plugins don't support timezones has been committed.
A new start here: the idea is to have a
DefaultDateSqlplugin that replaces the default class for theviews.date_sqlservice, and theMysqlDateSqlplugin 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 aConnection::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.
Comment #20
mondrakeComment #21
jhedstromThe 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?
Comment #22
mondrake@jhedstrom
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
PlatformSQLclass like in #14, a stub implementation, and some tests.Comment #24
mondrakeComment #25
gchauhan commentedComment #26
mondrake@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.
Comment #27
mondrakeUpdated IS since the scenario changed quite a bit since OP.
Comment #28
mondrakeComment #32
mondrakeComment #33
daffie commented@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.
Comment #34
mondrake@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.
Comment #35
mondrakeActually, this is a feature request I think.
Comment #36
colanWould 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.