Regression #14015
closed
Alias list is not sorted
100%
Description
The alias list on firewall_aliases.php is not sorted as expected. The array is run through asort() which isn't sorting based on alias name, it should be run through msort($a_aliases, 'name') instead.
To me, I have a fix ready.
Updated by Jim Pingle almost 2 years ago
- Status changed from New to Feedback
- % Done changed from 0 to 100
Applied in changeset 4342d179afe732baf3a73e2db4381b57b1aa7703.
Updated by Jim Pingle almost 2 years ago
- Status changed from Feedback to In Progress
- % Done changed from 100 to 90
Looks like there may be an issue if aliases are not sorted in the config, the ID in the list may not match the ID in the config array.
Sorting the same way before looking up the ID to edit would rectify that (in lieu of using the name as the key, which may be better long term.)
Updated by Danilo Zrenjanin almost 2 years ago
I couldn't reproduce the issue on the:
23.01-RELEASE (amd64) built on Fri Feb 10 20:06:33 UTC 2023 FreeBSD 14.0-CURRENT
I made an extensive list of aliases that were getting sorted by name alphabetically after saving. Can you provide an example to test?
Updated by Jim Pingle almost 2 years ago
Danilo Zrenjanin wrote in #note-3:
I couldn't reproduce the issue on the:
I made an extensive list of aliases that were getting sorted by name alphabetically after saving. Can you provide an example to test?
Not yet, I'm just basing this off a report on the forum. I have not reproduced it yet nor attempted to fix it, that's why I marked it back as "in progress". It also works fine for me when I tried it on a couple different systems last night, but I'll see what I can come up with today yet.
Updated by Jim Pingle almost 2 years ago
This was also broken again by 29cd08ea0da6246ad416e33b3788c05c0b0a5172 during a rector pass. It was changed back to asort and the sort-on-save part was lost in that change. We can't rely on asort because it's only case sensitive so it sorts them out of order if there are aliases that start with both upper and lower case letters, and there isn't a flag to pass to asort which makes it case insensitive that I see.
There is SORT_FLAG_CASE but it doesn't actually work as expected here, on its own or combined with others, such as SORT_STRING | SORT_FLAG_CASE | SORT_NATURAL, it still sorts either all the uppercase first or all the uppercase last.
Updated by Jim Pingle almost 2 years ago
- Status changed from In Progress to Feedback
- % Done changed from 90 to 100
Should be fixed by 5c4a6ada1867a7d6ec13461680d8309d154c90b1 seems to be OK in various tests I've done.
Because asort does not handle natural case-insensitive sorting of multi-dimensional arrays properly, I added a custom sort which preserves the index keys but does a natural case-insensitive sort on the alias names.
The recent rector pass also removed sort-on-save which we didn't really need, but it was nice to have the aliases sorted in the ruleset so I preserved that behavior by using the same unction when getting the aliases to put in the ruleset.
Updated by Jim Pingle almost 2 years ago
Also of note: I added another patch to the system patches package which applies on top of the previous patch (4342d179afe732baf3a73e2db4381b57b1aa7703) so a user can apply both to get the new behavior on 23.01 since the code on 23.05 diverged too much to apply the new commit directly.
diff --git a/src/usr/local/www/firewall_aliases.php b/src/usr/local/www/firewall_aliases.php
index 52c2c28503..8c2c2a832d 100644
--- a/src/usr/local/www/firewall_aliases.php
+++ b/src/usr/local/www/firewall_aliases.php
@@ -114,7 +114,8 @@ display_top_tabs($tab_array);
</thead>
<tbody>
<?php
- foreach (msort($a_aliases, 'name') as $i => $alias):
+ uasort($a_aliases, function ($a, $b) { return strnatcasecmp($a['name'], $b['name']); });
+ foreach ($a_aliases as $i => $alias):
unset ($show_alias);
switch ($tab) {
case "all":
The other changes in the commit that fixed this aren't relevant on 23.01.
Updated by Danilo Zrenjanin almost 2 years ago
- Status changed from Feedback to Resolved
I tested the patch with different upper-lower case setups. The patch works fine.
I am marking this ticket resolved.