Simplify php script

[edeckers]

Hi @rbicelli , thank you so much for your work, I really appreciate this template!

Of course, before installing any script on my firewall I want to know exactly what it does; especially when AllowRoot=1 is required. And while studying pfsense_zbx.php I stumbled upon a few bugs and inconsistencies, so I figured I'd squash those and contribute these changes to your project. However... I may have lost track of time and gone a little overboard refactoring, because I enjoyed it: my apologies if that was too forward of me!

There are a lot of changes, all of them backwards compatible. I hope you like it and merge it, but I understand if you won't.

Changes

• Wrap all commands and discoveries in classes and use them instead of switch statements
• Wrap all exec statements in Shell class, so it is easy for users to inspect which commands will possibly be run as root
• Wrap all pfSense methods and consts in PfEnv class, so these 'undefined' symbols are abstracted away from the rest of the code
• Lower method complexity by splitting them up and by keeping nesting of if's, loops, etc. to a minimum
• Avoid variable mutations and other side-effects as much as reasonable
• Replace all manual JSON generation for discovery rules with json_encode
• Re-order code
• Use consistent naming, quotes, etc.
• Move all require statements to top of file
• Replace magic numbers with constants
• Fix get_dhcp method, which was missing some variable declarations

[JSuenram]

AMAZING Update... hopefully it will be accepted....

[rbicelli]

Hi and Thanks! I didn't wrapped in classes since the whole pfsense codebase isn't object oriented. Please give me some time in order to review changes and test them.

[rbicelli]

I appreciate your work but honestly I think if accepting your PR I can no longer be able to maintain the code. I think the way it's written, even if not as modern as php is today, is more readable and maintainable from my (a sysadmin who can code) perspective. Now I'im ready to work on pfsense 2.6 and Zabbix 6 so I'll start to check every PR and prepare the update. I'll try to review the code one more time.

[edeckers]

Alright, I totally understand if and why you won't merge! No hard feelings at all :)

I think the way it's written, even if not as modern as php is today, is more readable and maintainable from my (a sysadmin who can code) perspective

Particular stuff that makes it harder to read for you? I can imagine the array_reduce, array_map et al don't help, as it is quite a diversion from your style. If this is the most important culprit, I can rewrite those to for loops.

[rbicelli]

Particular stuff that makes it harder to read for you? I can imagine the array_reduce, array_map et al don't help, as it is quite a diversion from your style. If this is the most important culprit, I can rewrite those to for loops.

I think if you were able to refactor it like this is because the original code was easy to read and understand, even by a php newbie :-)

These are the points that make it harder to read:

• inline if statements: it's my limit, but hopping from a language to another (C#, php, phython) I prefer the classic syntax, which to me is more understandable
• switch statement removed from main(): leaving the switch statement it's easy to understand which commands and subcommands are implemented, that array_map things force me to search in the code
• arrow functions widely used nested in array_map statements make me scratch my head and again

As I said, it's a personal limit and if this really is an improvement to the project I'll accept the PR and force myself to understand and work with it. Now I'm reading the code again and again and trying to understand it. Maybe in a day or two I'll change my mind.

But I'd also like to hear other contributors impressions.

[edeckers]

As I said, it's a personal limit and if this really is an improvement to the project I'll accept the PR and force myself to understand and work with it. Now I'm reading the code again and again and trying to understand it. Maybe in a day or two I'll change my mind.

Some changes are definitely improvements as I fixed a few bugs along the way for example, but a large part of the changes also follows from my personal preference, so those are not improvements per se; one could even argue they are actually setbacks, since the code is now harder to maintain for you.

I've added two commits since your last comment, one of which replaces all occurrences of array_map with foreach and the second replaces the more complex ternary operator occurrences with explicit if statements.

Do these changes significantly improve the code's readability for you? Because then I'll get rid of the array_reduce as well.

As far as the command pattern goes: I'm not a big fan of huge switch statements, but I also understand your gripe with the commands. I'll see if I can come up with a solution that uses the best of both worlds!

[rbicelli]

Hi! Since this PR has high impact on source what about merging it in another branch?

[edeckers]

Hi @rbicelli , that's fine by me! Although I do wonder: what do you achieve by doing so? If you're not merging this PR to master, my code can just live in the branch where it is now, no? And if you ever do want to merge it, you'll have to solve the conflicts with the master branch either way. Or am I missing the point?

Either way, I'm cool with it :)

[rbicelli]

Hello, I merged the PR then renamed original script to pfsense_zbx_legacy.php. Then I tried yours on a test box (pfsense 2.6). Unfortunately the script isn't working as expected:

• Network interfaces discovery does not produce output
• Service discovery isn't backwards compatible (with combined services like openvpn it doesn't create the instance string correctly).

So I stopped here, swapped files and named yours pfsense_zbx_rc.php. To me is better for testing and when I'm sure your script is working exactly as mine I will swap them.

Thanks :-)

[edeckers]

Thank you for merging this PR!

I just checked and noticed that I'm running into the exact same problem with regards to the interface discovery not producing output, but I failed to realize that earlier because I used the FreeBSD OS network interface discovery rules on my local installation. They look kind of similar.

When I find the time I'll fix it and send you a new PR.

[rbicelli]

You have also to check service discovery (the sprintf instruction must add a . between service name and its instance). I'd like both scripts are interchangeable.