WordPress.org

Make WordPress Core

Opened 13 months ago

Closed 13 months ago

Last modified 9 months ago

#51752 closed enhancement (wontfix)

Enable the static variable in wp_get_environment_type() to be reset

Reported by: stevegrunwell Owned by:
Milestone: Priority: normal
Severity: normal Version: 5.5
Component: Bootstrap/Load Keywords:
Focuses: Cc:

Description

While the static $current_env variable in wp_get_environment_type() is a good choice performance-wise (especially since this generally won't change), it's become a pain-point when writing automated tests.

For example, imagine we have two PHPUnit test classes: StagingTest and ProductionTest, responsible for testing the difference in a plugin's behavior in staging and production environments, respectively:

<?php

/**
 * Tests the behavior in a STAGING environment.
 */
class StagingTest extends TestCase {
    
    public function setUp() {
        putenv('WP_ENVIRONMENT_TYPE=staging');

        // Continue the WordPress bootstrapping.
        parent::setUp();
    }

    /**
     * @test
     */
    public function it_should_put_the_plugin_in_staging_mode() {
        $this->assertTrue( myplugin_is_staging_mode() );
    }
}

/**
 * Tests the behavior in a PRODUCTION environment.
 */
class ProductionTest extends TestCase {
    
    public function setUp() {
        putenv('WP_ENVIRONMENT_TYPE=production');

        // Continue the WordPress bootstrapping.
        parent::setUp();
    }

    /**
     * @test
     */
    public function it_should_not_put_the_plugin_in_staging_mode() {
        $this->assertFalse( myplugin_is_staging_mode() );
    }
}

Unfortunately, one (or both) of these test classes would need to be run in separate PHP processes or risk whichever one ran first being the $current_env for the duration of the test suite.

To get around this, I propose a $use_cache parameter for the function; it would default to true, but a false value would cause $current_env variable to be re-calculated.

Attachments (1)

51752.diff (731 bytes) - added by stevegrunwell 13 months ago.

Download all attachments as: .zip

Change History (5)

#1 @TimothyBlynJacobs
13 months ago

I'm -1 on this for the same reasons as #50896. If the environment type changes during a request, it becomes dangerous to developers to adapt their code based on the environment.

#2 @johnbillion
13 months ago

  • Milestone Awaiting Review deleted
  • Resolution set to wontfix
  • Status changed from new to closed

Yeah I'm -1 on this too. Your myplugin_is_staging_mode() function should receive the environment type as a parameter so it's testable on its own.

function myplugin_is_staging_mode( string $mode ) : bool {
    return 'staging' === $mode;
}

// Testing:
myplugin_is_staging_mode( 'production' );
myplugin_is_staging_mode( 'staging' );

// Actual usage:
myplugin_is_staging_mode( wp_get_environment_type() );

Your current test only tests that the WP_ENVIRONMENT_TYPE environment variable gets passed through to wp_get_environment_type(), which means it's testing WordPress more than your own code.

#3 @stevegrunwell
13 months ago

Apologies that the example provided was overly-simplistic, but the hundreds of tests for the platform-wide plugin I maintain wouldn't fit well here 😅

My concern stems from the fact that once the static variable is set, there's no way to change it without spawning a new PHP process; a small performance gain for production, but a road-block when it comes to testing.

Imagine the following scenario in a plugin:

<?php

if ( 'production' !== wp_get_environment_type() ) {
    add_action( 'admin_notices', 'myplugin_show_non_production_notice' );
}

It would be difficult to test that the action gets hooked in a staging environment if another test had previously called wp_get_environment_type(), as the static variable has polluted the global state. It's the same issue we've been fighting forever with the popularity of the Singleton pattern, only PHP's reflection doesn't allow us to reset static variables in functions.

I definitely understand the concerns posed by @TimothyBlynJacobs and @johnbillion, however, and I see now that adding a bypass parameter isn't the way to go.

WordPress is a tightly-coupled application by nature, which can introduce some challenges when testing. This is one of the things I've been aiming to address with the assertwell/phpunit-global-state Composer package; if anyone comes across this ticket at a later date and is like "well, how the heck do I test that branch, runkit may be your best bet (taking care to restore the original function or risk an even worse headache):

<?php

namespace Tests;

use AssertWell\PHPUnitGlobalState\Functions;
use WP_UnitTestCase;

class MyPluginTest extends WP_UnitTestCase {
        use Functions;

        /**
         * @test
         */
        public function it_should_display_a_notice_if_in_non_production_environments() {
                $this->redefineFunction( 'wp_get_environment_type', function () {
                        return 'staging';
                } );

                ( new MyPlugin() )->add_hooks();

                $this->assertIsNumeric(
                        has_action( 'admin_notices', 'myplugin_show_non_production_notice' ),
                        'Expected a staging notice to be displayed in non-production environments.'
                );
        }

        /**
         * @test
         */
        public function it_should_not_display_a_notice_in_production_environments() {
                $this->redefineFunction( 'wp_get_environment_type', function () {
                        return 'production';
                } );

                ( new MyPlugin() )->add_hooks();

                $this->assertFalse(
                        has_action( 'admin_notices', 'myplugin_show_non_production_notice' ),
                        'The staging notice should not appear in production environments.'
                );
        }
}

This ticket was mentioned in Slack in #core by timothybjacobs. View the logs.


9 months ago

Note: See TracTickets for help on using tickets.