#57262 closed defect (bug) (duplicate)
force_ssl_admin can be set to return a string
Reported by: |
|
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
@
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
@
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)
#4
@
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.
@swissspidy commented on PR #3719:
6 months ago
#5
Closing in favor of https://github.com/WordPress/wordpress-develop/pull/7624
Trac ticket: https://core.trac.wordpress.org/ticket/57262