force_ssl_admin can be set to return a string

[pbearne]

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

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

[costdev]

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.

[autotutorial]

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;
}
?>