Make WordPress Core

Opened 4 years ago

Closed 4 years ago

Last modified 3 years ago

#50896 closed enhancement (wontfix)

Add filters to wp_get_environment_type()

Reported by: crstauf's profile crstauf Owned by:
Milestone: Priority: normal
Severity: normal Version: 5.5
Component: Bootstrap/Load Keywords: has-patch
Focuses: Cc:

Description (last modified by SergeyBiryukov)

When first introduced and described on the Make blog by Yoast, the function contained filters. These filters were removed in [48662] because "the function is called too early for any plugins to properly hook into these filters".

The only _current_ use of wp_get_environment_type() by core is for setting the WP_DEBUG constant when in a development environment. While it's true that filters won't work in that scenario, the filters would be useful for themes and plugins.

In my case, I want to set the default environment to "development": while there's only one production environment, there are multiple development environments that can be created at any time. Setting the environment default to "development" means that whenever a new development environment is created, the code runs in its safest mode from the start, even without explicitly defining WP_ENVIRONMENT_TYPE constant.

Reverting [48662] would close this ticket.

Change History (19)

#1 in reply to: ↑ description ; follow-up: @SergeyBiryukov
4 years ago

  • Description modified (diff)

Hi there, welcome back to WordPress Trac! Thanks for the ticket.

When first introduced and described on the Make blog by Yoast, the function contained filters. These filters were removed in [48662] because "the function is called too early for any plugins to properly hook into these filters".
...
Reverting [48662] would close this ticket.

Just noting that simply reverting [48662] won't work as expected. As previously noted in comment:58:ticket:33161, the result of the function is cached into a static variable (per comment:44:ticket:33161) on the first run (which is too early for filters), and cannot be changed later.

So we'll also have to remove the static variable, which may lead to issues, as noted in comment:44:ticket:33161.

Last edited 4 years ago by SergeyBiryukov (previous) (diff)

#2 in reply to: ↑ 1 @crstauf
4 years ago

Replying to SergeyBiryukov:

Just noting that simply reverting [48662] won't work as expected. As previously noted in comment:58:ticket:33161, the result of the function is cached into a static variable (per comment:44:ticket:33161) on the first run (which is too early for filters), and cannot be changed later.

So we'll also have to remove the static variable, which may lead to issues, as noted in comment:44:ticket:33161.

Oh sweet, I like the use of the static variable; definitely don't think that should be removed. It does create a problem though if someone wants to change the default environment type.

Working on a possible solution; the following described approaches do not seem to work:

  1. Applying a filter to the static variable defeats the whole purpose of the static variable, so that's out.
  1. Defining a constant (ex: WP_DEFAULT_ENVIRONMENT_TYPE) creates the same problem.

#3 @crstauf
4 years ago

A simple description of the solution:

The returned value must not be permanently set for the current request until after all mu-plugins have been loaded.

#4 follow-up: @Clorith
4 years ago

I'm not sure I get the underlying problem here, wouldn't it be _easier_ to define the WP_ENVIRONMENT constant on your development site, than implementing a filter?

#5 follow-up: @crstauf
4 years ago

Here's what I'm working with at the moment:

<?php
function wp_get_environment_type() {
        static $current_env = '', $filtered_env = '';

        if ( $filtered_env ) {
                return $filtered_env;
        }

        if ( did_action( 'muplugins_loaded' ) ) {
                $filtered_env = apply_filters( 'wp_get_environment_type', $current_env );
                return $filtered_env;
        }

        if ( $current_env ) {
                return $current_env;
        }

        ...
}

