https://redmine.pfsense.org/https://redmine.pfsense.org/favicon.ico?16780521162023-02-22T11:50:09ZpfSense bugtrackerpfSense - Regression #14015: Alias list is not sortedhttps://redmine.pfsense.org/issues/14015?journal_id=658142023-02-22T11:50:09ZJim Pingle
<ul><li><strong>Status</strong> changed from <i>New</i> to <i>Feedback</i></li><li><strong>% Done</strong> changed from <i>0</i> to <i>100</i></li></ul><p>Applied in changeset <a class="changeset" title="Correct alias sort order. Fixes #14015" href="https://redmine.pfsense.org/projects/pfsense/repository/2/revisions/4342d179afe732baf3a73e2db4381b57b1aa7703">4342d179afe732baf3a73e2db4381b57b1aa7703</a>.</p> pfSense - Regression #14015: Alias list is not sortedhttps://redmine.pfsense.org/issues/14015?journal_id=662282023-03-16T21:05:46ZJim Pingle
<ul><li><strong>Status</strong> changed from <i>Feedback</i> to <i>In Progress</i></li><li><strong>% Done</strong> changed from <i>100</i> to <i>90</i></li></ul><p>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.</p>
<p>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.)</p> pfSense - Regression #14015: Alias list is not sortedhttps://redmine.pfsense.org/issues/14015?journal_id=662312023-03-17T03:19:27ZDanilo Zrenjanin
<ul></ul><p>I couldn't reproduce the issue on the:</p>
<pre>
23.01-RELEASE (amd64)
built on Fri Feb 10 20:06:33 UTC 2023
FreeBSD 14.0-CURRENT
</pre>
<p>I made an extensive list of aliases that were getting sorted by name alphabetically after saving. Can you provide an example to test?</p> pfSense - Regression #14015: Alias list is not sortedhttps://redmine.pfsense.org/issues/14015?journal_id=662332023-03-17T06:35:54ZJim Pingle
<ul></ul><p>Danilo Zrenjanin wrote in <a href="#note-3">#note-3</a>:</p>
<blockquote>
<p>I couldn't reproduce the issue on the:<br />I made an extensive list of aliases that were getting sorted by name alphabetically after saving. Can you provide an example to test?</p>
</blockquote>
<p>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.</p> pfSense - Regression #14015: Alias list is not sortedhttps://redmine.pfsense.org/issues/14015?journal_id=662342023-03-17T09:27:51ZJim Pingle
<ul></ul><p>This was also broken again by <a class="changeset" title="Aliases config access refactor by brd" href="https://redmine.pfsense.org/projects/pfsense/repository/2/revisions/29cd08ea0da6246ad416e33b3788c05c0b0a5172">29cd08ea0da6246ad416e33b3788c05c0b0a5172</a> during a rector pass. It was changed back to <code>asort</code> and the sort-on-save part was lost in that change. We can't rely on <code>asort</code> 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.</p>
<p>There is <code>SORT_FLAG_CASE</code> but it doesn't actually work as expected here, on its own or combined with others, such as <code>SORT_STRING | SORT_FLAG_CASE | SORT_NATURAL</code>, it still sorts either all the uppercase first or all the uppercase last.</p> pfSense - Regression #14015: Alias list is not sortedhttps://redmine.pfsense.org/issues/14015?journal_id=662402023-03-17T11:00:47ZJim Pingle
<ul><li><strong>Status</strong> changed from <i>In Progress</i> to <i>Feedback</i></li><li><strong>% Done</strong> changed from <i>90</i> to <i>100</i></li></ul><p>Should be fixed by <a class="changeset" title="Improve alias sorting (again). Issue #14015 asort does not handle natural case-insensitive sorti..." href="https://redmine.pfsense.org/projects/pfsense/repository/2/revisions/5c4a6ada1867a7d6ec13461680d8309d154c90b1">5c4a6ada1867a7d6ec13461680d8309d154c90b1</a> seems to be OK in various tests I've done.</p>
<p>Because <code>asort</code> 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.</p>
<p>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.</p> pfSense - Regression #14015: Alias list is not sortedhttps://redmine.pfsense.org/issues/14015?journal_id=662412023-03-17T11:03:30ZJim Pingle
<ul></ul><p>Also of note: I added another patch to the system patches package which applies on top of the previous patch (<code>4342d179afe732baf3a73e2db4381b57b1aa7703</code>) 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.</p>
<pre><code class="diff syntaxhl"><span class="gh">diff --git a/src/usr/local/www/firewall_aliases.php b/src/usr/local/www/firewall_aliases.php
index 52c2c28503..8c2c2a832d 100644
</span><span class="gd">--- a/src/usr/local/www/firewall_aliases.php
</span><span class="gi">+++ b/src/usr/local/www/firewall_aliases.php
</span><span class="p">@@ -114,7 +114,8 @@</span> display_top_tabs($tab_array);
</thead>
<tbody>
<?php
<span class="gd">- foreach (msort($a_aliases, 'name') as $i => $alias):
</span><span class="gi">+ uasort($a_aliases, function ($a, $b) { return strnatcasecmp($a['name'], $b['name']); });
+ foreach ($a_aliases as $i => $alias):
</span> unset ($show_alias);
switch ($tab) {
case "all":
</code></pre>
<p>The other changes in the commit that fixed this aren't relevant on 23.01.</p> pfSense - Regression #14015: Alias list is not sortedhttps://redmine.pfsense.org/issues/14015?journal_id=662972023-03-23T07:56:48ZDanilo Zrenjanin
<ul><li><strong>Status</strong> changed from <i>Feedback</i> to <i>Resolved</i></li></ul><p>I tested the patch with different upper-lower case setups. The patch works fine.</p>
<p>I am marking this ticket resolved.</p>