CRM-18231 Environment variables #8724

Edzelopez · 2016-07-19T11:35:11Z

CRM-18231: Support safe migration from production to non-production instances

Edzelopez · 2016-07-21T12:05:21Z

jenkins, retest this please

Edzelopez · 2016-07-22T11:27:49Z

Could one of the admins please verify this functionality?

JoeMurray · 2016-07-22T14:35:58Z

@pradpnayak please test this. We should when ready close and resubmit a cleaner one rather than squashing. This should go in for 4.7.11

seamuslee001 · 2016-07-24T05:15:50Z

CRM/Upgrade/Incremental/sql/4.7.10.mysql.tpl

@@ -21,3 +22,14 @@ INSERT INTO
   `civicrm_option_value` (`option_group_id`, {localize field='label'}label{/localize}, `value`, `name`, `grouping`, `filter`, `is_default`, `weight`, `is_optgroup`, `is_reserved`, `is_active`)
 VALUES
   (@option_group_id_ext, {localize}'{ts escape="sql"}odt{/ts}'{/localize}, @option_group_id_ext_val+1, 'odt', NULL, 0, 0, @option_group_id_ext_wt+1, 0, 1, 1);
+
+-- CRM-18231


This will need to move to 4.7.11 as RC for 4.7.10 has been cut

seamuslee001 · 2016-07-24T06:36:06Z

Also bin/regen.sh needs to be run to update civicrm_generated.mysql

monishdeb · 2017-04-28T17:33:11Z

PR commits has been squashed into one
upgrade code is shifted from 4.7.10 to 4.7.20 sql script
included regen changes

colemanw · 2017-06-26T21:20:45Z

Sorry @monishdeb this slipped through the cracks and it needs rebasing again. I'll get it reviewed/merged this cycle. Thanks.

monishdeb · 2017-07-04T12:44:36Z

@colemanw I have rebased it!

eileenmcnaughton · 2017-08-29T02:26:51Z

CRM/Admin/Form/Setting.php

