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

$config['flood_cache'] undefined by default, causes error on config page in PHP 8.x #525

Closed
discomrade opened this issue Jan 11, 2023 · 4 comments · Fixed by #526
Closed

Comments

@discomrade
Copy link

The flood_cache config was introduced in this 2013 commit: f309e4037c and has been unchanged since.

I believe was is left commented and undefined intentionally. This is the only place it is used, where it changes code behavior if it has been set:

vichan/inc/filters.php

Lines 191 to 198 in fa198e2

// Determine how long we need to keep a cache of posts for flood prevention. Unfortunately, it is not
// aware of flood filters in other board configurations. You can solve this problem by settings the
// config variable $config['flood_cache'] (seconds).
if (isset($config['flood_cache'])) {
$max_time = &$config['flood_cache'];
} else {
$max_time = 0;

However, newer PHP versions will raise an error if this config option is undefined, because this line doesn't check if the value exists before assigning it:
$c = @$config[$var['name']];

I'd like a second opinion on the best way to solve this.

  • Is it valid to have an unset config variable and just check before use?
  • Should it have a default value of 0 [disabled] to avoid creating a second config variable?
@ctrlcctrlv
Copy link
Member

if this is now invalid you'll want to change its behavior yes. e.g. you can set it by default to -1.

@perdedora
Copy link
Member

You can fix using:

$c = array_key_exists($var['name'], $config) ? $config[$var['name']] : null;

@discomrade
Copy link
Author

discomrade commented Jan 12, 2023

You can fix using:

$c = array_key_exists($var['name'], $config) ? $config[$var['name']] : null;

I was thinking of doing a check similar to that, although it may be more useful to keep it how it is and have the code throw an error when it's trying to read a config key which doesn't exist. flood_cache is the only one, I think the config is the issue rather than the reader.

I think a designated disable value of -1 is the right way to do this

discomrade added a commit to discomrade/vichan that referenced this issue Jan 12, 2023
The original code left flood_cache undefined, leading to errors in newer versions of PHP. See vichan-devel#525
@ctrlcctrlv
Copy link
Member

I suggested -1 because it's a horrible code smell to have config keys which cause different behavior when set vs unset, all config keys should be required to have some value. E.g. in Rust most of the time configuration is a struct so removing keys isn't even possible.

Also I really think my role with this project is just teaching people how to code better these days lol. So I just always suggest the best thing to do not necessarily easiest.

ctrlcctrlv pushed a commit that referenced this issue Jan 12, 2023
The original code left flood_cache undefined, leading to errors in newer versions of PHP. See #525
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 a pull request may close this issue.

3 participants