IMO the ideal solution does not require two static variables (what I'm working on currently), but a solution that does not use two static variables is (feeling) unlikely to be simple or elegant.

#6 in reply to: ↑ 4 @crstauf
4 years ago

Replying to Clorith:

I'm not sure I get the underlying problem here, wouldn't it be _easier_ to define the WP_ENVIRONMENT constant on your development site, than implementing a filter?

@Clorith If there were only one development, staging, and production site, a constant works: set it once and forget about it. However, in a workflow where there can be multiple development sites (think multiple developers creating local instances) absolutely ensuring the creation of a constant in wp-config.php before any request is made to the local instance is (unfortunately) impossible (at least has been in my experience).

The advantage of an mu-plugin is that it would be easily distributed with the codebase (due to its location within wp-content directory), and would thus near-absolutely ensure the environment type of "development".

Version 0, edited 4 years ago by crstauf (next)

#7 @SergeyBiryukov
4 years ago

  • Component changed from General to Bootstrap/Load
  • Description modified (diff)

#8 in reply to: ↑ 5 @crstauf
4 years ago

Replying to crstauf:

a solution that does not use two static variables is (feeling) unlikely to be simple or elegant.

So glad I was wrong; this is much more elegant, though perhaps slightly less performant:

<?php
function wp_get_environment_type() {
        static $cached_env = '';
        
        if ( $cached_env ) {
                return $cached_env;
        }
        
        ...
        
        if ( did_action( 'muplugins_loaded' ) ) {
                $cached_env = apply_filters( 'wp_get_environment_type', $current_env );
                return $cached_env;
        }

        return $current_env;
}

I used $cached_env just to be descriptive; there's probably a more appropriate variable name.

Last edited 4 years ago by crstauf (previous) (diff)

#9 @desrosj
4 years ago

  • Version changed from trunk to 5.5

This ticket was mentioned in PR #474 on WordPress/wordpress-develop by crstauf.


4 years ago
#10

  • Keywords has-patch added

Allow mu-plugins to override the returned value of wp_get_environment_type().

Trac ticket: https://core.trac.wordpress.org/ticket/50896

#11 follow-up: @TimothyBlynJacobs
4 years ago

I’m not sure this is a good idea. AFAICT, this change would make it so that the environment type could be production before mu plugins are completely loaded, but then something completely different after mu plugins are loaded. That is the kind of inconsistency that caching the value was supposed to avoid.

#12 in reply to: ↑ 11 @crstauf
4 years ago

Replying to TimothyBlynJacobs:

this change would make it so that the environment type could be production before mu plugins are completely loaded, but then something completely different after mu plugins are loaded.

@TimothyBlynJacobs You are correct, this would happen. However, core only uses wp_get_environment_type() prior to muplugins_loaded to auto-set the WP_DEBUG constant, which is innocuous (typically production environments explicitly define as false), and the added value of mu-plugins being able to adjust is worth it IMO.

And as I mentioned previously, if an mu-plugin is using wp_get_environment_type() prior to muplugins_loaded action, I assign that responsibility to the site's developer to resolve.

#13 @crstauf
4 years ago

FWIW, I did think of another approach last night: check if WP_DEVELOPMENT_TYPE constant is defined or the environment variable is set, and if not, wp_die() with a message asking for the value to be explicitly set. This is not ideal, but if adding filters into the function is significantly problematic, there is that option.

Here's the mu-plugin if anyone ends up here looking for something similar:

<?php
if ( ! function_exists( 'require_set_environment_type' ) ) {

        /**
         * Prevent site load if environment type not explicitly set.
         *
         * @uses wp_get_environment_type()
         * @uses wp_die() 
         */
        function require_set_environment_type() {
                if ( ! function_exists( 'wp_get_environment_type' ) ) {
                        return;
                }
                        
                $current_env = '';
                
                if ( function_exists( 'getenv' ) ) {
                        $has_env = getenv( 'WP_ENVIRONMENT_TYPE' );
                        if ( false !== $has_env ) {
                                $current_env = $has_env;
                        }
                }

                if ( defined( 'WP_ENVIRONMENT_TYPE' ) ) {
                        $current_env = WP_ENVIRONMENT_TYPE;
                }
                        
                if ( wp_get_environment_type() === $current_env ) {
                        return;
                }
                        
                wp_die( 'Explicitly define a valid environment type to activate the site.<br />See <code>wp_get_environment_type()</code> for more info.' );
                exit;
        }
        
        require_set_environment_type();
        
}

#14 follow-up: @chesio
4 years ago

Environment type should be constant, it simply feels bad to think about it as a filterable value that can potentially change multiple times during single request.

Setting the environment default to "development" means that whenever a new development environment is created, the code runs in its safest mode from the start, even without explicitly defining WP_ENVIRONMENT_TYPE constant.

I would argue that production is the safest mode. I would prefer any of my development websites to accidentally run in production mode than any of my production websites to accidentally run in development mode :-) In production HTTPS is forced, regular code integrity checks are run, assets are optimized etc...

