Skip to content
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

Merged
merged 4 commits into from
Nov 21, 2023

Conversation

mkorthof
Copy link
Contributor

@mkorthof mkorthof commented Nov 18, 2023

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:

  • xExtension-CustomJS
  • xExtension-CustomCSS
  • xExtension-ImageProxy

Copy link
Member

@Frenzie Frenzie left a 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;
Copy link
Member

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()?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Equivalent to empty()

@Alkarex
Copy link
Member

Alkarex commented Nov 18, 2023

Looks good 👍🏻
Would you be able to give a quick try to the extensions, to check that are still running without crash?

@mkorthof
Copy link
Contributor Author

Looks good 👍🏻 Would you be able to give a quick try to the extensions, to check that are still running without crash?

I'm actually using CustomJS myself so that got the most testing :) It's been working fine so far.
Also enabled the other extensions and tested changing settings of the ones that can be configured (CustomJS, CustomCSS, ImageProxy).

No errors in apache or frss logs and no other issues afaict. Using latest docker image (e4e587aeecf5).

@mkorthof
Copy link
Contributor Author

Did quick test with with Dockerfile-Oldest, no issues.

@Alkarex
Copy link
Member

Alkarex commented Nov 21, 2023

Thanks for the tests 👍🏻

@Alkarex Alkarex merged commit ce44911 into FreshRSS:master Nov 21, 2023
pfactum added a commit to pfactum/freshrss-greader-redate that referenced this pull request Dec 24, 2023
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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants