Make WordPress Core

Opened 18 months ago

Last modified 17 months ago

#57262 new defect (bug)

force_ssl_admin can be set to return a string

Reported by: pbearne's profile pbearne Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version:
Component: General Keywords: has-patch has-unit-tests dev-feedback
Focuses: Cc:

Description

See these tests

$this->assertTrue( force_ssl_admin( 'a string' ), 'set true' );
$this->assertSame( 'a string', force_ssl_admin(), 'check is still true' );

I will add a check that Bool is passed in not string

Change History (3)

This ticket was mentioned in PR #3719 on WordPress/wordpress-develop by @pbearne.


18 months ago
#1

  • Keywords has-patch has-unit-tests added

#2 @costdev
18 months ago

  • Keywords dev-feedback added

force_ssl_admin( $force = null ) is documented to accept string|bool $force and to return bool.

PR 3719 breaks BC by hard deprecating the string type for $force.

IMO, the only "BC break" as such, should be that undocumented return types are no longer supported.


Currently:

  • If no value is passed, it returns the stored value.
  • If a value is passed, it updates the stored value, and returns the old value.

As the function "Determines whether to force SSL used for the Administration Screens.", it doesn't make sense for it to return anything other than bool, so I'd suggest we don't look at altering documented return types, and instead focus on fixing the code.


From what I can tell at the moment (I'm on mobile - makes it a little difficult), this should be sufficient to ensure the function's signature remains intact, and the return value is of type bool as documented:

<?php
// Change this:
$forced     = $force;

// To this:
$forced     = (bool) $force;

Here's some tests: https://3v4l.org/ml1fn


Adding dev-feedback to gather thoughts on the preferred approach here.

#3 @autotutorial
17 months ago

Returning $forced is useful when using the default parameter $force = null but the first time even if you call it

<?php

define('FORCE_SSL_ADMIN', true );
force_ssl_admin( (bool) FORCE_SSL_ADMIN );
$variable = force_ssl_admin(); // second call for set value true

the value is false regardless if the constant it returns evaluates to true or false. After first call is not setting but getting value. This Makes sense unless you need to toggle between true and false or vice versa for a special, perhaps multisite setup.
Use casting explicit bool into your code

<?php
define('FORCE_SSL_ADMIN', 'false' );
function force_ssl_admin( $force = null ) {
        static $forced = false;

        if ( ! is_null( $force ) ) {
                $old_forced = $forced;
                $forced     = $force; // maybe here $forced = (bool) $force
                return $old_forced;
        }

        return $forced;
}

$test1 = (bool) force_ssl_admin( (bool) FORCE_SSL_ADMIN );
$test2 = (bool) force_ssl_admin();
$test3 = (bool) force_ssl_admin();
$test4 = (bool) force_ssl_admin();

/*Expected Result:
bool(false)
bool(true)
bool(true)
bool(true)
*/
var_dump( $test1, $test2, $test3, $test4 );
?>

I'm with preference to adding explicit bool cast for the six uses of force_ssl_admin in core, feel free to comment ( without BC)
Otherwise, to improve performance, I'd set the default to true and is_null. Although with certificate expiration exceptions (hence short term) the majority is https for modern times. In both cases the manual must clarify if the value is different from the first call a second call is needed to retrieve the expected value.

<?php
function force_ssl_admin( $force = null ) {
        static $forced = true;

        if ( is_null( $force ) ) {
                return $forced;
        }

        $forced = (bool) $force;
        return $forced;
}
?>
Last edited 17 months ago by autotutorial (previous) (diff)
Note: See TracTickets for help on using tickets.