Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CRM-21818: set timezone when initializing civicrm #523

Merged
merged 1 commit into from May 1, 2018

Conversation

kerasai
Copy link

@kerasai kerasai commented Mar 1, 2018

JIRA Issue: https://issues.civicrm.org/jira/browse/CRM-21818

It seems as though the timezone handling was omitted when porting to Drupal 8.

See the Drupal 7 implementation.

@civicrm-builder
Copy link

Can one of the admins verify this patch?

@seamuslee001
Copy link
Contributor

Jenkins ok to test

@seamuslee001
Copy link
Contributor

ping @dsnopek @jackrabbithanna

@@ -49,7 +49,7 @@ public function initialize() {
}

// Initialize the system by creating a config object
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment here should probably be updated

@dsnopek
Copy link

dsnopek commented Mar 1, 2018

Otherwise, calling this method is good! It's needed to get correct timezone handling. (FYI, @kerasai is a colleague at MDW ;-))

@dsnopek
Copy link

dsnopek commented Apr 9, 2018

We've been using this in production for over a month. Is there anything we can do to help get this merged?

@totten
Copy link
Member

totten commented May 1, 2018

To facilitate distribution via composer, the Civi-D8 code is being migrated to a new repository with a slightly different branch naming convention (ie civicrm-drupal@8.x-master becomes civicrm-drupal-8@master). The repo is setup is now, and we need to wind-down or migrate any open D8 PRs.

Here's a quick review:

(CiviCRM Review Template WORD-1.1)

  • (r-explain) Pass
  • (r-test) Pass: The test-failure reported in Github UI can't possible be accurate -- the real issue is that we don't have a Civi-D8 test suite right now. That's not the PR's problem.
  • (r-code) Pass
  • (r-doc) Pass
  • (r-maint) Pass: We don't have a Civi-D8 test suite right now. That's not the PR's problem.
  • (r-run) Pass: Trusting @dsnopek's comments
  • (r-user) Pass: Small change and generally aligned with common practice in other CMS integrations
  • (r-tech) Pass: Small change and generally aligned with common practice in other CMS integrations. Even if there were an r-tech issue, it'd be OK to change slightly since we're still pre-release.

@totten totten merged commit 575ecea into civicrm:8.x-master May 1, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants