Make WordPress Core

Opened 2 years ago

Closed 6 months ago

Last modified 6 months ago

#57262 closed defect (bug) (duplicate)

force_ssl_admin can be set to return a string

Reported by: pbearne's profile pbearne Owned by:
Milestone: 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 (5)

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


2 years ago
#1

  • Keywords has-patch has-unit-tests added

#2 @costdev
2 years 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
2 years 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)

Version 30, edited 2 years ago by autotutorial (previous) (next) (diff)

#4 @swissspidy
6 months ago

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

Closing as a duplicate of #60023 which aims to force a bool return type.

Note: See TracTickets for help on using tickets.