#15 in reply to: ↑ 14 @crstauf
4 years ago

Replying to chesio:

Environment type should be constant, it simply feels bad to think about it as a filterable value that can potentially change multiple times during single request.

@chesio I agree: ideally, the environment type is set once and remains that same value. If it were not for the automatic setting of the WP_DEBUG constant, this could be done: define the WP_ENVIRONMENT_TYPE constant in an mu-plugin, and all's well. However (as I've mentioned previously, so forgive me if you've already read it and I'm now repeating for no value), given that the automatic setting of the WP_DEBUG in a production environment is most often innocuous (defined prior in wp-config.php), allowing the environment type to be filterable only by mu-plugins and then permanently set for the request I think is acceptable.

Replying to chesio:

I would argue that production is the safest mode. I would prefer any of my development websites to accidentally run in production mode than any of my production websites to accidentally run in development mode :-) In production HTTPS is forced, regular code integrity checks are run, assets are optimized etc...

(Again, I've mentioned this previously, just not sure if you've read it; please forgive me if you have.)

You are absolutely right: it is important that production run in production mode. It is equally important that development sites not run production-only code. Consider an API with a request cap or fees, automated sending out of emails, etc. Given that production is special and only one production environment exists, it is very likely that production will have been given special attention and that the WP_ENVIRONMENT_TYPE constant (and other important constants) defined in wp-config.php. Even if production were not special, there is still only one of them, so it's a "set it and forget it" situation. With development sites, there could be dozens (think local instances), and in my experience, very easy to forget to set the environment type before a single request is made to the local environment.

#16 follow-up: @TimothyBlynJacobs
4 years ago

And as I mentioned previously, if an mu-plugin is using wp_get_environment_type() prior to muplugins_loaded action, I assign that responsibility to the site's developer to resolve.

I disagree with this for a couple of reasons.

  1. You also need to consider drop-ins that can make use of that value.
  2. Network activated plugins are loaded before muplugins_loaded is fired which means plugins in general can't rely on it on multisite.
  3. Mu-plugins are often the way that hosting companies provide functionality specific to their host. They wouldn't be able to rely on this value either then.
  4. Mu-plugins aren't just used by highly customized sites with developers where they know how each and every one of them work under the hood. Plugins can make use of mu-plugins as well when they need to be loaded early.

Additionally, while debug mode is the only place Core is currently using the environment early in the load process, that isn't guaranteed into the future. If Core can't rely on that value being constant throughout the request that would hamper future development opportunities.

Lastly, I don't think distributing and mandating an mu-plugin be installed on dev sites is any easier than mandating that a value be set in wp-config.php. Both are susceptible to user error. For this type of site, I think you'd be better off tracking the whole thing in git and having a wp-config-partial.php that defines the environment type by default. Or mandating use of a development environment that would populate WP_ENVIRONMENT_TYPE.

#17 in reply to: ↑ 16 @crstauf
4 years ago

Replying to TimothyBlynJacobs:

Lastly, I don't think distributing and mandating an mu-plugin be installed on dev sites is any easier

Indeed it is easier, and not susceptible to user error: a lot of WordPress sites commit only the wp-content directory, which contains the mu-plugin directory, so any checkout is going to have the mu-plugin which dictates the environment type.

For this type of site, I think you'd be better off tracking the whole thing in git and having a wp-config-partial.php that defines the environment type by default.

This doesn't work, because some local WordPress environment creators (like Local WP) define wp-config.php at environment creation, and so would still need manual intervention.

#18 @crstauf
4 years ago

  • Resolution set to wontfix
  • Status changed from new to closed

I'm closing this ticket, because while I would've liked to be able to have the default environment be "development", there's seemingly not a viable option to make that happen. If wp_get_environment_type() had not been used for defining WP_DEBUG perhaps it would've happened, but I understand the issues mentioned.

Best option now is to require explicit defining of the environment type (source), otherwise keep the site inoperable.

#19 @desrosj
3 years ago

  • Milestone Awaiting Review deleted
Note: See TracTickets for help on using tickets.