Update haproxy_global.php to fix 14445 #1282
Conversation
In pfSense CE 2.7, PHP has been updated to 8.2.6. This version of PHP is stricter in using variables. This fixes the issue described in 14445 (https://redmine.pfsense.org/issues/14445) by explicitly defining an array before assigning a value to an element of that array.
There was a problem hiding this comment.
There is a better way to reach the goal here, of which you may not be aware. We have recently changed to using new array and config access functions that let us work around these PHP problems in an abstract and safer way.
We have a new development document for them here: https://docs.netgate.com/pfsense/en/latest/development/php-config-arrays.html
If you would like to update or resubmit the PR with that change, feel free, but if you are not able to or do not have the time, I can make the change manually.
Additionally, the Makefile needs to have the PORTREVISION or PORTVERSION increased to trigger a new build/update.
Thanks!
| @@ -135,6 +135,7 @@ | |||
| unset($haproxycfg['config'][0]['enable']); | |||
| } | |||
| } else { | |||
| $haproxycfg['config'][0] = array(); | |||
There was a problem hiding this comment.
That would help if just the one level is missing but a better fix is to not add this line, but to use the new access functions to read the configuration variable below. I'll post a follow-up comment on that line.
| @@ -135,6 +135,7 @@ | |||
| unset($haproxycfg['config'][0]['enable']); | |||
| } | |||
| } else { | |||
| $haproxycfg['config'][0] = array(); | |||
| $haproxycfg['config'][0]['enable'] = 'off'; | |||
There was a problem hiding this comment.
Using our new access functions, this line would become:
array_set_path($haproxycfg, 'config/0/enable', 'off');That way is safe no matter what parts may be missing from the configuration data.
|
Thanks!, @jim-p for the quick review. As a total php (and pfSense) newbie, I'd like to attempt making the requested changes. But I need some time to work through your comments. This is a learning process for me. If I can't grok it all, I'll let you know here. Otherwise, please forgive in advance any additional effort on your part to correct my feeble attempts and, maybe, ultimately, having to do it yourself. :-) |
|
Go for it! If you get stuck, let us know. Given that it's a small change it's no trouble at all if we have to make a fix manually, so no worries either way. |
Cosmetic change only
|
I reworked the whole It may be overkill -- or even wrong -- but I think that if the project needs to move to these functions because of PHP 8, it might be useful to work them in everywhere the code is revised instead of leaving a mixture of old and new configuration access styles. I tested on my CE 2.7 by starting and stopping HAProxy and visually inspecting Status, Services in both enabled and disable state to ensure that it was not possible to start from the Status, Services dialog if HAProxy is disabled. I also tested access to the sites HAProxy is proxying to ensure that in fact this change does not affect the ability to actually stop and start the package. For some reason, I can't seem to fix the indentation, which has resulted in a second, oddball commit. It looks fine in VS Code in my local fork of the repo. |
PORTREVISION = 1
There was a problem hiding this comment.
We do intend to fully convert everything in base and packages over to the new config access functions over time, but it's an ongoing process and there is only so much developer time to go around.
If you look at recent commits by us, there is a steady stream of packages being converted (including FRR in the last week or so), it just takes time and the larger/more complex packages take even more time.
Thanks!
| if (is_array($haproxycfg['config'][0])) { | ||
| unset($haproxycfg['config'][0]['enable']); | ||
| if (array_get_path($haproxycfg, 'config/0/enable')) { | ||
| array_set_path($haproxycfg, 'config/0/enable', null); |
There was a problem hiding this comment.
This one should probably be array_del_path($haproxycfg, 'config/0/enable') rather than setting it to null. It may happen to work out the same in most places the way the logic is coded but it's better to go that route because the old code was unsetting it rather than setting it to null.
The leading whitespace there is spaces, not tabs, but I see you mentioned a potential issue in your editor. If your editor is set to use leading spaces it may be making it difficult to use tabs there, but I would have expected it to do the same on the block below and it didn't, which is odd.
|
@jim-p: I actually thought about using Also, I was unsure of exactly what the logic was doing. It seemed to me, just reading the code cold, that the Anyway, I hope this commit better conforms to the project's standards overall. Thanks. |
|
I merged this in manually so I could fix that whitespace issue, and also I made a similar change on the Thanks! |
|
@jim-p, thank you! Appreciate the chance to work with the pfSense maintainers. |
In pfSense CE 2.7, PHP has been updated to 8.2.6. This version of PHP is stricter in using variables.
This fixes the issue described in 14445 (https://redmine.pfsense.org/issues/14445) by explicitly defining an array before assigning a value to an element of that array.