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

drush civicrm-install now accepts optional site_key #97

Open
wants to merge 5 commits into
base: 1.x-master
Choose a base branch
from

Conversation

tabroughton
Copy link
Contributor

When installing civicrm it is possible to set the site_key in civicrm.settings.php manually, the comment there gives guidance how to do this (eg. best to use md5sum etc).

When using drush to install civicrm there is no option to do this. This PR fixes that and adds an optional parameter site_key which if present will add the parameter to the civicrm.settings.php

This should be a non-breaking change to any code that's out there but adds needed functionality. I have tested with and without specifying a key and all works as expected.

@civicrm-builder
Copy link

Can one of the admins verify this patch?

@civibot
Copy link

civibot bot commented Aug 20, 2019

(Standard links)

@MegaphoneJon
Copy link
Contributor

Hi Tom - I've been lurking on your PRs with interest! This one caught my eye because I do a fair amount of programmatic deployment of CiviCRM. Do you want to say a bit more about your overall project, either here or on https://chat.civicrm.org? Your improvements are much appreciated, but I also want to make sure that you're not creating work for yourself. For instance, I usually use Ansible to deploy my site keys, and am happy to share ideas if it'll save you some effort.

@tabroughton
Copy link
Contributor Author

Hi Tom - I've been lurking on your PRs with interest! This one caught my eye because I do a fair amount of programmatic deployment of CiviCRM. Do you want to say a bit more about your overall project, either here or on https://chat.civicrm.org? Your improvements are much appreciated, but I also want to make sure that you're not creating work for yourself. For instance, I usually use Ansible to deploy my site keys, and am happy to share ideas if it'll save you some effort.

Thanks Jon, it would be good to chat yes. I'm currently just exploring (in quite a lot of detail) how Civicrm might be used with docker, to automate deployments for both development and production etc. It's all very new to me. I'm mainly interested in how close to getting to a self service model with as many settings and configurations done as possible without any manual intervention.

I'm on the chat channel, in backdrop and town square atm

@totten
Copy link
Member

totten commented Aug 20, 2019

jenkins, add to whitelist

@@ -512,6 +513,10 @@ function _civicrm_generate_settings_file($dbuser, $dbpass, $dbhost, $dbname, $mo
$cms = $v['cms'];
}

if (!$sitekey = drush_get_option('site_key', FALSE)) {
$sitekey = md5(uniqid('', TRUE) . $baseUrl);
Copy link
Member

Choose a reason for hiding this comment

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

This is tangential, but reading this code reminds me... uniqid() function is not a great source of entropy. I think this snippet of code probably originated in a much earlier iteration of civicrm-core:install/civicrm.php. That was a changed a while back (a few years ago?), so I'd suggest copying a more recent snippet, i.e.

Suggested change
$sitekey = md5(uniqid('', TRUE) . $baseUrl);
$sitekey = md5(rand() . mt_rand() . rand() . uniqid('', TRUE) . $baseUrl);

(Long term, we should swap this over to civicrm-setup so that code is not replicated as much; and we should update the shared code to prioritize better sources of entropy when available. But that's a bigger scope. The suggestion here is just sync-up with the current install/civicrm.php.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes I agree longer term this would be good to reuse code rather than replicate it, I'd noted that when I was looking at some other issues a few weeks ago (it puzzled me slightly). I'll add the code snippet you suggested and fix the few code style issues. The function I used was the one already in the drush script. I think an overhaul of this file would be useful at some stage, it's quite messy and will be difficult to maintain.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added the new sitekey gen function (copied from civicrm_setup as suggested) - I should have commited the suggestion above but didn't see that button until afterwards.

I've run tests without the parameter, with the parameter but no value and with the parameter and a value and they all work as expected.

@totten
Copy link
Member

totten commented Aug 21, 2019

@tabroughton - Thanks for the update. Just an FYI, the build-bot (Jenkins) needed a little nudge to run the tests since you're relatively new 'round here. You can now drill down on the red "X" for details. It hit a couple of small style issues that should be updated. (The buildbot will run again if you push an update, and it should run automatically on any future PRs.)

@tabroughton
Copy link
Contributor Author

tabroughton commented Aug 21, 2019

@totten The last report wasn't accurate, jenkins reported white pace on line 512, it was actually on 514. is this an issue that might need to be looked into? (I think it got me before too 2 commits ago).

Also have now enabled php-enable-drupal-coding-style in emacs so I hope that helps, any links to other tools to check style much appreciated.

@totten
Copy link
Member

totten commented Aug 21, 2019

The last report wasn't accurate, jenkins reported white pace on line 512, it was actually on 514...

Ah, there's been some unrelated-but-concurrent work on the same file in another recent PR. When the buildbot runs, it does a tentative/offline merge of the base-branch and PR-branch before checking anything. Seems like that might explain some shifted line numbers?

Also have now enabled php-enable-drupal-coding-style in emacs so I hope that helps, any links to other tools to check style much appreciated.

Good call there. Civi's code-style standard is a relaxed version of Drupal's code-style. If it passes a Drupal-style checker, then it should pass Civi's.

There may be subtle differences in how various editors/IDEs/presets enforce the style. To reproduce the buildbot results precisely, grab https://github.com/civicrm/coder/ (a small fork of https://www.drupal.org/project/coder) and call phpcs:

phpcs --standard=/path/to/civicrm/coder/coder_sniffer/Drupal file1.php file2.php...

(Most strictly speaking, the bot uses buildkit's civilint (code), but getting that may be a bigger change in workflow, and it's just a wrapper for phpcs --standard=...)

@seamuslee001
Copy link
Contributor

Jenkins re test this please

2 similar comments
@seamuslee001
Copy link
Contributor

Jenkins re test this please

@seamuslee001
Copy link
Contributor

Jenkins re test this please

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants