flyntwp / flynt

Component based WordPress starter theme, powered by ACF Pro and Timber, optimized for a11y and fast page load results.
https://flyntwp.com
MIT License
734 stars 84 forks source link

fix(EnvironmenVariable): use wp 5.5 core function #298

Closed timohubois closed 4 years ago

timohubois commented 4 years ago

Since the WordPress 5.5 release they added a new function into the core check to get environment variables.

See details here: https://make.wordpress.org/core/2020/07/24/new-wp_get_environment_type-function-in-wordpress-5-5/

domtra commented 4 years ago

hi @pixelsaft , thanks for the pull request. at this point i would integrate the new functionality a bit different, because

  1. flynt might be used with a wp version older than 5.5,
  2. removing the WP_ENV constant would more or less be a breaking change which would result in us having to release a version 2.0.0, which at this point we do not want to do yet.

i would actually not change inc/componentLogServer.php at all. the functions.php i would adjust to:

if (!defined('WP_ENV')) {   
    define('WP_ENV', function_exists('wp_get_environment_type') ? wp_get_environment_type() : 'production');    
} elseif (!defined('WP_ENVIRONMENT_TYPE')) {
   define('WP_ENVIRONMENT_TYPE', WP_ENV);
}

do you agree and care to adjust your pull request?

timohubois commented 4 years ago

Hi @domtra, thanks for the feedback. Your adjustments make sense and we created a new commit.

domtra commented 4 years ago

@pixelsaft , great! could you please double check for linting errors? best run npm run lint locally. i fixed the travis-ci integration. it is now required to pass this check in order to merge something into the master branch. this way we can ensure correct code formatting. thank you!

timohubois commented 4 years ago

@domtra: done. Travis outputs successful and scrutinizer found no issues, now.

domtra commented 4 years ago

thanks for this, @pixelsaft!