-          $options = civicrm_api3('Setting', 'getoptions', array(
-            'field' => $setting,
-          ));
+          if (array_key_exists('optionGroupName', $props['pseudoconstant'])) {


getoptions should do this - & if it doesn't get label then I feel like we should address that (possibly as an option on the call)

(although you could probably settle for name for this PR & punt that)

eileenmcnaughton · 2017-08-29T02:27:59Z

CRM/Admin/Form/Setting/Debugging.php

+  public function postProcess() {
+    $params = $this->controller->exportValues($this->_name);
+
+    if ($params['environment'] != 'Production') {


I think this should be an ontoggle function in the definition of the setting

eileenmcnaughton · 2017-08-29T02:31:07Z

@colemanw this slipped through the cracks again. I did a quick read & this seems superficially good. I think it got stalled on whether it could be perfect. I made a couple of comments & it needs re-basing but I think it would be good to give a clear steer on what would get this over the line.

I also wonder if this could be seen as experimental at first, since it is not expected to affect production environments but I suspect once merged people will have some ideas to change it.

pinging @xurizaemon for input

colemanw · 2017-08-29T03:04:04Z

CRM/Admin/Form/Setting.php

+          if (defined('CIVICRM_ENVIRONMENT')) {
+            $element->freeze();
+            CRM_Core_Session::setStatus(ts('The environment settings have been disabled because it has been overridden in the settings file.'), ts('Environment settings'), 'info');
+          }


This looks wrong to me. This function is a generic form generator (in a class extended by 14 child classes) and you're making the assumption that all select elements are the environment setting.

eileenmcnaughton · 2017-08-29T03:21:07Z

@colemanw I see you picked up something that needs fixing - what's your big picture on this pr?

monishdeb · 2017-08-29T07:50:18Z

api/v3/Setting.php

+      CRM_Core_OptionGroup::values($pseudoconstant['optionGroupName'], FALSE, FALSE, TRUE),
+      $params, 'Setting', 'getoptions'
+    );
+  }


@eileenmcnaughton reason to add this code is we cannot call CRM_Core_Pseudoconstant::get(..) OR buildOptions here as it needs DAO/BAO name to be passed either as parameter or class respectively as stated above at line 156.

monishdeb · 2017-08-29T07:51:48Z

@eileenmcnaughton @colemanw I have made additional fixes as per comment and rebased the PR as it gone stale. Can you please check now?

eileenmcnaughton · 2017-08-29T07:51:54Z

@colemanw I think the latest fixes address the issues raised so far so what do we still need to consider?

eileenmcnaughton · 2017-08-29T11:02:52Z

@monishdeb test fail seems related

monishdeb · 2017-08-29T12:02:45Z

@eileenmcnaughton yup fixed it.

colemanw · 2017-08-29T16:41:10Z

CRM/Admin/Form/Setting.php

+          if ($setting == 'environment' && defined('CIVICRM_ENVIRONMENT')) {
+            $element->freeze();
+            CRM_Core_Session::setStatus(ts('The environment settings have been disabled because it has been overridden in the settings file.'), ts('Environment settings'), 'info');
+          }


I really think this code belongs in theCRM_Admin_Form_Setting_Debugging class. After it calls parent::buildQuickform() it can do something like:

if (defined('CIVICRM_ENVIRONMENT')) { $element = $this->getElement('environment'); $element->freeze(); CRM_Core_Session::setStatus(ts('The environment settings have been disabled because it has been overridden in the settings file.'), ts('Environment settings'), 'info'); }

colemanw · 2017-08-29T16:44:04Z

@eileenmcnaughton this looks good to me.
@monishdeb can you please make that last change & then we're good to merge.

colemanw · 2017-08-29T17:37:44Z

Not sure why a bunch of mailing tests just failed.
@civicrm-builder retest this please.

monishdeb · 2017-08-29T17:40:52Z

Yeah, annoying test failures, that runs clean on local :(

colemanw · 2017-08-29T20:52:10Z

Oh I see why they are related:

Failure in api call for job process_mailing: Job has not been executed as it is a Development (non-production) environment.

monishdeb · 2017-08-29T21:10:26Z

Yup, reason why I set the environment setting to Production here But still its failing only on Jenkins not on local. Need to dig further to fix those test failures.

eileenmcnaughton · 2017-08-31T21:54:24Z

Jenkins is still grumpy

seamuslee001 · 2017-09-02T22:11:07Z

@eileenmcnaughton i note that Jenkins appears to now be happy with this PR

eileenmcnaughton · 2017-09-03T21:42:34Z

Looks like @colemanw said this was ready to merge with one change, now made, so merging - thanks @seamuslee001

totten · 2017-09-03T22:54:38Z

tests/phpunit/CRM/Core/BAO/SettingTest.php

+    $values = CRM_Core_BAO_Setting::getItem(CRM_Core_BAO_Setting::DEVELOPER_PREFERENCES_NAME, 'environment');
+    $this->assertEquals('Staging', $values);
+
+    define('CIVICRM_ENVIRONMENT', 'Development');


(a) It's not safe for tests to call define() (at least, given the standard headless test environment). Trivially, suppose this test runs twice in a row. The second time, it will fail. Less trivially, it makes the test environment harder to predict. Compare:

When you run CRM_AllTests, any earlier tests (like CRM/Activity/**Test.php) run in an open-ended environment. (The default appears to be Production.)

When you run CRM_AllTests, any later tests (like CRM/Utils/**Test.php) are unalterably pegged to the Development environment.

When you run any test by itself (like CRM/Utils/**Test.php), you again have an open-ended environment (where the default is Production).

Consequently, tests will some of the time run in Production and some of the time in Development.

(Edit: Also, if the dev machine has its own define(), then this test will crash on trying to modify it.)

(b) Does it actually help to provide special handling for this setting (define(), CRM_Core_Config::environment()) rather than vanilla settings ($civicrm_setitings, Civi::settings()->get('environment'))?

For example, if the reason for the define() is that you want to be able to hard-code a value in civicrm.settings.php... then that's already handled by setting $civicrm_settings['domain']['environment']. (Note: Recall that in 4.7 you don't need to index by the DEVELOPER_PREFERENCES_NAME or whatever -- they're all just aliases for domain.)

On the other hand, if the setting is boot critical (meaning that it must be available as part of the early initialization of the settings/extensions/container system), then maybe it is needed. (But I'd really curious to know why it's boot-critical -- nothing pops while reading.)

My immediate thinking here is that what the test here is trying to prove is that if there is a value set in civicrm.settings.php that it overrides everything else. so you have a method set using essentially Civi::settings()->set to set the value to staging but then someone is then overriding that by setting Development in civicrm.settings.php However i'm thinking that the style at https://github.com/civicrm/civicrm-core/pull/8724/files#diff-d392832fe610e31e06039270b3a100e6R546 seems to be more robust way of testing civicrm.settings.php

@seamuslee001 check Tim's comment about getMandatory - it makes me happy to see that

eileenmcnaughton · 2017-09-03T23:07:33Z

ouch @monishdeb @Edzelopez can you look at @totten's comments & submit a follow up?

@totten are your concerns serious enough for us to revert if not resolved by the time the rc is released?

eileenmcnaughton · 2017-09-03T23:28:24Z

Also re-reading - why are we adding the define in at all? ie. you can override settings in an unover-ridable way in the settings file currently - but perhaps we don't have a method to detect that?

totten · 2017-09-03T23:56:04Z

For determining if the setting has been overridden, I think one can change the test:

if (defined('CIVICRM_ENVIRONMENT') ...

if (Civi::settings()->getMandatory('environment') !== NULL) ...

eileenmcnaughton · 2017-09-04T00:04:04Z

nice!

MegaphoneJon · 2017-12-15T18:45:09Z

Note that this PR introduced a regression, see here:
#11417

civicrm-builder added the master label Jul 19, 2016

Edzelopez force-pushed the CRM-18231 branch from 4804d27 to a458416 Compare July 21, 2016 11:17

Edzelopez changed the title ~~[DO NOT MERGE - Submitted to facilitate testing] CRM-18231 Environment variables~~ [ready for review by core team] CRM-18231 Environment variables Jul 21, 2016

seamuslee001 reviewed Jul 24, 2016
View reviewed changes

pradpnayak force-pushed the CRM-18231 branch from 901b08f to ac0b995 Compare March 26, 2017 22:36

monishdeb force-pushed the CRM-18231 branch 4 times, most recently from c6c1db3 to 89d6fd1 Compare April 28, 2017 17:29

eileenmcnaughton changed the title ~~[ready for review by core team] CRM-18231 Environment variables~~ [ready for review] CRM-18231 Environment variables May 3, 2017

monishdeb force-pushed the CRM-18231 branch from 89d6fd1 to ebb3e73 Compare July 4, 2017 12:43

eileenmcnaughton reviewed Aug 29, 2017

View reviewed changes

colemanw requested changes Aug 29, 2017

View reviewed changes

monishdeb force-pushed the CRM-18231 branch from ebb3e73 to e439a7c Compare August 29, 2017 05:11

monishdeb reviewed Aug 29, 2017

View reviewed changes

monishdeb force-pushed the CRM-18231 branch from 239ca23 to 9a25285 Compare August 29, 2017 09:56

monishdeb force-pushed the CRM-18231 branch from 9a25285 to 649835d Compare August 29, 2017 12:02

monishdeb force-pushed the CRM-18231 branch 2 times, most recently from 9f42e89 to 90faa8e Compare August 29, 2017 16:34

colemanw reviewed Aug 29, 2017

View reviewed changes

monishdeb force-pushed the CRM-18231 branch from 90faa8e to 376ea64 Compare August 29, 2017 16:49

colemanw approved these changes Aug 29, 2017

View reviewed changes

monishdeb force-pushed the CRM-18231 branch from 376ea64 to f133857 Compare August 29, 2017 19:51

colemanw mentioned this pull request Aug 31, 2017

Integration with drupal/civi environment variables fuzionnz/nz.co.fuzion.environmentindicator#2

Open

Edzelopez and others added 2 commits September 1, 2017 02:04

CRM-18231 Added settings on developer page

f008885

additional fixes

f41c478

monishdeb force-pushed the CRM-18231 branch from f133857 to 98fe9ae Compare August 31, 2017 20:42

monishdeb force-pushed the CRM-18231 branch from 98fe9ae to 0e2538e Compare September 1, 2017 09:34

QA fixes

03c5ceb

monishdeb force-pushed the CRM-18231 branch from 0e2538e to 03c5ceb Compare September 1, 2017 10:50

eileenmcnaughton changed the title ~~[ready for review] CRM-18231 Environment variables~~ CRM-18231 Environment variables Sep 3, 2017

eileenmcnaughton merged commit f7a4b13 into civicrm:master Sep 3, 2017

totten reviewed Sep 3, 2017

View reviewed changes

CRM-18231 Environment variables #8724

CRM-18231 Environment variables #8724

Conversation

Edzelopez commented Jul 19, 2016 • edited by civicrm-builder Loading

Edzelopez commented Jul 21, 2016

Edzelopez commented Jul 22, 2016

JoeMurray commented Jul 22, 2016

Choose a reason for hiding this comment

seamuslee001 commented Jul 24, 2016

monishdeb commented Apr 28, 2017

colemanw commented Jun 26, 2017

monishdeb commented Jul 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eileenmcnaughton commented Aug 29, 2017

Choose a reason for hiding this comment

eileenmcnaughton commented Aug 29, 2017

Choose a reason for hiding this comment

monishdeb commented Aug 29, 2017

eileenmcnaughton commented Aug 29, 2017

eileenmcnaughton commented Aug 29, 2017

monishdeb commented Aug 29, 2017

Choose a reason for hiding this comment

colemanw commented Aug 29, 2017

colemanw commented Aug 29, 2017

monishdeb commented Aug 29, 2017

colemanw commented Aug 29, 2017

monishdeb commented Aug 29, 2017

eileenmcnaughton commented Aug 31, 2017

seamuslee001 commented Sep 2, 2017

eileenmcnaughton commented Sep 3, 2017

totten Sep 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eileenmcnaughton commented Sep 3, 2017

eileenmcnaughton commented Sep 3, 2017

totten commented Sep 3, 2017

eileenmcnaughton commented Sep 4, 2017

MegaphoneJon commented Dec 15, 2017

Edzelopez commented Jul 19, 2016 •

edited by civicrm-builder

Loading

totten Sep 3, 2017 •

edited

Loading