Make WordPress Core

Opened 22 months ago

Closed 18 months ago

Last modified 10 months ago

#55741 closed enhancement (fixed)

Check the value of WP_ENVIRONMENT_TYPE before using it

Reported by: dd32's profile dd32 Owned by: desrosj's profile desrosj
Milestone: 6.1 Priority: low
Severity: trivial Version:
Component: Bootstrap/Load Keywords: has-patch commit
Focuses: Cc:

Description

The constant WP_ENVIRONMENT_TYPE as used by wp_get_environment_type() doesn't validate the value of the constant before overriding the environment value.

This is problematic, especially during unit testing, as it becomes impossible to alter the environment during the unit test run, if the environment is forcefully setting the constant (such as through wp-env).

While it could be possibly beneficial to validate that the value is within the whitelist, at least checking it's non-empty may ease testing for some configurations:

  • load.php

     
    227227        }
    228228
    229229        // Fetch the environment from a constant, this overrides the global system variable.
    230         if ( defined( 'WP_ENVIRONMENT_TYPE' ) ) {
     230        if ( defined( 'WP_ENVIRONMENT_TYPE' ) && WP_ENVIRONMENT_TYPE ) {
    231231                $current_env = WP_ENVIRONMENT_TYPE;
    232232        }

Attachments (3)

55741.diff (1017 bytes) - added by khokansardar 22 months ago.
55741.20220604.1.patch (3.1 KB) - added by ramon fincken 22 months ago.
55741 patch
55741.20220604.2.patch (3.1 KB) - added by ramon fincken 22 months ago.
55741.20220604.2

Download all attachments as: .zip

Change History (14)

#1 @SergeyBiryukov
22 months ago

  • Milestone changed from Awaiting Review to 6.1

This ticket was mentioned in PR #2730 on WordPress/wordpress-develop by itzmekhokan.


22 months ago
#2

  • Keywords has-patch added

As WP_ENVIRONMENT_TYPE variable declaration is a string type, so there must be some condition checking of values as well.

Trac ticket: 55741

@khokansardar
22 months ago

#3 @ramon fincken
22 months ago

Went ahead and removed double code from class-wp-debug-data.php and upgraded the checks.

@ramon fincken
22 months ago

55741 patch

@ramon fincken
22 months ago

55741.20220604.2

#4 @ramon fincken
22 months ago

added patch 2, as the is_string is not needed anymore due to the in_array strict checker

#5 @ramon fincken
22 months ago

About the string strict check. You might add (via filter) a non string environment and if you have defined it at your wp-config.php the check will pass.
This is an edge case however, if you want to cover this in the wp_environment_is_allowed function you will need to add an is_string check

#6 @desrosj
18 months ago

  • Keywords commit added; 2nd-opinion removed
  • Owner set to desrosj
  • Status changed from new to reviewing

Thanks for the patches!

While there is some potential for consolidation and flexibility through new filters, I think that deserves a broader discussion.

I think that 55741.diff is preferred here as it simply fixes the specific issue being reported without additional refactoring.

#7 @desrosj
18 months ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

In 54239:

Bootstrap/Load: Confirm the value of WP_ENVIRONMENT_TYPE before using.

This adjusts the check for the presence of the WP_ENVIRONMENT_TYPE constant to also confirm it is set to a non-empty value before using.

Props dd32, khokansardar, ramon-fincken.
Fixes #55741.

#9 @ramon fincken
18 months ago

Yes this will suffice @desrosj

I went perhaps a bit too far introducing the filter.

Will still love if the if/else/check was just 1 central function as having duplicate code will/might lead to issues later on.

#10 @desrosj
18 months ago

@ramon-fincken I’m still open to exploring it, but think it’s best in a new ticket.

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


10 months ago

Note: See TracTickets for help on using tickets.