pfSense

No, sorry, sir. We are back to the flip every save. This no logic MUST go away.

Yes, Chris seems to have reversed the fix I made for the "flip every save" problem. The whole thing needs some re-engineering.

Yes. The only way to fix this without reversing the evil logic would be reverting this commit and changing the description to the equally evil "Check this checkbox to disable this" in the GUI.

Can we please revert the broken commit and fix the description until this is recoded properly?

This does NOT work. Keep clicking Save and watch the checkbox and ipsec.conf flip. Not really sure what to say here. Fix the description if unable to recode the double negative bullshit. Sorry but this is getting absurd.

And let me say, it was just fine until this evil commit that twisted the logic into this stupidity.

https://github.com/pfsense/pfsense/commit/0a9e6c85f05b1027156618a9ccf1e1b12f31683e

https://github.com/pfsense/pfsense/pull/1715
Ermal's change/fix seems good (although the whole thing screws with people's brains, but that is another issue). The "aim" is that the user interface checkbox displays the boolean value the opposite way around to what is actually in the $config. That means:
1) Read config into $pconfig array
2) If $pconfig value is false then set the checkbox, otherwise clear it (code by cmb)
3) When processing $POST value on save, if $POST value is "yes" then unset in config, otherwise set in config (Ermal's fix a few hours back)
4) Set the $pconfig value to be the opposite of what came in $POST - otherwise the immediately redisplayed checkbox value is actually the opposite of the way it should be - my pull request now.

This change to address point (4) finishes making the UI processing consistently display the opposite truth value to what is stored in the config. At least now the front end works consistently (even if backwards and you don't like the name 'noshuntlaninterfaces' :)

Now it works consistently, someone can check the back-end implementation and if that does the wrong/opposite thing then that can be fixed in the back-end without needing to touch the front-end UI logic.

Phillip Davis wrote:

https://github.com/pfsense/pfsense/pull/1715

Applied your pull request on top of the whole mess without reverting anything.

- The GUI now really shows what's configured in ipsec.conf, not the opposite.
- Does not flip on save.
- Ticking/unticking the checkbox does enable/disable conn bypasslan as expected.

Thanks, Phil!!!

Kill Bill wrote:

And let me say, it was just fine until this evil commit that twisted the logic into this stupidity.

https://github.com/pfsense/pfsense/commit/0a9e6c85f05b1027156618a9ccf1e1b12f31683e

It was a game for you to test you communication skills.
I thought you have to taste what happens on this side :)

Anyway its fixed now let me know if its still broken.

It works now... (The time wasted here would be enough of a hint to not ever do things like this again. There's a forum thread dedicated to this coding style, so leaving it at that... :-P)

Thanks.

https://www.youtube.com/watch?v=kWmbXMXns28

The fish-slapping drama continues here https://github.com/pfsense/pfsense/pull/1715
That pull request is closed, but so far the changes Ermal has made fix parts of the remaining issue or introduce other side-effects. Let's see how long we can keep this one going - it helps distract all of us from attending to other more worthy bugs :)

I still would love to hear why exactly do we desperately need to spare one line in default config.xml and why the setting needs to be like <noshuntlaninterfaces /> instead of <shuntlaninterfaces>no<shuntlaninterfaces/>

Top secret reasons, sorry. The NSA won't let us tell.

I'm kidding...point being, why would you want additional lines in the default config that are unnecessary? Sure, a single line is no big deal, but it's a matter of principle. If every single line were no big deal, we'd have hundreds of lines for the same reason. Every default config option shouldn't require a config.xml parameter to be set to use the default. The absence of a config.xml setting means it's at its default, not the presence of it.

Chris Buechler wrote:

I'm kidding...point being, why would you want additional lines in the default config that are unnecessary? Sure, a single line is no big deal, but it's a matter of principle. If every single line were no big deal, we'd have hundreds of lines for the same reason. Every default config option shouldn't require a config.xml parameter to be set to use the default. The absence of a config.xml setting means it's at its default, not the presence of it.

Chris, when you no longer have defaults in the config, you don't know what the default is (without digging into the code). You don't even know that such configuration option exists. Having even the defaults in the config.xml makes it self-documenting. You can grep the code and see what that config option does, if non-obvious. You also can change those easily when manually imporing the config.xml on the same/different box or whatever. I cannot see how hundreds of lines do any harm in the config.xml. (The empty tags style does not do much good here either.)

This results in extremely messy code that "screws with people's brains", as Phil noted.

There's a forum thread for this, better move the debate there though. https://forum.pfsense.org/index.php?topic=92631.0

Ermal merge pull 1715, and as far as I can see that resolves the remaining issue.