FIX stabilise typed APIs #10740
FIX stabilise typed APIs #10740
Conversation
src/Security/RememberLoginHash.php
Outdated
| @@ -102,7 +102,7 @@ public static function getLogoutAcrossDevices(): bool | |||
| if (is_bool(static::$logoutAcrossDevices)) { | |||
| return static::$logoutAcrossDevices; | |||
| } | |||
| return static::config()->get('logout_across_devices'); | |||
| return (bool)static::config()->get('logout_across_devices'); | |||
There was a problem hiding this comment.
I assume this one is because the return type is explicit, so this protects against null?
Is this ever hit before cache is generated?
There was a problem hiding this comment.
I haven't come across that in this case, but I feel it's kinda irrelevant. Good practice and clean, stable code is the objective here.
src/ORM/DataObject.php
Outdated
| return $this->config()->get('general_search_field_name'); | ||
| $configSetting = $this->config()->get('general_search_field_name'); | ||
| return is_string($configSetting) ? $configSetting : self::$general_search_field_name; |
There was a problem hiding this comment.
Is this ever hit before cache is generated? If not I'd be inclined to leave it as is.
There was a problem hiding this comment.
I'm inclined to disagree on principle.
Or, if it is to stay 'as is' then the return type should be dropped.
The change is small, does no harm, and enforces the return type.
I guess it could lead to an obscure error later in execution by silently ignoring a bad config setting, perhaps throwing an error would be better?
There was a problem hiding this comment.
PHP itself will already throw an error if the type is wrong, which will tell developers they've set the config to an invalid value.
My hesitance here comes from the fact that we're not really doing this anywhere else, except perhaps in places where different value types are expected and need to be handled.
This seems like the sort of thing we should have a considered standard approach for, and have an issue to handle these across the board in a uniform way.
As it stands, I'd prefer this pr to only include the changes for the same site issue -but if you want to keep them here it's be good to have another opinion or two.
There was a problem hiding this comment.
I can remove the non samesite things, but since it's not super urgent lets get some other opinions in.
I agree, I think there should be a standard approach - no points awarded for guessing my position :P
There was a problem hiding this comment.
Please remove the non samesite things, it's not related to the issue being fixed and just makes for muddled PRs
src/ORM/DataObject.php
Outdated
| return $this->config()->get('general_search_field_name'); | ||
| $configSetting = $this->config()->get('general_search_field_name'); | ||
| return is_string($configSetting) ? $configSetting : self::$general_search_field_name; |
There was a problem hiding this comment.
Please remove the non samesite things, it's not related to the issue being fixed and just makes for muddled PRs
src/Security/RememberLoginHash.php
Outdated
| @@ -102,7 +102,7 @@ public static function getLogoutAcrossDevices(): bool | |||
| if (is_bool(static::$logoutAcrossDevices)) { | |||
| return static::$logoutAcrossDevices; | |||
| } | |||
| return static::config()->get('logout_across_devices'); | |||
| return (bool)static::config()->get('logout_across_devices'); | |||
There was a problem hiding this comment.
Please don't add code unrelated to the issue being fixed, it's just a distraction that slows downs the pull-request process.
Any sort of attempt to globally enforce type hinting should be handled in an different issue
Since 4.12 the use of typehints and return types has caused issues with values fetched directly from config without validation. This has lead to upgrade woes in a minor version (silverstripe#10721) with no immediate recourse other than manual system intervention. To use types, we should ensure types, leaving a stable API that won't error on a bad value - or should give a thoughtful and directive error message if so. Issue silverstripe#10721 summary: SessionMiddleware runs before FlushMiddleware SessionMiddleware causes a PHP fatal error passing `null` to a `string` parameter. `null` comes from config, because default string value doesn't exist. We need flush for this - but system execution never makes it that far.
NightJar
force-pushed
the
pull/4.12/compatible-types
branch
from
cc728ab
to
f268692
Compare
Since 4.12 the use of typehints and return types has caused issues with values fetched directly from config without validation. This has lead to upgrade woes in a minor version with no immediate recourse other than manual system intervention.
To use types, we should ensure types, leaving a stable API that won't error on a bad value - or should give a thoughtful and directive error message if so.
resolves #10721