Inverted logic and noblah-style variables
-
Just as an example of what sort of confusion this causes: https://redmine.pfsense.org/issues/4640
These things are evil and scattered across a couple of places in pfsense. Beyond the noshuntlaninterfaces linked above, I found these in my configuration backups (pretty sure the list is incomplete).
- nodnsrebindcheck
- noantilockout
- nologbogons
- nologprivatenets
- no_tun_ipv6
It makes work harder for the users, since they need to dig into the notes next to the checkboxes to check whether the checkbox actually does what they want or does the exact opposite, since the "descriptions" are actually completely non-descriptive (like for the DNS Rebind Check or Anti-lockout).
It causes bugs in code logic as linked above.
It makes things more difficult and error-prone in general, not easier.
It makes changing defaults more difficult.
Please, avoid this. This in fact should be a policy.
-
Just as an example of what sort of confusion this causes: https://redmine.pfsense.org/issues/4640
Meanwhile, this bug has been "fixed" in a way that the checkbox does the exact opposite of what's described in the GUI. Sigh… ::)
-
But at least the setting does not flip every time save is pressed :P
@cmb made the change of config variable name, I'm sure he will get to look at the implementation end and sort that out.Sometimes the negative names are needed - the default is that the system does "something or other" and an option to "not do something or other" is needed. In most cases the word "disable" can be easier to understand than "no" - "disabledhcpserver", "disableinterface", "disablethingammyjig" - maybe that would help the code to be read and understood.
-
But at least the setting does not flip every time save is pressed :P
@cmb made the change of config variable name, I'm sure he will get to look at the implementation end and sort that out.Well, the code was correct until the last couple of commits for the previous ticket, where someone thought something is enabled by default when it's clearly not and never was.
-
I'm no fan of that style of variable/setting either, I'd love to see those get cleaned up. I'd also like to see the configuration values be explicit rather than assumed, like:
<do_stuff>enabled</do_stuff>
or
<do_stuff>disabled</do_stuff>Or yes/no, or on/off, Rather than assume one way or the other based on the presence of an empty tag.
The other way may save a little space in the config but it can get confusing fast.
Having all settings be affirmative is also a good goal, so there wouldn't be any "check this box to disable this behavior" – For that to be more effective visually, it may require changing to slider-style controls rather than checkboxes.
-
I'm no fan of that style of variable/setting either, I'd love to see those get cleaned up.
Indeed, some of the inconsistency there drives me nuts. In this case, I added it as <noshuntlaninterfaces>so it doesn't have to exist in the default config, and config upgrade code is unnecessary. That's been the default behavior from the very first release in 2004 up until 2.2.0. 2.2.2 brought that back, though with an issue with the checkbox that's since resolved. I think it might be best to not add config settings for defaults, similar to what IOS and its workalikes show for their "show run" output. Granted that's much different since you're routinely looking through "show run" output, and people don't often get into their config.xml.
It's something we'll have to discuss further and standardize for v3.0.</noshuntlaninterfaces>
-
@cmb:
Indeed, some of the inconsistency there drives me nuts. In this case, I added it as <noshuntlaninterfaces>so it doesn't have to exist in the default config, and config upgrade code is unnecessary. That's been the default behavior from the very first release in 2004 up until 2.2.0. 2.2.2 brought that back, though with an issue with the checkbox that's since resolved.</noshuntlaninterfaces>
That did not work - The reversed logic is evil.
-
I guess behind this was the need or the PHP code to handle properly the case when the value is not present in the config. There are many values like this which are assumed to some default in the php code in the case the definition is missing from the XML. That requirement makes indeed the programmer to use inverted logic variables. Thank God the inverted logic is suggested by the variable name…
I've seen cases where the PHP code explicitly deletes (unsets) the value from the config file for the negative value, and sets it to "true" in the config file for the positive value. There's no possibilty to ever see "false" in the XML config file for that variable, although the parsing code handles that too.
