Use the environment setting instead of checking for a git directory

[mattwire]

Overview

Environment can be one of "Production", "Staging" or "Development". The isDevelopment() function currently controls whether to enable MYSQL strict mode and whether to log deprecation notices.

Before

Check for presence of .svn or .git directory in CiviCRM root.

After

Check if the value of environment is set to Development.

Technical Details

We don't need to cache because it's cached by SettingsBag.

Comments

[civibot]

(Standard links)

• If this is your first pull-request for CiviCRM, please browse CONTRIBUTING.md for information about the development and testing processes.
• If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

[demeritcowboy]

Since the test sites and the test runs have this set to Production, then if the check for .git is going to change then I think the test site build scripts should set it to Development. I haven't checked but I think buildkit also leaves it at Production, so this might change what developers see too if they don't change that setting.

[mattwire]

@totten It would be useful to get your thoughts here? Checking for the presence of .git/.svn doesn't seem a very reliable way of checking if we are in development mode especially if the live site is deployed via git. Given that we have the UI setting now for Production/Staging/Development it makes sense to use it?

[adixon]

Great, thanks for this. Obviously that's pretty old code, and I support your modernization efforts!

I believe there still might be some distinction between the civi 'setting' of prod/stage/devel vs. what we might want to get from this function. My understanding of those settings is that they will configure whether your site is in production or not, which might be a different concept from whether errors should be emitted or not, particular in the case of automated testing e.g, you may be turning things off when not in production that do need to be tested.

Would it be a terrible idea to have a separate flag that would determine whether to emit warnings and or enable mysql strict mode? Something you can just turn off and on separately in your civicrm.settings.php file maybe?

Agree that @totten master of automated testing would probably have some thoughts. Keeping him to a yes/no is a challenge of course ...

[adixon]

Looks to be used only as advertised, at least in core. History of the isDevelopment function shows it to be imported from svn 7 years ago. I think it's worth a more aggressive cleanup ...

$grep -R isDevelopment *
api/v3/System.php:        'dev' => (bool) CRM_Utils_System::isDevelopment(),
CRM/Core/Error/Log.php:      if (CRM_Utils_System::isDevelopment() && CRM_Utils_Array::value('civi.tag', $context) === 'deprecated') {
CRM/Core/DAO.php:    if (CRM_Utils_Constant::value('CIVICRM_MYSQL_STRICT', CRM_Utils_System::isDevelopment())) {
CRM/Utils/File.php:    if (CRM_Utils_Constant::value('CIVICRM_MYSQL_STRICT', CRM_Utils_System::isDevelopment())) {
CRM/Utils/System.php:  public static function isDevelopment() {

[adixon]

Here's an alternative proposal - let's just deprecate the badly named and executed isDevelopment function, and use some kind of isTesting function in place of it's current usage instead. I'll guess that @totten already has such a function somewhere.

The usage and execution of the function really is not related to whether a site is in development or not, it's about whether it's running tests, for the purposes of emitting warnings and for testing mysql strict mode.

[homotechsual]

Why not handle this the Symfony way - use APP_ENV and accept prod, test, or dev

[eileenmcnaughton]

We have been removing code after 6 months or so or no-one reporting seeing it because we have a degree of confidence that is long enough for a dev to see a deprecated warning. If we merge this how confident are we that devs will still see it? (I don't personally have development mode currently enabled on any of my dev sites)

If less dev sites are seeing deprecation warnings then we could change the policy to be ??18 months??

Note that production sites should not have E~Deprecated enabled in their php_error settings & generally that has been our advice to sites seeing it.

[eileenmcnaughton]

Actually - looks like

error_reporting(E_ALL & ~E_NOTICE);

would be the most commons setting - so we could tie in with that using

 trigger_error('Use Bar instead', E_USER_NOTICE);

instead of User deprecated to reduce live-site noise

[mattwire]

@eileenmcnaughton The issue here is not so much the configuration of a production site e-notice settings but that the logic in isDevelopment() is flawed. It is perfectly reasonable to have a .git directory on a production site for many deployments. isDevelopment() is called from four places in core at the moment.
For example in one place it controls whether sql queries are run in strict mode or not:

    if (CRM_Utils_Constant::value('CIVICRM_MYSQL_STRICT', CRM_Utils_System::isDevelopment())) {
      $db->query('SET SESSION sql_mode = STRICT_TRANS_TABLES');
    }

Give me a day or so and I'll move this over to lab.civicrm.org - we have two alternative proposals from @adixon and @MikeyMJCO which I think would be worth exploring.

[eileenmcnaughton]

@mattwire OK - cool - the deprecatedError helps us to decruft our codebase so people seeing that 'fairly aggressively' means we can be 'fairly aggressive' about removing code.

The usage you posted above maybe isn't even needed (as long as the test class applies it)

[artfulrobot]

Bah! yeah, I use .git on all my prod sites. Only just realised this had consequences.

[eileenmcnaughton]

Closing this for now as I think the latest discussion was to explore the issue in gitlab first