-
Notifications
You must be signed in to change notification settings - Fork 65
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make extensions use typed methods Minz_Request::paramString and Minz_Session::paramString #181
Conversation
…inz_Session::paramString instead
…inz_Request::paramString instead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fine by me in principle, though I'm a little worried about the addition of strict_types changing behavior? I'd rather have seen that in a separate PR since the param stuff is insta-mergeable. ;-)
FreshRSS_Context::$user_conf->image_proxy_scheme_default = Minz_Request::param('image_proxy_scheme_default', self::SCHEME_DEFAULT); | ||
FreshRSS_Context::$user_conf->image_proxy_scheme_include = Minz_Request::param('image_proxy_scheme_include', ''); | ||
FreshRSS_Context::$user_conf->image_proxy_url_encode = Minz_Request::param('image_proxy_url_encode', ''); | ||
FreshRSS_Context::$user_conf->image_proxy_url = Minz_Request::paramString('image_proxy_url', true) ?: self::PROXY_URL; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is that null coalescing parameter thing equivalent to isset()?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Equivalent to empty()
Looks good 👍🏻 |
I'm actually using CustomJS myself so that got the most testing :) It's been working fine so far. No errors in apache or frss logs and no other issues afaict. Using latest docker image (e4e587aeecf5). |
Did quick test with with Dockerfile-Oldest, no issues. |
Thanks for the tests 👍🏻 |
Follow FreshRSS/Extensions#181 and use `Minz_Request::paramBoolean()` instead of `Minz_Request::param()` + typecasting. Fixes: javerous#6 Signed-off-by: Oleksandr Natalenko <[email protected]>
As per short discussion in PR #5830:
FreshRSS/FreshRSS#5830 (comment)
FreshRSS/FreshRSS#5830 (comment)
Tested the 3 extensions with actual changes in docker container running
d2aef84827ee freshrss/freshrss:edge
: